Skip to content

feat: System operate log #3198

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.

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
merged 1 commit into from
Jun 5, 2025
Merged

feat: System operate log #3198

merged 1 commit into from
Jun 5, 2025

Conversation

shaohuzhang1
Copy link
Contributor

feat: System operate log

Copy link

f2c-ci-robot bot commented Jun 5, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

f2c-ci-robot bot commented Jun 5, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

get_knowledge_operation_object(keywords.get('knowledge_id')),
get_document_operation_object(keywords.get('document_id'))
)
)
def put(self, request: Request, workspace_id: str, knowledge_id: str, document_id: str):
return result.success(ParagraphSerializers.Association(
data={
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 for ParagraphView in Django is generally well-structured and follows best practices. However, there are a few areas with potential improvements and adjustments:

  1. Duplicate Logging Decorators: Both @log decorators are applied to methods that perform similar operations (create, migrate, batch_generate_related, edit_paragraph, delete_paragraph, add_problem_associations, disassociate_issue, and add_or_remove_relationships`). Consider consolidating these into a single decorator or extracting them into utility functions.

  2. Consistent Error Handling in Log Messages: Ensure that error messages include relevant details such as the request.data. This makes it easier to debug issues when errors occur.

  3. Use of Default Values for Parameters: Although not explicitly mentioned, parameters like paragraph_id should have default values if they might be optional to avoid runtime errors.

  4. Simplified Response Management: In places where response management is straightforward, consider using more concise syntax or built-in methods like return success(response) directly instead of creating an intermediate object (e.g., response = ...; return success(response)) where possible.

  5. Importing Order: The order of imports could potentially improve readability. Ensure that commonly used types and libraries are imported first to reduce redundancy at the beginning of each file.

Here's a simplified version of how the get method in one example operation can look:

from typing import Callable

def get_knowledge_operation_object(knowledge_id: int | None = None) -> Any:
    # Logic to retrieve knowledge operation object based on id
    pass

def get_document_operation_object(document_id: int | None = None) -> Any:
    # Logic to retrieve document operation object based on id
    pass

class ParagraphView(APIView):
    ...
    
    @get(operation_name='Retrieve Paragraph')
    def get(self, request: Request, workspace_id: str, knowledge_id: str, document_id: str, paragraph_id: int | None = None) -> Response:
        try:
            if paragraph_id is None:
                raise ValueError("Invalid paragraph ID")
            
            kwargs = {
                'workspace_id': workspace_id,
                'knowledge_id': knowledge_id,
                'document_id': document_id,
                'paragraph_id': paragraph_id
            }
            
            op_obj = get_knowledge_document_operation_object(keywords['knowledge_id'])
            doc_op_obj = get_document_operation_object(kwargs.get('document_id'))
 
             # Retrieve and return the paragraph data
             paragraph_data = ParagraphAPI().read(paragraph_id)
             
             return ApiResponse.success({
                 'paragraph': paragraph_data
             })
        except Exception as e:
            logger.error(f"Error retrieving paragraph (ID={kwargs['paragraph_id']}): {str(e)}", extra={'keywords': kwargs})
            return ApiResponse.failed(str(e))

This refactored version reduces repeated code and improves logging consistency. Adjustments specific to each view would require careful evaluation based on the actual implementation logic within those classes.

@@ -129,6 +171,8 @@ def get(self, request: Request, workspace_id: str, model_id: str):
responses=ProvideApi.ModelParamsForm.get_response(),
tags=[_('Model')]) # type: ignore
@has_permissions(PermissionConstants.MODEL_READ.get_workspace_permission())
@log(menu='model', operate='Save model parameter form',
get_operation_object=lambda r,k: get_model_operation_object(k.get('model_id')))
def put(self, request: Request, workspace_id: str, model_id: str):
return result.success(
ModelSerializer.ModelParams(data={'id': model_id}).save_model_params_form(request.data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code Review

Syntax Errors

  • There are no syntax errors in the provided code segment.

Potential Issues

  • File Structure: The @date field at the beginning of the file is missing its closing parenthesis, which is syntactically incorrect. Ensure you close all comments properly.
  • Imports: Some imports might not be needed based on context clues (e.g., query_params_to_single_dict and encryption_str seem unnecessary).
  • Function Definitions: Functions like get_edit_model_details, get_model_operation_object, etc., can be merged or simplified if they are not used elsewhere.
  • Logging Logic: Ensure that the logging function (log) supports all necessary attributes and parameters (menu, operate, get_operation_object, get_details). Check if these attributes match the expected input.

Optimization Suggestions

  • **Django ORM Efficiency`:
    • Use more efficient queries where applicable. For instance, filtering with .filter() and selecting specific fields using .values(). This reduces memory usage and improves performance.
  • Error Handling: Add explicit error handling to manage exceptions when dealing with database operations.
  • Code Readability: Although short now, consider adding some blank lines for better readability after certain blocks.

Suggested Fixes

# Remove extraneous characters in comment line
+from django.db.models import QuerySet
 from drf_spectacular.utils import extend_schema
 from rest_framework.views import APIView
 from django.utils.translation import gettext_lazy as _
@@ -14,11 +15,36 @@
 from common.auth import TokenAuth
 from common.auth.authentication import has_permissions
 from common.constants.permission_constants import PermissionConstants
+from common.log.log import log
 from common.result import result
+from common.utils.common import query_params_to_single_dict
 
-from models_provider.api.model import ModelCreateAPI, GetModelApi, ModelEditApi, ModelListResponse, DefaultModelResponse
-from models_provider.api.provide import ProvideApi
+from models_provider.models import Model
+
+def encryption_credential(credential):
+    """Encrypts credentials by recursively encrypting dictionary values."""
+    if isinstance(credential, dict):
+        return {k: encryption_str(v) for k, v in credential.items()}
+    return credential
  
-
-
+def get_edit_model_details(request):
+    """Extracts details relevant for logging and response formatting."""
+    path = request.path
+    body = request.data
+    query = request.query_params
+    return {
+        'path': path,
+        'body': {**body},
+        'query': query
+    }
  
   def get_model_operation_object(model_id):
       """Retrieves operation object based on model ID. Returns empty dictionary if not found."""
       model_model = QuerySet(models=Model).filter(id=model_id).first()
       return {"name": model_model.name} if model_model else {}
  
  
class Model(APIView):
@@ -22,6 +50,8 @@ class Model(APIView):
         requests.get(ModelCreateAPI.get_request()),
@@ -33,8 +57,7 @@ class Model(APIView):
                responses=ModelCreateAPI.get_response())
             @has_permissions(PermissionConstants.MODEL_CREATE.get_workspace_permission())
-           @log(menu='model', operate='Create model')
-           def post(self, request: Reques(workspace_id: str):
+           @log(menu='model', operate='Create model', get_operation_object=get_model_operation_object)
             return result.success(
                 ModelSerializer.create(data={**request.data, 'workspace_id': request.user.id}).insert(),

These changes make the code cleaner while ensuring proper functionality and maintainability.

'dataset_type': knowledge_dict.get("type", ""),
'document_name': document_dict.get("name", ""),
'document_type': document_dict.get("type", ""),
} No newline at end of file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your code is mostly clear and well-structured, but there are a few areas where you can make improvements for better performance and clarity:

  1. Function Naming: The function names can be more descriptive to improve readability. For example:

    • get_document_operation_object should be renamed to something like retrieve_document_details.
    • get_document_operation_object_batch could become batch_retrieve_document_details.
    • get_knowledge_document_operation_object might be clearer as compose_document_and_knowledge.
  2. Use of F-Strings: While f-strings are generally preferred, they can be overused. Replace all instances with .format() or directly interpolate strings in Python version >= 3.6.

  3. Return Statement: Ensure that each function returns a dictionary unless specified otherwise. This makes the code easier to understand and maintain.

  4. Null Check: Instead of checking if document_model is not None, use bool(document_model) which is more concise.

  5. Efficient Query Execution: Directly pass variables to method calls rather than using string concatenation within methods.

Here's the updated code incorporating these suggestions:

@@ -0,0 +1,35 @@
+from django.db.models import QuerySet
+
+from knowledge.models import Document

+
+
+def retrieve_document_details(document_id: str) -> dict:
+    document_model = QuerySet(model=Document).filter(id=document_id).first()
+    if document_model:
+        return {
+            "name": document_model.name,
+            "type": document_model.type,
+        }
+    return {}

+def batch_retrieve_document_details(document_id_list: str) -> dict:
+    document_models = QuerySet(Document).filter(id__in=document_id_list)
+    if document_models.exists():
+        return {
+            "name": ",".join(map(str, [doc.name for doc in document_models])),
+            'document_list': [
+                {'name': doc.name, 'type': doc.type} for doc in document_models
+            ],
+        }
+    return {}  

+def compose_document_and_knowledge(knowledge_dict: dict, document_dict: dict) -> dict:
+    name_part = "{}/{}".format(
        knowledge_dict.get("name", ""), 
        document_dict.get("name", "")
    )
    
    dataset_name = knowledge_dict.get("name", "")  
    dataset_desc = knowledge_dict.get("desc", "")
    dataset_type = knowledge_dict.get("type", "")
    
    document_name = document_dict.get("name", "")  
    document_type = document_dict.get("type", "")

    return {
        "name": name_part,
        "dataset_name": dataset_name,
        "dataset_desc": dataset_desc,
        "dataset_type": dataset_type,
        "document_name": document_name,
        "document_type": document_type,
    }

These changes focus on improving code clarity, readability, and efficiency without altering its functionality. Make sure to test the functions after making these changes to ensure they behave as expected.

@zhanweizhang7 zhanweizhang7 merged commit e63572b into v2 Jun 5, 2025
3 of 5 checks passed
@zhanweizhang7 zhanweizhang7 deleted the pr@v2@feat_system_operate_log branch June 5, 2025 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants