-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Support application、setting、user log recorder #2617
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/test-infra 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 |
@@ -337,5 +372,6 @@ class UserListView(APIView): | |||
responses=result.get_api_array_response(UserSerializer.Query.get_response_body_api()), | |||
tags=[_("User")]) | |||
@has_permissions(PermissionConstants.USER_READ) | |||
@log(menu='User', operate='Get user list by type') | |||
def get(self, request: Request, type): | |||
return result.success(UserSerializer().listByType(type, request.user.id)) |
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 contains several improvements and optimizations:
-
Imports: The imports have been corrected to reflect the actual file names in the directory structure (
common.auth.authentication
tocommon.auth.authenticate
, etc.). -
Class Methods: Class methods like
login
,register
, etc., now include annotations for logging, which will help trace each API call effectively. -
Logging Functionality:
- A
_get_details
function captures HTTP request details such as path, body (with sensitive data masked), and query parameters. This helps in debugging by providing comprehensive context about requests. - Annotations are used with the
@log
decorator to specify the menu item and operation performed, allowing tracing through application logs.
- A
-
Encryption Usage: An example of using the
encryption
helper method withinlog
is shown when handling passwords. This is not recommended; instead, consider encrypting passwords at rest wherever possible. -
Docstrings and Comments:
- All classes, methods, and sections that should be tested have appropriate docstring comments explaining their purpose.
Example of How you can use Log Decorator
Here's how you might apply the @log
decorator:
class Profile(APIView):
@log(menu='System Parameters', operate='Get MaxKB Related Information')
def get(self, request: Request) -> Response:
return result.success(SystemSerializer.get_profile())
In this case, logging.info
would be called during the execution of the Profile.get()
method to log the action performed, along with additional contextual information from the _get_details
function.
Please ensure you have properly set up logging configuration in Python (settings.py
) or another relevant location where your logging calls reside. Also make sure to handle exceptions appropriately across different APIs to maintain robustness.
@@ -235,6 +256,7 @@ class ModelForm(APIView): | |||
manual_parameters=ProvideApi.ModelForm.get_request_params_api(), | |||
tags=[_('model')]) | |||
@has_permissions(PermissionConstants.MODEL_READ) | |||
@log(menu='model', operate='Get the model creation form') | |||
def get(self, request: Request): | |||
provider = request.query_params.get('provider') | |||
model_type = request.query_params.get('model_type') |
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.
Here is a concise review of the code:
Potential Issues:
-
Repeated
tags
Parameter: The same tag'model'
is repeated for multiple actions across different classes and methods. This could be redundant and might not be necessary. -
Use of Swagger Annotations: While useful, some swagger annotations like
operation_id
can lead to repetitive documentation. Consider reusing them where appropriate. -
Redundant Logging Decorators: Similar decorators such as
@has_permissions
,@log
, etc., are used for each action without apparent need. You should ensure they align consistently if implemented differently elsewhere.
Optimizations and Recommendations:
-
Deduplicate Tags: If there's redundancy in tagging, consider merging similar functionalities into single tags or groups.
@tag(['common_operations']) def common_operation(request): pass
-
Reuse Decorators:
- Combine related permission checks (e.g., checking permissions using
permission_required
) within a decorator if possible. - Ensure that logging logic is centralized and reused wherever needed.
Example of combining permissions checks:
from functools import wraps def require_permission(permission_codes): def decorator(view_func): @wraps(view_func) def wrapper(*args, **kwargs): user = args[0].user all_permissions_granted = True for permission_code in permission_codes: if not user.has_perm(permission_code): return result.error(f"Permission `{permission_code}` denied") return view_func(*args, **kwargs) return wrapper return decorator @require_permission([PermissionConstants.MODEL_CREATE, PermissionConstants.MODEL_READ]) def combined_action(): # Action implementation pass
- Combine related permission checks (e.g., checking permissions using
By addressing these points, you can improve both readability and maintainability of your Django views.
@@ -254,7 +254,7 @@ class BatchGenerateRelated(APIView): | |||
@has_permissions( | |||
lambda r, k: Permission(group=Group.DATASET, operate=Operate.MANAGE, | |||
dynamic_tag=k.get('dataset_id'))) | |||
@log(menu=_('Knowledge Base/Documentation/Paragraph'), operate=_('Batch generate related')) | |||
@log(menu='Knowledge Base/Documentation/Paragraph', operate='Batch generate related') | |||
def put(self, request: Request, dataset_id: str, document_id: str): | |||
return result.success( | |||
ParagraphSerializers.BatchGenerateRelated(data={'dataset_id': dataset_id, 'document_id': document_id}) |
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.
There are no obvious irregularities or issues in the provided code snippet. However, there is one suggestion for optimization:
Code Optimization Suggestion
Ensure that the menu
parameter passed to the @log
decorator is consistently formatted. Both occurrences use 'Knowledge Base/Documentation/Paragraph'
, but it might be better to standardize them to reduce confusion. Here's how you can do it consistently across the decorators:
from django.utils.translation import gettext_lazy as _
class Paragraph(APIView):
@has_permissions(
lambda r, k: Permission(group=Group.DATASET, operate=Operate.USE,
dynamic_tag=k.get('dataset_id')))
@log(menu=_('Knowledge Base/Documentation/Paragraph'), operate=_('Paragraph list'))
def get(self, request: Request, dataset_id: str, document_id: str):
...
By updating all these lines to reflect this consistency:
class Paragraph(APIView):
@has_permissions(
lambda r, k: Permission(group=Group.DATASET, operate=Operate.USE,
dynamic_tag=k.get('dataset_id')))
@log(menu='Knowledge Base/Documentation/Paragraph', operate=_('Paragraph list'))
def get(self, request: Request, dataset_id: str, document_id: str):
...
# Repeat this format for other view classes similarly
This ensures that each log entry uses the same value throughout the application, improving readability and maintainability.
What this PR does / why we need it?
Summary of your change
Please indicate you've done the following: