feat(server): add contentTypes filter to AssetFilter #1440
Conversation
WalkthroughThe changes update the asset filtering functionality across multiple layers of the system. A new field, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/API
participant R as Resolver/Controller
participant U as Asset Usecase
participant Repo as Asset Repository
participant SA as Storage Adapter (Memory/Mongo)
C->>R: Request assets (with contentTypes filter)
R->>U: Call FindFilesByIDs(ctx, ids, AssetFileFilter, operator)
U->>Repo: Invoke FindFilesByIDs(ctx, ids, AssetFileFilter)
Repo->>SA: Call FindByIDs(ctx, ids, assetFileFilter)
SA-->>Repo: Return filtered asset files
Repo-->>U: Return asset files map
U-->>R: Return asset files map
R-->>C: Respond with asset connection
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/internal/adapter/integration/asset.go(1 hunks)server/internal/adapter/integration/server.gen.go(3 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/pkg/integrationapi/types.gen.go(2 hunks)server/schemas/integration.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / lint
- GitHub Check: ci-server / test
🔇 Additional comments (11)
server/internal/usecase/interfaces/asset.go (1)
51-51: Implementation looks good for the ContentType filter.The addition of the
ContentType *[]stringfield to theAssetFilterstruct enables filtering assets by content type. The use of a pointer to a slice allows for both optional filtering and multiple content types, which aligns well with the PR objective of supporting comma-separated content types.server/pkg/integrationapi/types.gen.go (2)
618-619: Good addition of the ContentTypesParam type.The new type definition correctly establishes ContentTypesParam as a slice of strings, allowing for multiple content types to be specified.
970-971: Properly integrated ContentType field in AssetFilterParams.The AssetFilterParams struct has been correctly updated to include the new ContentType field with the appropriate JSON tag for serialization, maintaining alignment with other filter parameters while supporting the new filtering capability.
server/schemas/integration.yml (3)
1363-1363: Correctly referenced the contentTypesParam.The contentTypesParam reference is properly added to the AssetFilter operation parameters, enabling the API to accept content type filtering requests.
1385-1388: Good addition of contentTypes in the response schema.Adding the contentTypes array to the response schema will allow clients to receive information about available content types, enhancing the API's usability.
1811-1818:❓ Verification inconclusive
Good implementation of the contentTypesParam definition.
The parameter is correctly defined as a header parameter that accepts an array of strings, which supports the PR objective of allowing comma-separated content types like "contentTypes=json,geojson".
One verification to consider: ensure that the API implementation correctly processes this header parameter, converting the comma-separated string into an array of strings.
🏁 Script executed:
#!/bin/bash # Check if the handler properly processes content type header # Look for code that parses the contentType header rg -A 5 'contentType.*header' --glob '*.go'Length of output: 45
Action Required: Please Verify Header Parsing Logic in API Implementation
The integration schema in server/schemas/integration.yml (lines 1811–1818) correctly defines the
contentTypesParamas a header parameter accepting an array of strings. However, our automated searches did not locate explicit Go code that processes this header—specifically, converting a comma-separated string (e.g.,"contentTypes=json,geojson") into an array.
Review the API handler or middleware:
Ensure that the code retrieving thecontentTypeheader (e.g., viaHeader.Get("contentType")) includes logic to split the string on commas (such as usingstrings.Split) and correctly forms the array.Action:
If this conversion is handled by built-in middleware or elsewhere, please confirm its behavior with manual verification or dedicated tests. Otherwise, consider implementing the necessary parsing logic.server/internal/adapter/integration/server.gen.go (3)
1371-1386: Well-implemented contentType header parameter handlingThe implementation correctly handles the new contentType header parameter following the code's established pattern for parameter binding. The code properly validates the parameter, checks for exactly one value, and returns appropriate error messages for invalid inputs.
3467-3471: Good addition of ContentTypes field to the response structureThe
AssetFilter200JSONResponsestruct has been appropriately updated to include the newContentTypesfield as a pointer to a string slice with theomitemptyJSON tag, which aligns with the PR's goal to add content type filtering capability.
5323-5398: Swagger specification updated to reflect new parameterThe base64-encoded swagger specification has been updated to include the new contentType parameter, ensuring API documentation stays in sync with the implementation.
server/internal/adapter/integration/asset.go (2)
35-39: Good variable renaming for clarityRenaming
ftofilterimproves code readability and makes the purpose of this variable more explicit.
49-52: Proper implementation for fetching file metadataThe addition of this code to fetch file information for the found assets is well-implemented with appropriate error handling. This is essential for the new content type filtering functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/internal/adapter/integration/asset.go (1)
225-233: New utility function to parse content type inputThe
convertStringToSlicefunction properly handles the comma-separated string format for content types by splitting and trimming whitespace. This ensures consistent processing of user input.For future enhancement, consider adding validation to ensure the content types are in a valid format or match a known list of allowed content types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/internal/adapter/integration/asset.go(3 hunks)server/internal/adapter/integration/server.gen.go(3 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/pkg/integrationapi/types.gen.go(2 hunks)server/schemas/integration.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pkg/integrationapi/types.gen.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - reearth-cms
- GitHub Check: Header rules - reearth-cms
- GitHub Check: ci-server / test
- GitHub Check: ci-server / lint
- GitHub Check: Pages changed - reearth-cms
🔇 Additional comments (9)
server/internal/usecase/interfaces/asset.go (1)
47-52: LGTM! Addition of ContentType filter adds valuable functionality.The new
ContentTypefield in theAssetFilterstruct is consistent with the existing pattern (pointer to string) and properly enables filtering assets by content type as specified in the PR objectives.server/internal/adapter/integration/server.gen.go (3)
1371-1376: Implementation looks good for contentTypes query parameter binding.The new query parameter binding code follows the consistent pattern used for other parameters, with appropriate error handling for invalid formats.
3457-3461: ContentTypes field added correctly to the response structure.The ContentTypes field has been properly added to the AssetFilter200JSONResponse struct as a pointer to a string with appropriate JSON tags, maintaining consistency with the existing pattern.
1371-1376:✅ Verification successful
Verify implementation completeness in handler functions.
These changes to the generated code look good, but ensure that the AssetFilter handler function in
server/internal/adapter/integration/asset.gocorrectly utilizes the new ContentTypes parameter for filtering assets based on content types.Also applies to: 3457-3461
🏁 Script executed:
#!/bin/bash # Check how the contentTypes parameter is used in the handler implementation # Find the AssetFilter function implementation rg -A 20 "func \(s \*Server\) AssetFilter\(" server/internal/adapter/integration/asset.goLength of output: 817
AssetFilter Handler Verification Complete
I verified that the new
contentTypesquery parameter is correctly passed from the generated code into the AssetFilter handler. Inserver/internal/adapter/integration/asset.go, the handler now assignsrequest.Params.ContentTypesto the filter’sContentTypefield—ensuring it’s available for asset filtering. This verification applies similarly to the code at lines 3457-3461.No further changes are needed.
server/schemas/integration.yml (2)
1363-1363: API enhancement: Added contentTypes filtering parameterNew query parameter for filtering assets by content types has been added to the API specification. This allows clients to request assets based on their content types using a comma-separated format (e.g.,
contentTypes=json,geojson).
1385-1386: API enhancement: Response updated to include contentTypesThe response schema has been updated to include a
contentTypesfield, which will return the content types that were used for filtering. This provides useful context to API consumers about which filters were applied.server/internal/adapter/integration/asset.go (3)
36-41: Variable renamed for improved clarityThe variable
fhas been renamed to more descriptivefilterand now includes a newContentTypefield. This improves readability and supports the new content type filtering functionality.
51-54: Added file lookup for content type filteringNew code retrieves the associated files for assets to enable content type filtering. This is necessary for the new filtering feature as content types are stored in the file objects.
76-82: Improved error handling for empty result setsGood addition of checks for empty result sets and adjustment of total count when filtering reduces the number of results. This ensures accurate pagination information is returned to clients.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/internal/adapter/integration/asset.go (2)
51-57: Consider adding a comment explaining fileMap retrievalThe new code correctly retrieves files for content type filtering, but a brief comment would improve code maintainability.
var fileMap map[asset.ID]*asset.File if filter.ContentType != nil { + // Retrieve files to access their content types for filtering fileMap, err = uc.Asset.FindFilesByIDs(ctx, assets.IDs(), op) if err != nil { return AssetFilter400Response{}, err } }
228-236: Add documentation to the helper functionThe new helper function is well implemented but lacks documentation. Consider adding a brief comment explaining its purpose and parameter/return values.
+// convertStringToSlice splits a comma-separated string into a trimmed slice of strings. +// Used for parsing content type filters provided in the API request. func convertStringToSlice(input string) []string { splitStrings := strings.Split(input, ",") for i := range splitStrings { splitStrings[i] = strings.TrimSpace(splitStrings[i]) } return splitStrings }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/adapter/integration/asset.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / test
- GitHub Check: ci-server / lint
🔇 Additional comments (3)
server/internal/adapter/integration/asset.go (3)
36-41: Great variable renaming and ContentType field additionRenaming the variable from
ftofilterimproves code readability. The addition of theContentTypefield properly implements the content type filtering functionality described in the PR objectives.
63-69: Robust content type filtering implementationThe implementation correctly handles file existence check before accessing content type, addressing a potential null pointer dereference. The use of
convertStringToSliceto parse comma-separated content types aligns well with the PR objective.
79-85: Good handling of empty results and pagination adjustmentThe new code properly handles the case when no assets match the filter criteria by returning a 404 response. Additionally, adjusting the total count when some assets are filtered out ensures pagination information remains accurate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/internal/adapter/integration/asset.go (3)
6-7: Remove unnecessary fmt import or replace print statements with proper loggingWhile the strings import is needed for content type handling, the fmt import is only used for debug print statements. Consider removing it or replacing print statements with proper structured logging using your project's logging system.
47-47: Remove debug print statementDebug print statements should not be left in production code. Replace with proper logging or remove entirely.
- fmt.Println("debug err not found FindByProject: ", err) + // Use proper logging system instead
81-84: Remove debug print and use consistent error handlingDebug print statement should be removed, and the error handling is inconsistent - the error variable is used in the print but
rerror.ErrNotFoundis returned.if len(itemList) == 0 { - fmt.Println("debug err not found item list == 0: ", err) return AssetFilter404Response{}, rerror.ErrNotFound }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/adapter/integration/asset.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / test
🔇 Additional comments (6)
server/internal/adapter/integration/asset.go (6)
36-42: LGTM: Good variable renaming and parameter additionRenaming from
ftofilterimproves code readability, and the addition ofContentTypeparameter properly connects the API request parameter to the filter structure.
44-44: LGTM: Updated function call with modified filter structureThe function call correctly uses the updated filter containing the contentTypes parameter.
53-59: LGTM: Efficient file data loading for content type filteringGood optimization by conditionally loading file data only when content type filtering is requested, avoiding unnecessary database queries when not needed.
61-76: LGTM: Content type filtering implementationThe content type filtering logic is properly implemented. It correctly:
- Parses the content type parameter into a slice
- Checks if the file exists (null check)
- Only filters by content type when specified
- Uses the new file information in asset construction
This addresses the previous review comments about null pointer checks.
86-88: LGTM: Total count adjustment for filtered resultsGood addition to adjust the total count when content filtering reduces the number of returned items below the expected total. This ensures pagination information remains accurate.
231-239: LGTM: Clean helper function for content type parsingThe
convertStringToSlicefunction effectively handles splitting comma-separated content types and properly trims whitespace. Consider adding a test for this utility function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/internal/adapter/integration/asset.go (1)
230-238: Well-implemented helper function for string conversion.The
convertStringToSlicefunction:
- Correctly splits comma-separated values
- Trims whitespace from each value, which improves usability by handling input like "json, geojson"
Consider adding a test case for this function to ensure it handles edge cases properly (empty strings, multiple commas, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/adapter/integration/asset.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / test
🔇 Additional comments (5)
server/internal/adapter/integration/asset.go (5)
6-6: Import added for contentType filtering functionality.The strings package addition is necessary for implementing the new contentTypes filter feature.
36-41: Good variable rename and contentType filter implementation.Renaming
fto the more descriptivefilterimproves code readability. The newContentTypefield correctly integrates with the request parameters, enabling the filtering by content types specified in the PR objective.
51-59: Well-structured content type filter implementation.The implementation effectively:
- Sets a flag to track if content type filtering is active
- Only fetches file data when necessary (when content type filtering is requested)
- Properly handles errors from the file retrieval operation
This approach is efficient as it avoids unnecessary database queries when the content type filter isn't being used.
61-76: Appropriate null check in content type filtering.The implementation correctly handles potential nil values by checking if the file exists before attempting to access its content type. This addresses the issue mentioned in the previous review comment and prevents null pointer exceptions.
The function returns early when content types don't match, which is efficient.
81-87: Proper handling of empty results and total count adjustment.The implementation correctly:
- Returns a 404 response when content type filtering results in no items
- Adjusts the total count to match the actual number of items after filtering
This ensures the pagination metadata remains accurate after content type filtering is applied.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/internal/adapter/integration/asset.go (1)
223-231: Consider exporting the utility function to a common package.The
convertStringToSlicefunction appears to be duplicated in multiple files (as seen in the asset_file.go changes). Consider moving it to a utility package for reuse.-func convertStringToSlice(input string) []string { - splitStrings := strings.Split(input, ",") - - for i := range splitStrings { - splitStrings[i] = strings.TrimSpace(splitStrings[i]) - } - - return splitStrings -}Create a new utility function in a common package:
// In a util or common package func ConvertCommaSeparatedToSlice(input string) []string { if input == "" { return nil } splitStrings := strings.Split(input, ",") for i := range splitStrings { splitStrings[i] = strings.TrimSpace(splitStrings[i]) } return splitStrings }🧰 Tools
🪛 GitHub Check: ci-server / lint
[failure] 223-223:
funcconvertStringToSliceis unused (unused)server/internal/infrastructure/memory/asset_file.go (1)
54-60: Duplicate code for parsing content types.The content type parsing logic is duplicated between this file and the integration file. This could lead to inconsistencies if one implementation changes but not the other.
Consider extracting this functionality to a common utility function:
-var contentTypes []string -if assetFileFilter.ContentTypes != nil { - contentTypes = strings.Split(*assetFileFilter.ContentTypes, ",") - for i, ct := range contentTypes { - contentTypes[i] = strings.TrimSpace(ct) - } -} +var contentTypes []string +if assetFileFilter.ContentTypes != nil { + contentTypes = util.ConvertCommaSeparatedToSlice(*assetFileFilter.ContentTypes) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/internal/adapter/integration/asset.go(3 hunks)server/internal/adapter/publicapi/asset.go(1 hunks)server/internal/infrastructure/memory/asset_file.go(2 hunks)server/internal/infrastructure/mongo/asset_file.go(3 hunks)server/internal/usecase/interactor/asset.go(1 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/usecase/interfaces/asset.go
🧰 Additional context used
🪛 GitHub Check: ci-server / lint
server/internal/adapter/integration/asset.go
[failure] 223-223:
func convertStringToSlice is unused (unused)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - reearth-cms
- GitHub Check: Header rules - reearth-cms
- GitHub Check: Pages changed - reearth-cms
- GitHub Check: ci-server / test
🔇 Additional comments (22)
server/internal/adapter/publicapi/asset.go (1)
65-65: API implementation updated to support content type filteringThe API now correctly passes an empty
AssetFileFilterto theFindFilesByIDsfunction instead ofnil, accommodating the new content types filter feature.server/internal/usecase/repo/asset.go (2)
17-19: Added new AssetFileFilter struct to support content type filteringThe new struct provides a clean way to pass content type filtering options through the repository layer.
32-32: Updated FindByIDs method signature to support content type filteringThe
AssetFileinterface now correctly accepts the newAssetFileFilterparameter, allowing for filtering assets by content type.server/internal/usecase/interactor/asset.go (2)
71-71: Updated FindFilesByIDs function signature to include the content type filterMethod signature updated to accept the new
assetFileFilterparameter, aligning with the PR objectives of supporting content type filtering.
77-79: Correctly passing content type filter to repository layerThe implementation properly passes the
ContentTypesfield from the received filter to the repository layer.server/internal/infrastructure/mongo/asset_file.go (2)
78-78: Updated FindByIDs method signature to support content type filteringMethod signature properly updated to include the new
assetFileFilterparameter, implementing the content type filtering feature.
187-195: Good utility function for parsing comma-separated content typesThe
convertStringToSlicefunction correctly handles the parsing of comma-separated content types and trims whitespace from each entry.server/internal/adapter/integration/asset.go (8)
6-6: Good addition of the strings import for content type processing.The strings package is necessary for the new content type parsing functionality.
35-41: Improved naming and added content type filter support.Renaming
ftoassetFilterenhances code readability, and the addition ofContentTypesparameter enables content type filtering capability as requested in the PR.
51-60: Well-implemented content type filtering setup.Good approach to conditionally fetch files only when content type filtering is active. This optimization prevents unnecessary database queries when the filter isn't being used.
62-71: Proper implementation of asset filtering with content type support.The code correctly uses
util.TryFilterMapto filter assets based on content types. The boolean return value is a good approach to indicate whether an asset should be included in the results.
68-70: Effective null checks for file existence.The check for both existence (
!ok) and null value (assetFile == nil) provides robust error handling and prevents null pointer dereferences.
73-74: Good update to asset creation with file parameter.Properly passing the file parameter to
NewAssetensures content type information is included in the response.
80-86: Improved count calculation for filtered results.Using
minto calculate the total count ensures consistent pagination when filtering is applied.
223-231:✅ Verification successful
Function flagged as unused by static analysis.
The
convertStringToSlicefunction is flagged as unused by the linter. While it's likely used in other files via imports, it's not directly used in this file.
🏁 Script executed:
#!/bin/bash # Check if this function is used in the codebase rg "convertStringToSlice" --type go -A 2 -B 2Length of output: 1227
Static Analysis False Positive – No Action Required
Our investigation confirms that although the
convertStringToSlicefunction is defined inserver/internal/adapter/integration/asset.goand flagged as unused in that file, it is indeed called fromserver/internal/infrastructure/mongo/asset_file.go(e.g.,contentTypes := convertStringToSlice(*assetFileFilter.ContentTypes)). The linter appears to be flagging it only within its defining file, while its cross-package usage is valid.🧰 Tools
🪛 GitHub Check: ci-server / lint
[failure] 223-223:
funcconvertStringToSliceis unused (unused)server/internal/infrastructure/memory/asset_file.go (7)
5-5: Good addition of the strings import for content type processing.The strings package is necessary for parsing comma-separated content types.
7-7: Appropriate imports for the new functionality.Added necessary imports for the repo package and the lo library, which are used for the content type filtering.
Also applies to: 12-12
47-47: Updated method signature to support content type filtering.Good update to include the
assetFileFilterparameter, allowing content type filtering in the memory implementation.
54-60: Well-implemented content type parsing.The code correctly handles parsing of comma-separated content types and trims whitespace from each value.
71-74: Proper content type filtering implementation.Good implementation of content type filtering that skips files with non-matching content types.
80-89: Well-implemented filtered file set creation.The implementation correctly filters the files based on content types and sets only matching files on the asset.
72-72:❓ Verification inconclusive
Verify behavior when contentTypes array is empty.
The current implementation checks
len(contentTypes) > 0before applying filtering, which is good. However, ensure that when no content types are specified, all files are returned.Also applies to: 84-84
🏁 Script executed:
#!/bin/bash # Check if there are any tests for content type filtering rg -A 10 -B 10 "Test.*ContentType" --type goLength of output: 4942
Action: Confirm behavior with empty contentTypes filtering
The current logic in
server/internal/infrastructure/memory/asset_file.go(lines 72 and 84) only applies filtering whencontentTypesisn’t empty (i.e. whenlen(contentTypes) > 0). While this approach is correct, please ensure that if no content types are specified, the code returns all files. I couldn’t find any tests explicitly verifying this “empty filter” scenario. Consider adding or checking for a dedicated test to confirm that an emptycontentTypesarray indeed bypasses filtering and includes every file.
server/schemas/integration.yml
Outdated
| @@ -1360,6 +1360,7 @@ paths: | |||
| - $ref: '#/components/parameters/pageParam' | |||
| - $ref: '#/components/parameters/perPageParam' | |||
| - $ref: '#/components/parameters/keywordParam' | |||
| - $ref: '#/components/parameters/contentTypesParam' | |||
There was a problem hiding this comment.
the implementation should be in gql
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1733-1738: Case inconsistency in enum constant naming.The enum constant for GEOJSON is defined as
ContentTypesEnumGeojson(title case) while in the GraphQL schema it's defined asGEOJSON(all uppercase). This inconsistency might lead to issues when unmarshaling values.Consider using consistent casing in enum constant naming:
- ContentTypesEnumGeojson ContentTypesEnum = "GEOJSON" + ContentTypesEnumGEOJSON ContentTypesEnum = "GEOJSON"server/internal/adapter/gql/generated.go (1)
5230-5233: Declared ContentTypesEnum and updated assets query.
DefiningJSONandGEOJSONas part ofContentTypesEnumis clear. Consider adding doc comments in the schema for clarity.Also applies to: 5242-5242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/internal/adapter/gql/generated.go(23 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(1 hunks)server/internal/adapter/gql/resolver_asset.go(1 hunks)server/schemas/asset.graphql(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
server/internal/adapter/gql/resolver_asset.go (1)
server/internal/adapter/gql/gqlmodel/models_gen.go (4) (4)
AssetSort(125-128)Pagination(747-753)ContentTypesEnum(1733-1733)AssetConnection(99-104)
server/internal/adapter/gql/generated.go (4)
server/internal/adapter/gql/gqlmodel/models_gen.go (17) (17)
ContentTypesEnum(1733-1733)e(1579-1585)e(1587-1589)e(1591-1602)e(1604-1606)e(1622-1628)e(1630-1632)e(1634-1645)e(1647-1649)e(1663-1669)e(1671-1673)e(1675-1686)e(1688-1690)e(1704-1710)e(1712-1714)e(1716-1727)e(1729-1731)server/internal/adapter/gql/gqlmodel/convert_schema_test.go (1) (1)
args(132-135)server/internal/adapter/gql/gqlmodel/convert_item.go (2) (2)
res(44-44)v(54-54)server/internal/adapter/gql/gqlmodel/convert_schema.go (10) (10)
v(210-210)v(223-223)v(236-236)v(249-249)v(262-262)v(277-277)v(299-299)v(314-314)v(330-330)v(353-353)
🔇 Additional comments (25)
server/schemas/asset.graphql (2)
144-147: Good addition of ContentTypesEnum for asset filtering.The new enum with JSON and GEOJSON values provides a clear, type-safe way to filter assets by content type.
156-156:❓ Verification inconclusive
The assets query update looks good but verify integration with loaders.
The GraphQL query signature has been properly updated to include the new contentTypes filter parameter.
🏁 Script executed:
#!/bin/bash # Verify the ContentTypes filter is properly passed through the loader chain rg -A 5 "FindByProject.*content" --type goLength of output: 44
Integration with loaders requires manual confirmation
The GraphQL schema update in
server/schemas/asset.graphqlfor adding the newcontentTypesfilter appears correct. However, our initial automated search did not return evidence that the updated parameter is passed through the loader chain (e.g., within functions matchingFindByProject.*contentTypes). Please manually verify that the loaders handling asset queries correctly integrate this new filter.
- File under review:
server/schemas/asset.graphql- Action needed: Confirm that the loader chain (likely involving functions related to
FindByProject) properly accepts and forwards thecontentTypesparameter.server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1740-1772: The ContentTypesEnum implementation is complete and follows the project pattern.The implementation includes all necessary methods for validation, string conversion, and GraphQL marshaling/unmarshaling. It correctly follows the pattern used for other enum types in the codebase.
server/internal/adapter/gql/generated.go (22)
552-552: Added contentTypes parameter to complexity calculation.
No concerns; extending the function signature to includecontentTypesis consistent with your approach for other query parameters.
1014-1014: Introduced contentTypes parameter in QueryResolver.
The updated signature aligns well with your new filter requirement. No issues spotted.
3374-3374: Propagated contentTypes argument into complexity function call.
The type assertion appears safe due to prior handling in argument parsing. Looks good.
8684-8688: Mapped contentTypes argument for assets query.
The argument extraction logic is consistent with existing patterns. No concerns.
8763-8779: Handled optional contentTypes argument.
Returning a default zero value whencontentTypesis absent is a good approach for optional fields.
24318-24318: Added contentTypes in resolver call.
This usage matches the new parameter signature. No issues noted.
25401-25402: GraphQL introspection addition (specifiedByURL).
Auto-generated introspection code—no modifications required.
33741-33770: GraphQL introspection addition (isRepeatable).
Auto-generated introspection code—no modifications required.
34267-34268: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
34505-34506: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
34740-34741: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
34808-34809: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
34873-34874: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
34938-34939: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
35004-35005: GraphQL introspection (isRepeatable).
Auto-generated introspection code—no modifications required.
35143-35169: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
35292-35293: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
35357-35358: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
35539-35540: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
48990-48991: Directive introspection (isRepeatable).
Auto-generated introspection code—no modifications required.
49264-49265: GraphQL introspection (specifiedByURL).
Auto-generated introspection code—no modifications required.
52664-52739: Unmarshal/marshal logic for contentTypes.
This is standard GraphQL generated code for handling enum arrays. Implementation looks correct.
| func (r *queryResolver) Assets(ctx context.Context, projectID gqlmodel.ID, keyword *string, sort *gqlmodel.AssetSort, pagination *gqlmodel.Pagination, contentTypes []*gqlmodel.ContentTypesEnum) (*gqlmodel.AssetConnection, error) { | ||
| return loaders(ctx).Asset.FindByProject(ctx, projectID, keyword, sort, pagination) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Function signature updated but contentTypes parameter is not used.
While the function signature has been updated to include the contentTypes parameter, it doesn't appear to be passed to the FindByProject method on line 175. This suggests the implementation might not be complete.
🏁 Script executed:
#!/bin/bash
# Check if the FindByProject method in the loader has been updated to accept contentTypes
rg -A 5 "FindByProject.*\(" --type goLength of output: 36613
Critical: Unused contentTypes Parameter in Assets Resolver
- The resolver's function signature now includes the
contentTypesparameter, but it isn’t passed to the underlyingFindByProjectcall. - The loader’s
FindByProjectmethod (and its associated use-case/repo functions) does not currently accept or handle acontentTypesargument. - Please either update the loader and the deeper application layers to incorporate this parameter (if filtering by content types is intended) or remove it from the resolver signature to avoid ambiguity.
Overview
add contentTypes filter to AssetFilter with comma-separated format
example: conteTypes=json,geojson
What I've done
What I haven't done
How I tested
test if the filter filters the API result based on contentTypes
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit