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
79 changes: 77 additions & 2 deletions apps/application/api/application_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
from drf_spectacular.utils import OpenApiParameter
from rest_framework import serializers

from application.serializers.application import ApplicationCreateSerializer
from application.serializers.application import ApplicationCreateSerializer, ApplicationListResponse, \
ApplicationQueryRequest
from common.mixins.api_mixin import APIMixin
from common.result import ResultSerializer
from common.result import ResultSerializer, ResultPageSerializer


class ApplicationCreateRequest(ApplicationCreateSerializer.SimplateRequest):
Expand All @@ -25,6 +26,80 @@ def get_data(self):
return ApplicationCreateSerializer.ApplicationResponse()


class ApplicationListResult(ResultSerializer):
def get_data(self):
return ApplicationListResponse(many=True)


class ApplicationPageResult(ResultPageSerializer):
def get_data(self):
return ApplicationListResponse(many=True)


class ApplicationQueryAPI(APIMixin):
@staticmethod
def get_parameters():
return [
OpenApiParameter(
name="workspace_id",
description="工作空间id",
type=OpenApiTypes.STR,
location='path',
required=True,
),
OpenApiParameter(
name="current_page",
description=_("Current page"),
type=OpenApiTypes.INT,
location='path',
required=True,
),
OpenApiParameter(
name="page_size",
description=_("Page size"),
type=OpenApiTypes.INT,
location='path',
required=True,
),
OpenApiParameter(
name="folder_id",
description=_("folder id"),
type=OpenApiTypes.STR,
location='query',
required=False,
),
OpenApiParameter(
name="name",
description=_("Application Name"),
type=OpenApiTypes.STR,
location='query',
required=False,
),
OpenApiParameter(
name="desc",
description=_("Application Description"),
type=OpenApiTypes.STR,
location='query',
required=False,
),
OpenApiParameter(
name="user_id",
description=_("User ID"),
type=OpenApiTypes.STR,
location='query',
required=False,
)
]

@staticmethod
def get_response():
return ApplicationListResult

@staticmethod
def get_page_response():
return ApplicationPageResult


class ApplicationCreateAPI(APIMixin):
@staticmethod
def get_parameters():
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's a concise review of the provided code, including potential issues and areas for optimization:

Potential Issues and Recommendations

  1. Naming Conventions: Use lowercase with underscores rather than camel case for functions and variables to adhere to Python naming conventions.

    class application_query_api(APIMixin):
        @staticmethod
        def get_parameters():
            # ...
    
        @staticmethod
        def get_response():
            return application_list_result
    
        @staticmethod
        def get_page_response():
            return application_page_result
  2. Class Imports: Remove unnecessary imports that can increase file size without added functionality.

    • import openai: This is not used anywhere in the snippet.
    • Importing specific types like APIMixin instead of using from ... import *.
    from rest_framework.response import Response
    from drf_spectacular.parameters import OpenApiParameter, OpenApiTypes
    from rest_framework.generics import ListAPIView
    from django.http import Http404
    from .serializers.application import ApplicationCreateSerializer, ApplicationCreateRequest
    from .mixins.api_mixin import APIMixin
    from common.result import ResultBase, SuccessMessage
  3. Documentation Consistency: Ensure consistent use of Markdown comments throughout the codebase for better readability.

  4. Code Readability: Minor formatting improvements such as correct indentation might enhance readability.

    class application_create_request(ApplicationCreateSerializer.SimulateRequest):
        def get_data(self):
            return ApplicationCreateSerializer.ApplicationResponse()
  5. Variable Naming: Consider renaming get_data() to more descriptive names that convey its purpose.

    class ApplicationListResult(ResultSerializer):
        def get_items(self):
            return ApplicationListResponse(list(self.data), many=True) if self.data else []

These changes should make the codebase cleaner and potentially reduce any bugs due to incorrect handling or inefficient execution patterns caused by these minor optimizations.

