Skip to content

feat: add serializer_class to Operate classes in model, module, and tool APIs #2929

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

Closed
wants to merge 1 commit into from
Closed

feat: add serializer_class to Operate classes in model, module, and tool APIs #2929

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
3 changes: 3 additions & 0 deletions apps/models_provider/views/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def get(self, request: Request):

class Operate(APIView):
authentication_classes = [TokenAuth]
serializer_class = ModelSerializer

@extend_schema(methods=['PUT'],
description=_('Update model'),
Expand Down Expand Up @@ -101,6 +102,7 @@ def get(self, request: Request, workspace_id: str, model_id: str):

class ModelParamsForm(APIView):
authentication_classes = [TokenAuth]
serializer_class = ModelSerializer

@extend_schema(methods=['GET'],
description=_('Get model parameter form'),
Expand Down Expand Up @@ -142,6 +144,7 @@ def get(self, request: Request, workspace_id: str, model_id: str):

class PauseDownload(APIView):
authentication_classes = [TokenAuth]
serializer_class = ModelSerializer

@extend_schema(methods=['PUT'],
description=_('Pause model download'),
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 provided code looks generally solid for handling API views with token-based authentication and a model serializer. However, there are some points to consider:

  1. ModelSerializer is Not Used: The serializer_class fields seem redundant because both Operate, ModelParamsForm, and PauseDownload views all have these attributes but do not utilize them within their respective methods.

  2. No Error Handling or Validation: Although this isn't explicitly shown here, in a production environment it would be wise to include error handling and validation, especially on the PUT methods like _update_model.

  3. API Endpoints Documentation: You might want to ensure that the description strings from @extend_schema are properly documented if used elsewhere (not included in this snippet).

Here's an optimized version of the class without unnecessary attributes:

from rest_framework.viewsets import ModelViewSet

class OperationalActions(ModelViewSet):
    authentication_classes = [TokenAuth]
    
    # Assuming you have relevant models and serializers
    serializer_class = YourModelSerializer
    
    @action(detail=True, methods=['put'], url_name='update-model', read_only=False)
    def update_model(self, request, pk=None):
        """
        Update an existing model.
        
        :param request: HTTP request containing the updated data.
        :param pk: Primary key of the model to update.
        :return: Updated model instance.
        """
        return self.update(request)

# Replace 'YourModel' with the actual model name derived from SerializerClass
operational_set = OperationalActions.as_view({'put': 'update_model'})

This approach leverages Django REST Framework (DRF) to handle CRUD operations more efficiently and reduces redundancy. Note that this assumes you're using DRF; if not, additional customization would be needed. Let me know if further clarification or changes are required!

Expand Down
1 change: 1 addition & 0 deletions apps/modules/views/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def post(self, request: Request, workspace_id: str, source: str):

class Operate(APIView):
authentication_classes = [TokenAuth]
serializer_class = ModuleSerializer

@extend_schema(methods=['PUT'],
description=_('Update module'),
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 snippet provided appears to be part of an Django REST Framework API with a ModuleSerializer. However, it lacks a closing bracket for the class Operate(APIView) declaration. Here is the corrected version:

from rest_framework.views import APIView
from rest_framework.authentication import TokenAuth
from drf_spectacular.utils import extend_schema

class Operate(APIView):
    authentication_classes = [TokenAuth]
    # Add any additional imports or configurations here

Additionally, if you have defined a serializers class named ModuleSerializer, ensure that your project's settings include 'rest_framework' and `drfr_spectacular'. It should also have 'rest_framework.schemas.openapi' in INSTALLED_APPS:

INSTALLED_APPS = [
    ...
    'rest_framework',
    'drf_spectacular'
],

Make sure all necessary serializers are properly imported into this view. The use case for having both 'TokenAuth' for authentication and 'drfr_SpecSchemaUtils.extend_schema()' for schema definition seems unnecessary without more context.

Overall, make sure all required libraries and settings are correctly configured based on the intended functionality of the API endpoint. If this is just part of a larger application framework, consider reviewing those parts as well.

Expand Down
1 change: 1 addition & 0 deletions apps/tools/views/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def post(self, request: Request, workspace_id: str):

class Operate(APIView):
authentication_classes = [TokenAuth]
serializer_class = ToolSerializer

@extend_schema(methods=['PUT'],
description=_('Update tool'),
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 provided appears to be incomplete and lacks additional details such as ToolSerializer, the _ function (translating text), and how the PUT method updates a tool within the given workspace ID. Here is an optimized version of what it should look like:

from rest_framework.permissions import TokenAuth
from django.views.decorators.csrf import csrf_exempt
from rest_framework.response import Response
fromrest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST

class PostAPIView(APIView):
    def post(self, request: Request, workspace_id: str):
        # Implement this logic for saving post data
        return Response({"message": "Post created successfully"}, status=HTTP_200_OK)

Comments on Optimization Suggestions:

  1. Authentication Classes: Ensure the TokenAuth class is correctly imported from django.contrib.auth.models.

  2. Serializers: The serializer_class attribute specifies which serializer instance will be used to serialize/deserialize input/output data based on its fields.

  3. Method Definitions: Methods defined inside classes should use the correct format (e.g., def put() instead of just put()).

    curl -X PUT http://example.com/api/workspace/{workspace_id}/operate/ \
    -H "Authorization: Token your_token_here" \
    -H "Content-Type: application/json" \
    -d '{"tool_data": {"name": "new_tool"}}'

  4. Error Messages: It's advisable to include meaningful error messages ({"error": "..."}) if validation fails during POST requests and to handle exceptions properly throughout the API.

  5. Data Validation: Consider using Django REST Framework’s serializers to automatically validate data against defined rules before saving or processing it within the viewsets/models/controllers.

Overall, while not shown here, other improvements could include proper URL routing setup (using urlpatterns in settings.py) and ensuring that tools being operated upon have appropriate permissions to update them.

Expand Down
Loading