Skip to content

fix: improve module tree query to use Q object for filtering by workspace_id #2923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
liuruibin merged 1 commit into from
Apr 18, 2025
Merged

fix: improve module tree query to use Q object for filtering by workspace_id #2923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions apps/common/constants/permission_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ class PermissionConstants(Enum):
RoleConstants.USER])
TOOL_MODULE_EDIT = Permission(group=Group.TOOL, operate=Operate.EDIT, role_list=[RoleConstants.ADMIN,
RoleConstants.USER])
TOOL_MODULE_DELETE = Permission(group=Group.TOOL, operate=Operate.DELETE, role_list=[RoleConstants.ADMIN,
RoleConstants.USER])

TOOL_CREATE = Permission(group=Group.TOOL, operate=Operate.CREATE, role_list=[RoleConstants.ADMIN,
RoleConstants.USER])
Expand Down
4 changes: 4 additions & 0 deletions apps/modules/api/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ def get_request():
return ModuleEditRequest


class ModuleDeleteAPI(ModuleReadAPI):
pass


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

@shaohuzhang1 shaohuzhang1 Apr 18, 2025

Choose a reason for hiding this comment

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

The provided code appears to be incomplete and lacks some necessary components, such as imports and a main execution point. However, I can offer some general suggestions for improvement:

from flask import Flask, request

app = Flask(__name__)

class APIMixin:
    @staticmethod
    def get_parameters():
        # Placeholder for parameter retrieval logic
        params = request.args.to_dict()
        return params

def get_request():
    # Placeholder for handling HTTP requests
    return {"method": "GET", "params": None}

@app.route('/module', methods=['GET'])
def module_read():
    parameters = APIMixin.get_parameters()
    print(parameters)
    return {"message": "Module read API called"}

if __name__ == '__main__':
    app.run(debug=True)

Key Suggestions:

  1. Imports: Add appropriate import statements at the beginning of your script.
  2. APIMixin Implementation: Ensure you implement the logic inside APIMixin.get_parameters() to handle different request types (e.g., POST, PUT, DELETE).
  3. Flask Application Setup: Use Flask's routing feature (@app.route) to define how you want to serve endpoints.
  4. Complete Code Structure: Consider adding more functionalities to complete the module.
  5. Error Handling: Implement error handling for edge cases like missing required fields.

This setup provides a basic structure that can be expanded with additional features as needed.

Expand Down
11 changes: 9 additions & 2 deletions apps/modules/serializers/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-

import uuid_utils.compat as uuid
from django.db.models import QuerySet
from django.db.models import QuerySet, Q
from django.utils.translation import gettext_lazy as _
from rest_framework import serializers

Expand Down Expand Up @@ -83,6 +83,13 @@ def one(self):
module = QuerySet(Module).filter(id=self.data.get('id')).first()
return ModuleSerializer(module).data

def delete(self):
self.is_valid(raise_exception=True)
if self.data.get('id') == 'root':
raise serializers.ValidationError(_('Cannot delete root module'))
Module = get_module_type(self.data.get('source'))
QuerySet(Module).filter(id=self.data.get('id')).delete()


class ModuleTreeSerializer(serializers.Serializer):
workspace_id = serializers.CharField(required=True, allow_null=True, allow_blank=True, label=_('workspace id'))
Expand All @@ -91,6 +98,6 @@ class ModuleTreeSerializer(serializers.Serializer):
def get_module_tree(self):
self.is_valid(raise_exception=True)
Module = get_module_type(self.data.get('source'))
nodes = Module.objects.filter(workspace_id=self.data.get('workspace_id')).get_cached_trees()
nodes = Module.objects.filter(Q(workspace_id=self.data.get('workspace_id'))).get_cached_trees()
serializer = ToolModuleTreeSerializer(nodes, many=True)
return serializer.data # 这是可序列化的字典
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Apr 18, 2025

Choose a reason for hiding this comment

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

No major issues were found. Some minor suggestions for improvement:

  1. Use QuerySet.all() instead of .objects directly, especially when getting all items:

    modules = Module.objects.filter(workspace_id=self.data.get('workspace_id')).all()

  2. Replace .get() with .filter().exists(), except in cases where you're sure there will be exactly one match.

  3. You might want to add a more descriptive custom validation error message instead of using default Django translations.

In summary, this code does not have serious flaws but follows best practices for handling queries in Django ORM and making it easier to expand upon.

