[shaohuzhang1]

chore: update community identifiers and enhance serializers for user and tool APIs

[shaohuzhang1]

Here's a review of the code:

Issues

1. Missing import: OpenApiTypes is used (type=OpenApiTypes.STR) but not imported. Add:

   from drf_spectacular.utils import OpenApiParameter, OpenApiTypes

2. Inconsistent language in descriptions: Some descriptions are Chinese ("工作空间id"), others English ("Version Name"). Standardize for consistency.

3. Redundant pagination parameters: For DRF pagination, current_page/page_size are usually query parameters, but they're marked location="path". Verify this matches your URL patterns — likely should be location="query".

4. Inheritance redundancy: ToolWorkflowVersionPageApi inherits from ToolWorkflowVersionListApi and re-declares get_response returning ToolWorkflowVersionPageResult — that's fine, but get_parameters duplication could be reduced by extracting base path params into a helper since workspace_id/tool_id appear at multiple levels.

5. Patching whitespace-only changes (single → double quotes) mixed with functional changes makes the diff noisier; consider separating formatting changes into their own commit.

Optimization Suggestions

1. DRY up repeated parameters: Extract common params:

   def _path_param(name, desc):
       return OpenApiParameter(name=name, description=desc, type=OpenApiTypes.STR, location="path", required=True)
   
   BASE_PARAMS = [_path_param("workspace_id", "工作空间id"), _path_param("tool_id", "工具id")]

   Then reuse across all API classes.

2. Serializer result classes can be more concise:

   class ToolWorkflowVersionResult(ResultSerializer):
       def get_data(self):
           return ToolVersionModelSerializer()

   If ResultSerializer supports it, consider a factory or lambda-based approach to avoid three near-identical classes.

3. Type hint for OpenApiTypes.STR on IDs: If IDs are UUIDs, consider OpenApiTypes.UUID for better schema validation in Swagger docs.

Summary

The main blocking bug is the missing OpenApiTypes import — the code will raise NameError at runtime. Everything else is cleanliness/consistency.

[shaohuzhang1]

Overall the diff is mostly schema additions, but there are several things to check:

1. Most important: pagination parameters are declared as path parameters

   name="current_page",
   type=OpenApiTypes.INT,
   location="path",
   required=True,

   and the same for page_size. Unless your URL is actually something like:

   /.../{current_page}/{page_size}/
   

   these should almost certainly be query parameters:

   OpenApiParameter(
       name="current_page",
       description="当前页码",
       type=OpenApiTypes.INT,
       location="query",
       required=False,
       default=1,
   ),
   OpenApiParameter(
       name="page_size",
       description="每页大小",
       type=OpenApiTypes.INT,
       location="query",
       required=False,
       default=20,
   ),

   As written, this will generate incorrect OpenAPI documentation and potentially incorrect client SDKs.

2. name parameter has no explicit location

   OpenApiParameter(
       name="name",
       description="Version Name",
       type=OpenApiTypes.STR,
       required=False,
   ),

   It probably defaults to query, but it is better to be explicit:

   location="query",

   Also consider making the description language consistent with the rest of the file, e.g. "版本名称".

3. Verify ToolVersionModelSerializer is the correct serializer
   The new classes are named ToolWorkflowVersion*, but they use ToolVersionModelSerializer. If ToolVersion is the actual version model for workflow versions, this is fine. Otherwise, it may be a naming/model mismatch.

4. Check whether ToolWorkflowVersionOperateApi really always returns a single version
   If this class is reused for DELETE, the response should usually be 204 No Content, not a serialized version. If it is reused for create/update, you may also need request serializers. If the operations differ, consider splitting the schema classes or overriding get_response() / request schema per operation.

5. DefaultResultSerializer import may become unused
   Ensure it is still used elsewhere in the file. If not, remove it.

6. Minor style/import issue
   The import line is long:

   from common.result import DefaultResultSerializer, ResultSerializer, ResultPageSerializer

   Consider wrapping:

   from common.result import (
       DefaultResultSerializer,
       ResultPageSerializer,
       ResultSerializer,
   )

7. Minor optimization
   ToolWorkflowVersionListResult and ToolWorkflowVersionPageResult duplicate the same get_data() logic. This is acceptable, but if more result types are added, consider a small shared base class.

8. Unrelated quote-style changes
   The changes from 'path' to "path" in ToolWorkflowExportApi are harmless but add diff noise if the project has not standardized on double quotes.

The main correction is the pagination parameter location; the rest are consistency and maintainability improvements.

[shaohuzhang1]

Overall, this is a schema/documentation-only change and does not alter runtime behavior. I do not see a clear functional bug, but there are a few schema-correctness and maintainability points to verify.

Potential issues

1. Verify the return type of get_response()

   • The new code assumes TokensAggregationAPI.get_response() returns a serializer class or OpenApiResponse, because it is now nested under 200.
   • If get_response() already returns a responses mapping/dict, then {200: <dict>} is likely invalid or produces an incorrect schema.
   • If it returns a serializer class, this change is correct.

2. Make sure the 400 response body matches ResultStringSerializer

   • DRF’s default ValidationError response is usually {"field": ["error"]} or {"detail": "..."}, not a custom result envelope.
   • If the project has a custom exception handler that wraps errors in ResultStringSerializer, this is correct.
   • Otherwise, the documented 400 schema will not match the actual response.

3. The 400 description may be too specific

   • "Invalid or missing time range parameters" only covers time-range errors.
   • These endpoints may also return 400 for invalid pagination, filters, or other query parameters.
   • Consider a more general description such as "Invalid query parameters", or keep endpoint-specific wording only if the validation really is time-range-only.

4. Authentication/permission errors are not documented

   • These views use TokenAuth and has_permissions.
   • If you are adding explicit error documentation, consider whether 401 and/or 403 should also be documented.
   • This may already be handled globally by drf-spectacular, but it is worth confirming.

5. Duplication of the same 400 response definition

   • The same OpenApiResponse block is repeated six times.
   • This is not a bug, but it increases the chance of inconsistency later.
   • Consider a shared constant/helper, for example:

   COMMON_400_RESPONSE = OpenApiResponse(
       response=ResultStringSerializer,
       description=_("Invalid query parameters"),
   )
   
   def common_responses(success_response):
       return {
           200: success_response,
           400: COMMON_400_RESPONSE,
       }

   Then use:

   responses=common_responses(TokensAggregationAPI.get_response())

6. Import style is acceptable but slightly redundant

   • from common import result and from common.result import ResultStringSerializer are fine.
   • No action is required unless the project has stricter import grouping rules.

7. Pre-existing issue: operation ID with spaces

   • In ApplicationMonitoring, the existing operation_id="Dialogue-related statistical trends" contains spaces.
   • OpenAPI operation IDs should preferably be stable, machine-friendly identifiers, e.g. homepage_dialogue_statistical_trends.
   • This is not caused by the diff, but it is an existing schema irregularity.

Recommendation

The change is reasonable if:

• get_response() returns a serializer class or OpenApiResponse, not a response dict.
• The actual 400 payload is indeed ResultStringSerializer.
• The 400 description is accurate for all validation failures on these endpoints.

Otherwise, adjust the schema to match the real exception-handling and response structure.

[shaohuzhang1]

Code Review

Issues

1. Repeated code (DRY violation): The same OpenApiResponse(response=ResultStringSerializer, description=_("Invalid or missing time range parameters")) block is duplicated 6 times. Extract it once:

   INVALID_TIME_RANGE_RESPONSE = OpenApiResponse(
       response=ResultStringSerializer,
       description=_("Invalid or missing time range parameters"),
   )

   Then use responses={200: ..., 400: INVALID_TIME_RANGE_RESPONSE}.

2. operation_id inconsistency: ApplicationMonitoring uses "Dialogue-related statistical trends" — an English human-readable string with spaces/uppercase as an operation_id. This breaks the typical snake_case convention and may cause invalid OpenAPI operationId values. Use e.g. homepage_application_monitoring.

3. Import style: from common import result plus from common.result import ResultStringSerializer — if only ResultStringSerializer is needed and result module import is unused elsewhere in this diff, prefer a single consistent import.

Suggestions

4. Verify accuracy of the 400 doc: Make sure these endpoints actually return 400 (not 200-wrapped error bodies) for invalid time ranges; otherwise the schema will mislead clients. If errors are returned via your Result wrapper with HTTP 200, document that instead.

5. Consider reusable serializer-level responses: If most views follow the {200: ..., 400: ...} pattern, consider defining a small helper or mixin to build the responses dict, reducing boilerplate further.

6. Minor: keep # type: ignore comments justified; if tags lists are valid, they may be removable.

Overall: functionally correct, but refactor the duplication and fix the operation_id naming.