Skip to content

Conversation

shaohuzhang1
Copy link
Contributor

fix: Application log permission error

Copy link

f2c-ci-robot bot commented Sep 23, 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 Sep 23, 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

@shaohuzhang1 shaohuzhang1 merged commit b9dcfaf into v1 Sep 23, 2025
3 of 4 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v1@fix_application_log branch September 23, 2025 06:53
compare=CompareConstants.AND)
def get(self, request: Request, application_id: str, work_flow_version_id: str):
return result.success(
ApplicationVersionSerializer.Operate(
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 is mostly correct but can be improved for clarity and robustness:

Issues Identified:

  1. Redundant compare=CompareConstants.AND:

    • The second @has_permissions decorator contains an unnecessary compare=CompareConstants.AND.
  2. Lack of Consistency in Method Naming:

    • The method names should match the endpoint they are associated with (e.g., get) to maintain consistency.

Optimization Suggestions:

  1. Parameter Validation:

    • Add input validation to ensure that application_id, current_page, and page_size are valid integers before processing.
  2. Use a More Concise Permission Check Setup:

    • Combine similar permissions checks into a single instance or use context managers for better readability and efficiency.

Here's the revised version:

from rest_framework import APIView
from rest_framework.request import Request
from .models import ApplicationVersionApi, OperateAPI, Group, OperationalType, \
                       Permission, CompareConstants, has_permissions

class Page(APIView):
    @has_permissions(PermissionConstants.APPLICATION_READ)
    def get(self, request: Request, application_id: str, current_page: int, page_size: int):
        # Validate inputs
        if not application_id.isdigit() or not page_size.isdigit():
            return result.failure("Invalid parameters")

        try:
            response_data = ApplicationVersionSerializer.Query(
                filter_params=(
                    {'application_id': application_id},
                    *result.get_query_params(request.query_params),
                ),
                pagination_info={'limit': page_size, 'offset': (int(page_size) * (int(current_page) - 1))}
            )
        except Exception as e:
            return result.exception(e)

        return result.success(response_data)


class Operate(APIView):
    @has_permissions(PermissionConstants.APPLICATION_READ)
    def get(self, request: Request, application_id: str, workflow_version_id: str):
        # Validate inputs
        if not application_id.isdigit() or not workflow_version_id.isdigit():
            return result.failure("Invalid parameters")

        try:
            response_data = ApplicationVersionSerializer.Operate(
                filter_params={
                    'workflow_version_id': workflow_version_id
                }
            )
        except Exception as e:
            return result.exception(e)

        return result.success(response_data)

Key Improvements:

  • Consistent Method Names: Ensures that each method name corresponds to its endpoint.
  • Input Validation: Added basic validation to check if application_id and page_size are digits.
  • Error Handling: Wrapped exception handling around operations to provide more meaningful error messages.
  • Reduced Redundancy: Eliminated redundant compare=CompareConstants.AND.
  • Readability: Improved variable naming and structure for better readability.

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.

1 participant