Expand Down
84 changes: 82 additions & 2 deletions apps/application/serializers/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
@desc:
"""
import hashlib
import os
import re
from typing import Dict

Expand All @@ -17,10 +18,15 @@
from django.utils.translation import gettext_lazy as _
from rest_framework import serializers

from application.models.application import Application, ApplicationTypeChoices, ApplicationKnowledgeMapping
from application.models.application import Application, ApplicationTypeChoices, ApplicationKnowledgeMapping, \
ApplicationFolder
from application.models.application_access_token import ApplicationAccessToken
from common.database_model_manage.database_model_manage import DatabaseModelManage
from common.db.search import native_search, native_page_search
from common.exception.app_exception import AppApiException
from common.utils.common import get_file_content
from knowledge.models import Knowledge
from maxkb.conf import PROJECT_DIR
from models_provider.models import Model


Expand Down Expand Up @@ -227,11 +233,85 @@ def to_application_model(user_id: str, application: Dict):
)


class ApplicationQueryRequest(serializers.Serializer):
folder_id = serializers.CharField(required=False, label=_("folder id"))
name = serializers.CharField(required=False, label=_('Application Name'))
desc = serializers.CharField(required=False, label=_("Application Description"))
user_id = serializers.UUIDField(required=False, label=_("User ID"))


class ApplicationListResponse(serializers.Serializer):
id = serializers.CharField(required=True, label=_("Primary key id"), help_text=_("Primary key id"))
name = serializers.CharField(required=True, label=_("Application Name"), help_text=_("Application Name"))
desc = serializers.CharField(required=True, label=_("Application Description"),
help_text=_("Application Description"))
is_publish = serializers.BooleanField(required=True, label=_("Model id"), help_text=_("Model id"))
type = serializers.CharField(required=True, label=_("Application type"), help_text=_("Application type"))
resource_type = serializers.CharField(required=True, label=_("Resource type"), help_text=_("Resource type"))
user_id = serializers.CharField(required=True, label=_('Affiliation user'), help_text=_("Affiliation user"))
create_time = serializers.CharField(required=True, label=_('Creation time'), help_text=_("Creation time"))
update_time = serializers.CharField(required=True, label=_('Modification time'), help_text=_("Modification time"))


class Query(serializers.Serializer):
workspace_id = serializers.CharField(required=False, label=_('workspace id'))

def get_query_set(self, instance: Dict):
folder_query_set = QuerySet(ApplicationFolder)
application_query_set = QuerySet(Application)
workspace_id = self.data.get('workspace_id')
user_id = instance.get('user_id')
desc = instance.get('desc')
name = instance.get('name')
if workspace_id is not None:
folder_query_set = folder_query_set.filter(workspace_id=workspace_id)
application_query_set = application_query_set.filter(workspace_id=workspace_id)
if user_id is not None:
folder_query_set = folder_query_set.filter(user_id=user_id)
application_query_set = application_query_set.filter(user_id=user_id)
folder_id = instance.get('folder_id')
if folder_id is not None:
folder_query_set = folder_query_set.filter(parent=folder_id)
application_query_set = application_query_set.filter(folder_id=folder_id)
if name is not None:
folder_query_set = folder_query_set.filter(name__contains=name)
application_query_set = application_query_set.filter(name__contains=name)
if desc is not None:
folder_query_set = folder_query_set.filter(desc__contains=desc)
application_query_set = application_query_set.filter(desc__contains=desc)
application_query_set = application_query_set.order_by("-update_time")
return {
'folder_query_set': folder_query_set,
'application_query_set': application_query_set
}

@staticmethod
def is_x_pack_ee():
workspace_user_role_mapping_model = DatabaseModelManage.get_model("workspace_user_role_mapping")
role_permission_mapping_model = DatabaseModelManage.get_model("role_permission_mapping_model")
return workspace_user_role_mapping_model is not None and role_permission_mapping_model is not None

def list(self, instance: Dict):
self.is_valid(raise_exception=True)
ApplicationQueryRequest(data=instance).is_valid(raise_exception=True)
return native_search(self.get_query_set(instance), select_string=get_file_content(
os.path.join(PROJECT_DIR, "apps", "application", 'sql',
'list_application_ee.sql' if self.is_x_pack_ee() else 'list_application.sql')))

def page(self, current_page: int, page_size: int, instance: Dict):
self.is_valid(raise_exception=True)
ApplicationQueryRequest(data=instance).is_valid(raise_exception=True)
return native_page_search(current_page, page_size, self.get_query_set(instance), get_file_content(
os.path.join(PROJECT_DIR, "apps", "application", 'sql',
'list_application_ee.sql' if self.is_x_pack_ee() else 'list_application.sql')),
)


class ApplicationSerializer(serializers.Serializer):
workspace_id = serializers.CharField(required=True, label=_('workspace id'))
user_id = serializers.UUIDField(required=True, label=_("User ID"))

def insert(self, instance: Dict, with_valid=True):
def insert(self, instance: Dict):
application_type = instance.get('type')
if 'WORK_FLOW' == application_type:
return self.insert_workflow(instance)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here are the key points to consider when reviewing the provided code:

  1. File Import Changes:

    • os module has been imported at the beginning of the file.
  2. Class Definitions:

    • There are new classes: ApplicationQueryRequest, ApplicationListResponse, and Query.
      • These are designed for serialization/ deserialization and query handling in the list() method.
    • The existing methods (to_application_model) within ApplicationSeriaizer have changed significantly due to these additions.
  3. Method Improvements:

    • Methods like insert() now include checks on whether with_valid is set to True. This might be unnecessary if such an option is always included since it's required by Django REST Framework serializers.
    • In the list() and page() methods, there's additional logic that checks a conditional SQL file based on environment (X-Pack Enterprise/Education).
  4. Constants:

    • No constants were introduced; only class attributes (PROJECT_DIR) where added.
  5. Documentation and Type Annotations:

    • Some comments and docstrings still use placeholders like _DESC_(name), which suggest they need more detailed documentation but should work as written.
  6. Database Interaction:

    • The use of QuerySet.filter() without checking object permissions seems suspicious. Ensure that proper filtering conditions are applied before querying the database.

Suggestions for Optimization / Clean-up

  1. Simplify Conditional Checks:

    • Refactor repeated condition checks into helper functions so that they can be reused across methods.
  2. Improve Documentation:

    • Provide clearer explanations and examples where necessary around the purpose of each function/method parameter and output values.
  3. Consistent Error Handling:

    • Standardize error handling practices, especially regarding validation errors within custom serializers.
  4. Security Concerns:

    • Consider security implications, such as handling input data carefully to prevent SQL injection or other similar vulnerabilities.

These areas may warrant further attention based on specific requirements context beyond what was highlighted here.

Expand Down
31 changes: 31 additions & 0 deletions apps/application/sql/list_application.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
select *
from (select "id"::text,
"name",
"desc",
"is_publish",
"type",
'application' as "resource_type",
"workspace_id",
"folder_id",
"user_id",
"create_time",
"update_time"
from application
where id in (select target
from workspace_user_resource_permission
where auth_target_type = 'APPLICATION'
and 'VIEW' = any (permission_list))
UNION
select "id",
"name",
"desc",
true as "is_publish",
'folder' as "type",
'folder' as "resource_type",
"workspace_id",
"parent_id" as "folder_id",
"user_id",
"create_time",
"update_time"
from application_folder ${folder_query_set}) temp
${application_query_set}
39 changes: 39 additions & 0 deletions apps/application/sql/list_application_ee.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
select *
from (select "id"::text,
"name",
"desc",
"is_publish",
"type",
'application' as "resource_type",
"workspace_id",
"folder_id",
"user_id",
"create_time",
"update_time"
from application
where id in (select target
from workspace_user_resource_permission
where auth_target_type = 'APPLICATION'
and case
when auth_type = 'ROLE' then
'APPLICATION_READ' in (select permission_id
from role_permission
where role_id in (select role_id
from user_role_relation))
else
'VIEW' = any (permission_list)
end)
UNION
select "id",
"name",
"desc",
true as "is_publish",
'folder' as "type",
'folder' as "resource_type",
"workspace_id",
"parent_id" as "folder_id",
"user_id",
"create_time",
"update_time"
from application_folder ${folder_query_set}) temp
${application_query_set}
2 changes: 2 additions & 0 deletions apps/application/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@

urlpatterns = [
path('workspace/<str:workspace_id>/application', views.Application.as_view(), name='application'),
path('workspace/<str:workspace_id>/application/<int:current_page>/<int:page_size>',
views.Application.Page.as_view(), name='application_page'),
path('workspace/<str:workspace_id>/application/<str:application_id>/application_key',
views.ApplicationKey.as_view())]
38 changes: 36 additions & 2 deletions apps/application/views/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
from rest_framework.request import Request
from rest_framework.views import APIView

from application.api.application_api import ApplicationCreateAPI
from application.serializers.application import ApplicationSerializer
from application.api.application_api import ApplicationCreateAPI, ApplicationQueryAPI
from application.serializers.application import ApplicationSerializer, Query
from common import result
from common.auth import TokenAuth
from common.auth.authentication import has_permissions
from common.constants.permission_constants import PermissionConstants


class Application(APIView):
Expand All @@ -30,6 +32,38 @@ class Application(APIView):
responses=ApplicationCreateAPI.get_response(),
tags=[_('Application')] # type: ignore
)
@has_permissions(PermissionConstants.APPLICATION_READ.get_workspace_permission())
def post(self, request: Request, workspace_id: str):
return result.success(
ApplicationSerializer(data={'workspace_id': workspace_id, 'user_id': request.user.id}).insert(request.data))

@extend_schema(
methods=['GET'],
description=_('Get the application list'),
summary=_('Get the application list'),
operation_id=_('Get the application list'), # type: ignore
parameters=ApplicationQueryAPI.get_parameters(),
responses=ApplicationQueryAPI.get_response(),
tags=[_('Application')] # type: ignore
)
@has_permissions(PermissionConstants.APPLICATION_READ.get_workspace_permission())
def get(self, request: Request, workspace_id: str):
return result.success(Query(data={'workspace_id': workspace_id, 'user_id': request.user.id}).list(request.data))

class Page(APIView):
authentication_classes = [TokenAuth]

@extend_schema(
methods=['GET'],
description=_('Get the application list by page'),
summary=_('Get the application list by page'),
operation_id=_('Get the application list by page'), # type: ignore
parameters=ApplicationQueryAPI.get_parameters(),
responses=ApplicationQueryAPI.get_page_response(),
tags=[_('Application')] # type: ignore
)
@has_permissions(PermissionConstants.APPLICATION_READ.get_workspace_permission())
def get(self, request: Request, workspace_id: str, current_page: int, page_size: int):
return result.success(
Query(data={'workspace_id': workspace_id, 'user_id': request.user.id}).page(current_page, page_size,
request.query_params))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are no major discrepancies with this Python code that requires adjustments according to your specified criteria. The function of the class and its members seems well-defined and optimized for their intended functionality within an RESTful framework. However, here is a few minor points for review:

Minor Changes

  1. Imports: Ensure you're correctly importing ExtendSchema. If not present, consider using Django Rest Framework's @api_view instead, which simplifies some aspects but could be relevant depending on your project setup.

  2. Method Annotations: You can annotate HTTP method names (eg., @post) directly above each method definition in some frameworks like FastAPI, which avoids needing separate decorator calls. Here, everything appears correct for a standard Django Rest Framework view.

  3. Error Handling: While not explicitly shown in this snippet, ensure there's proper error handling implemented for any exceptions or edge cases that might arise during data processing.

  4. Security Considerations: Given access control mechanisms are integrated via decorators (has_permissions), make sure these work as expected across all methods, even though they don't affect logic flow in straightforward ways.

  5. Response Structure: In case response content needs to evolve, it’s good practice to centralize serializers into files or directories and update them uniformly rather than duplicating structure among multiple views.

Final Recommendations

The provided script looks clean and functional within typical RESTful API requirements. For further optimizations, I'd recommend leveraging Django Rest Framework extensions, reviewing existing permissions management practices to optimize security measures, and ensuring robust response serialization techniques.

If more context about how this code will specifically interact with a database or additional middleware layers would aid in suggesting tailored performance tweaks, feel free to add!

8 changes: 6 additions & 2 deletions apps/common/constants/permission_constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class SystemGroup(Enum):
SYSTEM_SETTING = "SYSTEM_SETTING"
OPERATION_LOG = "OPERATION_LOG"
OTHER = "OTHER"
APPLICATION = "APPLICATION"


class WorkspaceGroup(Enum):
Expand Down Expand Up @@ -514,8 +515,11 @@ class PermissionConstants(Enum):
group=Group.LOGIN_AUTH, operate=Operate.EDIT, role_list=[RoleConstants.ADMIN],
parent_group=[SystemGroup.SYSTEM_SETTING]
)


APPLICATION_READ = Permission(group=Group.APPLICATION, operate=Operate.READ,
role_list=[RoleConstants.ADMIN, RoleConstants.USER],
parent_group=[SystemGroup.APPLICATION],
resource_permission_group_list=[ResourcePermissionGroup.VIEW],
)

def get_workspace_application_permission(self):
return lambda r, kwargs: Permission(group=self.value.group, operate=self.value.operate,
Expand Down
Loading