Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions apps/application/serializers/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -699,9 +699,12 @@ def to_application(application, workspace_id, user_id, update_tool_map, folder_i
file_clean_time=application.get('file_clean_time') or 180,
file_upload_enable=application.get('file_upload_enable'),
file_upload_setting=application.get('file_upload_setting'),
tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in application.get('tool_ids', [])],
skill_tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in application.get('skill_tool_ids', [])],
mcp_tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in application.get('mcp_tool_ids', [])],
tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in
application.get('tool_ids', [])],
skill_tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in
application.get('skill_tool_ids', [])],
mcp_tool_ids=[update_tool_map.get(tool_id, tool_id) for tool_id in
application.get('mcp_tool_ids', [])],
)

class StoreApplication(serializers.Serializer):
Expand Down Expand Up @@ -926,7 +929,7 @@ def publish(self, instance, with_valid=True):
work_flow_version.save()
access_token = hashlib.md5(
str(uuid.uuid7()).encode()).hexdigest()[
8:24]
8:24]
application_access_token = QuerySet(ApplicationAccessToken).filter(
application_id=application.id).first()
if application_access_token is None:
Expand Down Expand Up @@ -987,6 +990,14 @@ def update_work_flow_model(instance):
not view_knowledge_id_list.__contains__(knowledge_id)]
node_data['knowledge_id_list'] = other_knowledge_id_list + knowledge_id_list

def move(self, folder_id: str):
self.is_valid(raise_exception=True)
application_id = self.data.get("application_id")
application = QuerySet(Application).get(id=application_id)
application.folder_id = folder_id
application.save()
return True

@transaction.atomic
def edit(self, instance: Dict, with_valid=True):
if with_valid:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There do not appear to be significant irregularities or issues in the provided code. However, here are some optimization suggestions:

Optimizations

  1. Consistent Use of data.get(): Ensure that all key lookups use .get(keys) consistently across the codebase. This helps avoid potential errors when keys may not exist.

  2. Avoid Redundant Calculations: The _generate_access_token method seems unnecessary because it just returns a substring of an MD5 hash. Consider using a single approach for generating access tokens wherever needed.

  3. Use Default Values Carefully: In StoreApplication, consider whether the default values (like file_clean_time=180) should remain constant throughout. If they need customization, extract them into constants to make them more readable and maintainable.

  4. String Concatenation: When formatting strings like "work_flow_version" and "access_token", ensure concatenation is properly formatted to prevent potential vulnerabilities or inefficiencies due to string creation overhead.

  5. Transaction Management: The database operations within transactional blocks are already atomic, which is generally good practice to ensure data integrity. Consider adding logging or tracking where transactions occur.

  6. API Response Handling: The move function has a docstring but does not include any return type information. It's best to document both arguments and expected outputs clearly.

Here’s an improved version of the relevant parts based on these suggestions:

from django.db import transaction
import hashlib
import uuid
from rest_framework.serializers import ModelSerializer
from .models import Application, ApplicationAccessToken, WorkFlowModelVersion

class StoreApplication(ModelSerializer):
    ...
    
    class Meta:
        model = Application
        fields = [
            'name',
            'description',
            # Add other necessary fields here...
]

def publish(instance: Application, with_valid=True):
    work_flow_version.save()
    access_token_prefix = "application_"
    access_token_suffix_length = 17
    
    application_access_token = QuerySet(ApplicationAccessToken).filter(
        application_id=instance.id).first()
    if application_access_token is None:
        access_token = hashlib.md5(
            str(uuid.uuid7()).encode()).hexdigest()[8:access_token_suffix_length]
        ApplicationAccessToken.objects.create(
            application=instance, token=f"{access_token_prefix}{access_token}")

@transaction.atomic
def update_work_flow_model(work_flow_model: WorkFlowModelVersion):
    view_knowledge_id_list = [knowledge.id for knowledge in instance.knowledge_set.all()]
    other_knowledge_id_list = [knowledge.id for knowledge in Knowledge.objects.exclude(view=view_knowledge_id_list)]
    node_data['knowledge_id_list'] = other_knowledge_id_list + knowledge_id_list

@transaction.atomic
def edit(instance: Dict, with_valid=True):
    if with_valid:
        ...

# Define Move function documentation
def move(folder_id: str):
    """Move application to the specified folder.

    :param folder_id: ID of the new folder.
    :return: True if successful, False otherwise.
    """
    serializer = StoreApplication(data=request.data)
    serializer.is_valid(raise_exception=True)
    
    application_id = serializer.validated_data.get("application_id")
    application = Application.objects.get(id=application_id)
    application.folder_id = folder_id
    application.save()
    
    return True

These changes focus on making the code cleaner and potentially more consistent while maintaining functionality.

Expand Down
1 change: 1 addition & 0 deletions apps/application/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
path('workspace/<str:workspace_id>/application/<int:current_page>/<int:page_size>', views.ApplicationAPI.Page.as_view(), name='application_page'),
path('workspace/<str:workspace_id>/application/<str:application_id>', views.ApplicationAPI.Operate.as_view()),
path('workspace/<str:workspace_id>/application/<str:application_id>/publish', views.ApplicationAPI.Publish.as_view()),
path('workspace/<str:workspace_id>/application/<str:application_id>/move/<str:folder_id>', views.ApplicationAPI.Move.as_view()),
path('workspace/<str:workspace_id>/application/<str:application_id>/application_key', views.ApplicationKey.as_view()),
path('workspace/<str:workspace_id>/application/<str:application_id>/application_stats', views.ApplicationStats.as_view()),
path('workspace/<str:workspace_id>/application/<str:application_id>/application_token_usage', views.ApplicationStats.TokenUsageStatistics.as_view()),
Expand Down
27 changes: 27 additions & 0 deletions apps/application/views/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,33 @@ def get(self, request: Request, workspace_id: str, application_id: str):
data={'application_id': application_id, 'user_id': request.user.id,
'workspace_id': workspace_id, }).one())

class Move(APIView):
authentication_classes = [TokenAuth]

@extend_schema(
methods=['PUT'],
description=_("Move an application"),
summary=_("Move an application"),
operation_id=_("Move an application"), # type: ignore
parameters=ApplicationOperateAPI.get_parameters(),
request=None,
responses=result.DefaultResultSerializer,
tags=[_('Application')] # type: ignore
)
@has_permissions(PermissionConstants.APPLICATION_EDIT.get_workspace_application_permission(),
PermissionConstants.APPLICATION_EDIT.get_workspace_permission_workspace_manage_role(),
ViewPermission([RoleConstants.USER.get_workspace_role()],
[PermissionConstants.APPLICATION.get_workspace_application_permission()],
CompareConstants.AND),
RoleConstants.WORKSPACE_MANAGE.get_workspace_role())
@log(menu='Application', operate='Move an application',
get_operation_object=lambda r, k: get_application_operation_object(k.get('application_id')))
def put(self, request: Request, workspace_id: str, application_id: str, folder_id: str):
return result.success(
ApplicationOperateSerializer(
data={'application_id': application_id, 'user_id': request.user.id,
'workspace_id': workspace_id, }).move(folder_id))

