fix: Workspace permission table

[zhanweizhang7]

What this PR does / why we need it?

Summary of your change

Please indicate you've done the following:

• Made sure tests are passing and test coverage is added if needed.
• Made sure commit message follow the rule of Conventional Commits specification.
• Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

[f2c-ci-robot]

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.

Details

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.

[f2c-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[shaohuzhang1]

The code contains several areas that need attention:

1. Function Overload: The get_queryset method accepts two arguments (instance and is_x_pack_ee). This might cause confusion since you don't utilize one of these parameters inside the method.

2. SQL Injection Potential: Using os.path.join to include file paths can lead to SQL injection attacks, especially if file paths are directly derived from inputs like workspace_id.

3. Duplicate Code:

   • Both the list and paginate methods contain similar logic for querying permissions based on conditions (e.g., user roles).
   • There's also repeated code when filtering resources under folders.

4. Static Method for Role Check: The is_x_pack_ee static method can be extracted into a separate function or decorator.

5. Potential Performance Issues:

   • If multiple queries are made depending on certain condition flags (like checking for X-Pack EE), it may have performance implications.
   • Consider caching results where appropriate to reduce redundant computations.

Here’s how I recommend refactoring the code:

class ResourceUserPermissionSerializer(serializers.Serializer):
    RESOURCE_MODEL_MAP = {
        'TICKET': Ticket,
        # Add other resource types below
    }

    # ... rest of the class definition ...

    def get_has_manage_permission_resource_under_folders(self, current_user_id, folder_ids, workspace_id, workspace_manage=False):
        resource_model = self.RESOURCE_MODEL_MAP[type(self.instance)]
        
        if not isinstance(folder_ids, Iterable):
            raise serializers.ValidationError('Folder IDs must be an iterable')
        
        qs_kwargs = {}
        qs_filter_conditions = []

        if workspace_manage:
            qs_filter_conditions.append(Q(workspace_id=workspace_id))
            qs_kwargs['folder__in'] = folder_ids  # Assuming you want only items with matching folders

        user_role_mapping_qs = DatabaseModelManage.get_model("workspace_user_role_mapping").filter(
            workspace_id=workspace_id,
            user_id=current_user_id
        )

        # Constructing dynamic filters based on user roles
        for role_condition in user_role_mapping_qs.values('role'):
            role_condition_dict = {f"role_{role_condition}": "USER"}
            qs_filter_conditions.append(role_condition_dict)

        if qs_filter_conditions:
            filter_dict = {} if not qs_kwargs else {'**': qs_kwargs}
            qs_filter_dict = {'**': {**{k: v for d in qs_filter_conditions for k, v in d.items()}, **filter_dict}}
            
            result = native_search(resource_model.objects.all(), get_file_content(
                os.path.join(PROJECT_DIR, "apps", "system_manage",
                             'sql',
                             (
                                 'get_resource_user_permission_detail_ee.sql' if type(qs_filter_dict) == dict else
                                 'get_resource_user_permission_detail.sql'
                             )
                         )),
                        **qs_filter_dict)
        
        return result

Changes Made:

1. Removed duplicate variables by using dictionary unpacking and iterating through multiple lists together.
2. Simplified the conditional logic involving ws_user_role_mapping.
3. Extracted role-based filtering into a loop and dynamically construct the query dictionary for better readability.
4. Added error handling for non-iterable folder ids.

[shaohuzhang1]

The code you provided appears to have several improvements that can be suggested for better readability, maintainability, and potentially performance:

Improvements:

1. Field Annotations: You've introduced annotations like Cast from Django's database functions module. This is a good approach to ensure data types are consistent before querying.

2. Database Function Usage: The usage of Cast('id', TextField()) is appropriate because it explicitly converts an integer column to a string type, which is useful when working with certain database operations where different data types might be required.

3. Optimization Suggestions:

   • Batch Deletion: Consider using bulk deletion methods (Source.objects.filter(...).delete() without iterating over each object individually) if applicable, especially in production environments.
   • Transaction Management: Ensure that transaction boundaries are correctly set up around resource deletions to maintain atomicity and consistency. However, since transaction.atomic() is not applied here, it needs to be explicitly ensured within the critical sections.

4. Code Readability:

   • Add comments explaining complex logic steps, such as why resources need to be filtered under specific workspace permissions.
   • Use meaningful variable names to improve understanding of their purpose in the context of folder and resource management.

5. Error Handling: Adding try-except blocks around database queries or resource processing would help manage exceptions gracefully and prevent unexpected application errors.

6. Future-Proofing: Ensure that any new features or changes in future versions of Django do not break current functionality due to compatibility issues.

These suggestions aim to enhance the quality and robustness of the code while making it more efficient and easier to understand.