14 changes: 13 additions & 1 deletion apps/modules/views/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from common.auth.authentication import has_permissions
from common.constants.permission_constants import Permission, Group, Operate
from common.result import result
from modules.api.module import ModuleCreateAPI, ModuleEditAPI, ModuleReadAPI, ModuleTreeReadAPI
from modules.api.module import ModuleCreateAPI, ModuleEditAPI, ModuleReadAPI, ModuleTreeReadAPI, ModuleDeleteAPI
from modules.serializers.module import ModuleSerializer, ModuleTreeSerializer


Expand Down Expand Up @@ -61,6 +61,18 @@ def get(self, request: Request, workspace_id: str, source: str, module_id: str):
data={'id': module_id, 'workspace_id': workspace_id, 'source': source}
).one())

@extend_schema(methods=['DELETE'],
description=_('Delete module'),
operation_id=_('Delete module'),
parameters=ModuleDeleteAPI.get_parameters(),
tags=[_('Module')])
@has_permissions(lambda r, kwargs: Permission(group=Group(kwargs.get('source')), operate=Operate.DELETE,
resource_path=f"/WORKSPACE/{kwargs.get('workspace_id')}"))
def delete(self, request: Request, workspace_id: str, source: str, module_id: str):
return result.success(ModuleSerializer.Operate(
data={'id': module_id, 'workspace_id': workspace_id, 'source': source}
).delete())


class ModuleTreeView(APIView):
authentication_classes = [TokenAuth]
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Apr 18, 2025

Choose a reason for hiding this comment

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

The provided code appears to be incomplete and contains a few irregularities and potential issues. Here's my analysis:

Irregularities and Potential Issues

  1. File Path: The file path modules/api/module.py suggests that this module handles API requests related to modules, but the imports and method definitions do not match this description.

  2. Missing Method Implementation: The ModuleTreeView class is imported at the top, but it seems like there should also be an implementation of methods such as get, post, put, etc. These methods are not present in the given snippet.

  3. Incomplete Error Handling: If you want to handle exceptions properly, consider adding more detailed error handling around database operations and permission checks.

  4. Version Mismatch: There might be a mismatch between the versions of packages if some changes have been made since 2021-09-01.

  5. Security Concerns: Ensure that parameter validation is robust and secure, especially with inputs like module_id.

  6. Django REST Framework Usage: If using Django REST Framework, make sure consistent use of mixins and base classes for views.

Optimization Suggestions

  1. Use DRF Mixins or Base Classes: For simplicity, consider using Django REST Framework (DRF) mixins or base classes to manage common view logic.

    from rest_framework.generics import ListAPIView, RetrieveAPIView, DestroyAPIView
from rest_framework.permissions import IsAuthenticatedOrReadOnly

@permission_required('your_permission_name')
@api_view(['DELETE'])
def delete_module(request, workspace_id, source, module_id):
    try:
        # Delete module logic here
        serializer = ModuleSerializer(queryset=Module.objects.filter(workspace__uuid=workspace_id, type=source, uuid(module_id)).first())
        return Response(serializer.data, status=status.HTTP_200_OK)
    except ObjectDoesNotExist:
        return Response({'error': 'Module does not exist'}, status=status.HTTP_404_NOT_FOUND)

  2. Database Indexes: Ensure that appropriate indexes are created on columns used frequently in queries to improve performance.

  3. Error Responses: Enhance error messages to provide more contextual information, which can help developers debug issues more easily.

Here's a minimal example demonstrating how you might structure these changes:

from django.contrib.auth.decorators import permission_required
from rest_framework.decorators import api_view
from rest_framework.response import Response
from rest_framework.status import HTTP_200_OK, HTTP_404_NOT_FOUND

# Assume ModuleSerializer and Module queryset are defined elsewhere

@permission_required('your_permission_name', raise_exception=True)
def delete_module(request, workspace_id, source, module_id):
    try:
        module = Module.objects.filter(workspace__uuid=workspace_id, type=source, uuid(module_id)).first()
        if module:
            module.delete()
            return Response(status=HTTP_200_OK)
        else:
            return Response({'error': 'Module does not exist'}, status=HTTP_404_NOT_FOUND)
    except Exception as e:
        return Response({'error': str(e)}, status=500)

This version assumes that:

  • ModuleSerializer includes a delete method.
  • Module model has type field instead of source.
  • Permission checking (Permission.group) needs to be adjusted based on actual permissions system setup.

Expand Down
Loading