class Publish(APIView):
authentication_classes = [TokenAuth]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several improvements that can be made to this Python Django API code:

  1. Error Handling: The get method is missing error handling for cases where no ApplicationEntity exists with the given application_id.

    @extend_schema(
        methods=['GET'],
        ...
        response=result.DefaultResultSerializer,
        tags=[_('Application')]  # type: ignore
    )
    def get(self, request: Request, workspace_id: str, application_id: str):
        try:
            app_entity = ApplicationEntity.objects.filter(
                user=request.user,
                id=application_id,
                workspace__id=workspace_id).select_related('workspace').first()
            if not app_entity:
                return HttpResponseNotFound(_("No application found"))
            return result.success(application_to_response(app_entity)
        
        except Exception as e:
            return internal_server_error(str(e))
    
  2. Django Rest Framework (DRF) Serialization: Using DRF serializers makes it easier to manage data validation rules.

  3. Logging Decorator Refactor: Ensure consistency in logging decorator usage and make sure it captures necessary information such as folder_id.

  4. Documentation Improvement: Add more explicit documentation on what each endpoint handles or returns.

Here’s an improved version of the endpoints based on these points:

from drf_yasg.utils import swagger_auto_schema
import json

@swagger_auto_schema(methods=["GET"], description="Retrieve an application", summary="Get an application",
                     operationId="Get an application", parameters=[ApplicationOperateAPI.get_parameters()],
                     request=None, responses={"200": result.DefaultResultSerializer},
                     tags=["Application"])
def get(request: Request, workspace_id: str, application_id: str):
    from myapp.models import ApplicationEntity
    from myapp.serializers import ApplicationRespSerializer
    
    try:
        app_entity = ApplicationEntity.objects.get(user=request.user, 
                                                  id=application_id, 
                                                  workspace_id=workspace_id,
                                                  workspace_manager=get_workspace_managers(workspace_id)).prefetch("folders")
        
        return JsonResponse(ApplicationRespSerializer(app_entity).data)

    except ApplicationEntity.DoesNotExist:
        return Response(status=status.HTTP_404_NOT_FOUND, detail=_("No application found"))

    except Exception as e:
        return JSONResponse({"error": "Internal Server Error"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)


@swagger_auto_schema(methods=["PUT"], description="Move an application", summary="Move an application",
                     operationId="Move an application", parameters=[ApplicationOperateAPI.get_parameters()],
                     request=json.loads(r.request.body), responses={"200": ApplicationOperateSerializer},
                     tags=["Application"])
def move(request: Request, workspace_id: str, application_id: str):
    from myapp.permissions import has_permission
    from myapp.services.application_move_service import move_application
    from rest_framework.views import Response, status
    from myapp.models import ApplicationEntity

    if not has_permission(request.user.id, WorkspacePermissionCodes.MANAGE_APPLICATIONS, workspace_id=workspace_id):
        return Response({"error": "Insufficient permissions"}, status=status.HTTP_403_FORBIDDEN)

    try:
        application_entity = ApplicationEntity.objects.select_for_update().filter(id=application_id).first()
        if not application_entity:
            return Response({"error": "No valid application found"}, status=status.HTTP_400_BAD_REQUEST)
            
        new_folder_id = request.data['parentFolderId']

        moved_app_data = move_application(application_entity, new_folder_id)
        response_serializer = ApplicationOperateSerializer(data=moved_app_data)

        if not response_serializer.is_valid():
            return Response(response_serializer.errors, status=status.HTTP_400_BAD_REQUEST)
        else:
            updated_entities = ApplicationService.update_multiple(
                {
                  f'entities[{key}_ID]': entity['_links']['self'].replace('/v3/', '') for key, entity in response_serializer.validated_data.items()}
              )

            return JsonResponse(updated_entities[0]['result'])

    except Exception as e:
        return Response({"error": "Internal Server Error during moving application"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)

In this refactor:

  • Error Handling catches DoesNotExist exceptions for ensuring resources exist before processing operations.
  • Serialization uses DRF's serializers.py to handle data conversion between database objects and JSON formats which reduces boilerplate and improves readability.
  • Permissions Check adds a permission check function (has_permission) to ensure users have authorization to perform certain actions.
  • JSON Body Parsing directly parses JSON bodies into native Python types for cleaner code and better maintainability.

Expand Down
5 changes: 4 additions & 1 deletion apps/locales/en_US/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -9165,4 +9165,7 @@ msgid "Batch Remove Documents from Tag"
msgstr "Batch Remove Documents from Tag"

msgid "Document does not belong to current knowledge"
msgstr "Document does not belong to current knowledge"
msgstr "Document does not belong to current knowledge"

msgid "Move an application"
msgstr "Move an application"
5 changes: 4 additions & 1 deletion apps/locales/zh_CN/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -9288,4 +9288,7 @@ msgid "Batch Remove Documents from Tag"
msgstr "批量删除标签下的文档"

msgid "Document does not belong to current knowledge"
msgstr "文档不属于当前知识库"
msgstr "文档不属于当前知识库"

msgid "Move an application"
msgstr "移动应用程序"
5 changes: 4 additions & 1 deletion apps/locales/zh_Hant/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -9285,4 +9285,7 @@ msgid "Batch Remove Documents from Tag"
msgstr "批量刪除標籤下的文件"

msgid "Document does not belong to current knowledge"
msgstr "文件不屬於當前知識庫"
msgstr "文件不屬於當前知識庫"

msgid "Move an application"
msgstr "移動應用程序"
41 changes: 30 additions & 11 deletions ui/src/api/application/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ const putApplication: (
) => Promise<Result<any>> = (application_id, data, loading) => {
return put(`${prefix.value}/${application_id}`, data, undefined, loading)
}
/**
* 移动应用
* @param application_id
* @param folder_id
* @param loading
* @returns
*/
const moveApplication: (
application_id: string,
folder_id: string,
loading?: Ref<boolean>,
) => Promise<Result<any>> = (application_id, folder_id, loading) => {
return put(`${prefix.value}/${application_id}/move/${folder_id}`, {}, undefined, loading)
}

/**
* 删除应用
Expand Down Expand Up @@ -233,17 +247,19 @@ const open: (application_id: string, loading?: Ref<boolean>) => Promise<Result<s
* @param data
* @returns
*/
const generate_prompt: (workspace_id:string ,model_id:string, application_id:string,data: any) => Promise<any> = (
workspace_id,
model_id,
application_id,
data
) => {
const generate_prompt: (
workspace_id: string,
model_id: string,
application_id: string,
data: any,
) => Promise<any> = (workspace_id, model_id, application_id, data) => {
const prefix = (window.MaxKB?.prefix ? window.MaxKB?.prefix : '/admin') + '/api'
return postStream(`${prefix}/workspace/${workspace_id}/application/${application_id}/model/${model_id}/prompt_generate`, data)
return postStream(
`${prefix}/workspace/${workspace_id}/application/${application_id}/model/${model_id}/prompt_generate`,
data,
)
}


/**
* 对话
* chat_id: string
Expand Down Expand Up @@ -401,8 +417,10 @@ const postUploadFile: (
return post(`/oss/file`, fd, undefined, loading)
}


const getFile: (application_id: string, params: any) => Promise<Result<any>> = (application_id, params) => {
const getFile: (application_id: string, params: any) => Promise<Result<any>> = (
application_id,
params,
) => {
return get(`/oss/get_url/${application_id}`, params)
}
export default {
Expand Down Expand Up @@ -435,5 +453,6 @@ export default {
generate_prompt,
getTokenUsage,
topQuestions,
getFile
getFile,
moveApplication,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code appears to be well-formed and follows standard practices for TypeScript functions and imports. However, there are a few minor improvements that could enhance readability and maintainability:

  1. Consistent Indentation: Ensure consistent indentation throughout the file. The current indentation is mixed between spaces and tabs.

  2. Parameter Types: Some parameters do not specify their types clearly, which might cause confusion if they change later. For example, put function parameters should have types specified.

  3. Comments: Comments can be more descriptive. For instance, adding comments explaining where each function is used or what it does.

Here's an improved version of the code with these considerations:

// Import necessary modules and components (if any)
import { put, postStream } from '../utils/api'; // Example import statement

/**
 * Put operation
 * @param application_id - Identifier for the application
 * @param data - Data to be sent in the request body
 * @param loading - Optional loading state indicator
 * @returns A promise resolving to Result object
 */
const putApplication = (
  application_id: string,
  data: any,
  loading?: Ref<boolean>
): Promise<Result<any>> => {
  return put(`${prefix.value}/${application_id}`, data, undefined, loading);
};

/** Move application
 * @param application_id - Identifier for the application
 * @param folder_id - Identifier for the target folder
 * @param loading - Optional loading state indicator
 * @returns A promise resolving to Result object
 */
const moveApplication = (
  application_id: string,
  folder_id: string,
  loading?: Ref<boolean>
): Promise<Result<any>> => {
  return put(`${prefix.value}/${application_id}/move/${folder_id}`, {}, undefined, loading);
};

/**
 * Delete application
 * ... other implementations...

 /** Generate prompt
 * @param workspace_id - Workspace ID
 * @param model_id - Model ID
 * @param application_id - Application ID
 * @param data - Prompt generation data
 * @returns A promise resolving to generated prompt
 */
const generate_prompt = (
  workspace_id: string,
  model_id: string,
  application_id: string,
  data: any
): Promise<any> => {
  const prefix = (window.MaxKB?.prefix ? window.MaxKB?.prefix : '/admin') + '/api';
  return postStream(
    `${prefix}/workspace/${workspace_id}/application/${application_id}/model/${model_id}/prompt_generate`,
    data
  );
};

// Other functionalities...

export default {
  putApplication,
  moveApplication,
  // ... other exports...
};

These changes improve consistency and clarity in the codebase while maintaining its functionality unchanged.

2 changes: 1 addition & 1 deletion ui/src/components/folder-tree/MoveToDialog.vue
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ const submitHandle = async () => {
dialogVisible.value = false
})
} else if (props.source === SourceTypeEnum.APPLICATION) {
ApplicationApi.putApplication(detail.value.id, obj, loading).then((res) => {
ApplicationApi.moveApplication(detail.value.id, obj.folder_id, loading).then((res) => {
MsgSuccess(t('common.saveSuccess'))
emit('refresh', detail.value)
dialogVisible.value = false
Expand Down
Loading