-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: System operate log #3198
Conversation
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. |
[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 |
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={ |
There was a problem hiding this comment.
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:
-
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. -
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. -
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. -
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. -
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)) |
There was a problem hiding this comment.
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
andencryption_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.
- Use more efficient queries where applicable. For instance, filtering with
- 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 |
There was a problem hiding this comment.
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:
-
Function Naming: The function names can be more descriptive to improve readability. For example:
get_document_operation_object
should be renamed to something likeretrieve_document_details
.get_document_operation_object_batch
could becomebatch_retrieve_document_details
.get_knowledge_document_operation_object
might be clearer ascompose_document_and_knowledge
.
-
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. -
Return Statement: Ensure that each function returns a dictionary unless specified otherwise. This makes the code easier to understand and maintain.
-
Null Check: Instead of checking
if document_model is not None
, usebool(document_model)
which is more concise. -
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.
feat: System operate log