feat(server): asset gzip upload #1363
Conversation
|
Warning Rate limit exceeded@rot1024 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces comprehensive support for content encoding in asset management across the ReEarth-CMS platform. The changes span multiple components of the system, including GraphQL schemas, API interfaces, file handling, and upload mechanisms. The primary focus is on adding a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Nitpick comments (8)
server/internal/infrastructure/gcp/file.go (1)
92-92: Simplify nestedTrimPrefixcalls for clarityConsider using
strings.TrimLeftto remove any leading slashes after trimming the prefixp. This improves code readability.Apply this diff to enhance the code:
- Name: strings.TrimPrefix(strings.TrimPrefix(attrs.Name, p), "/"), + Name: strings.TrimLeft(strings.TrimPrefix(attrs.Name, p), "/"),server/internal/adapter/gql/generated.go (1)
5157-5158: Improve documentation for content encoding.The comment "specify 'gzip' if you want to uplaod a gzip file" contains a typo ("uplaod") and could be more detailed about:
- Supported encoding types
- Impact on file serving
- Client-side requirements
- # specify "gzip" if you want to uplaod a gzip file so that the server can serve it with the correct content-encoding. + # Specify "gzip" if you want to upload a gzip-compressed file. The server will preserve the encoding and serve + # the file with the appropriate Content-Encoding header, allowing clients to handle decompression automatically.Also applies to: 5169-5170
server/pkg/asset/file_builder.go (1)
66-71: LGTM! Good optimization for content type detection.The method intelligently avoids unnecessary content type detection when it's already set. Consider adding a comment explaining this optimization for better maintainability.
func (b *FileBuilder) GuessContentTypeIfEmpty() *FileBuilder { + // Only detect content type if not already set, avoiding unnecessary mime type lookups if b.f.contentType == "" { b.detectContentType = true } return b }server/internal/usecase/gateway/file.go (1)
24-24: Enhance error message for clarity.Consider making the error message more specific about supported content encodings.
- ErrUnsupportedContentEncoding error = rerror.NewE(i18n.T("unsupported content encoding")) + ErrUnsupportedContentEncoding error = rerror.NewE(i18n.T("unsupported content encoding: only gzip and identity are supported"))server/internal/usecase/interfaces/asset.go (2)
34-36: Consider adding field documentation for ContentEncoding.The new
ContentEncodingfield would benefit from documentation explaining its purpose and accepted values (e.g., "gzip").
53-58: Consider grouping related fields together.The content-related fields (
ContentType,ContentLength,ContentEncoding) should be grouped together for better code organization.type AssetUpload struct { URL string UUID string + ContentType string + ContentLength int64 + ContentEncoding string - ContentType string - ContentLength int64 - ContentEncoding string Next string }server/pkg/asset/file.go (2)
12-18: Consider adding field documentation and validation.The
contentEncodingfield would benefit from documentation specifying accepted values. Also, consider adding validation for supported content encodings.
109-114: Consider maintaining field order consistency.The
contentEncodingfield in theClone()method should maintain the same order as in the struct definition for better readability.return &File{ name: f.name, size: f.size, contentType: f.contentType, + contentEncoding: f.contentEncoding, path: f.path, children: children, - contentEncoding: f.contentEncoding, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
server/e2e/integration_asset_project_test.go(0 hunks)server/e2e/integration_asset_test.go(0 hunks)server/e2e/integration_item_import_test.go(4 hunks)server/internal/adapter/gql/generated.go(29 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/internal/adapter/gql/loader_integration.go(0 hunks)server/internal/adapter/gql/resolver_asset.go(2 hunks)server/internal/adapter/integration/asset.go(4 hunks)server/internal/adapter/integration/convert.go(3 hunks)server/internal/adapter/integration/item.go(0 hunks)server/internal/adapter/integration/server.gen.go(1 hunks)server/internal/infrastructure/fs/file.go(1 hunks)server/internal/infrastructure/gcp/file.go(5 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/usecase/gateway/file.go(1 hunks)server/internal/usecase/interactor/asset.go(7 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/pkg/asset/asset.go(3 hunks)server/pkg/asset/file.go(4 hunks)server/pkg/asset/file_builder.go(2 hunks)server/pkg/file/file.go(5 hunks)server/pkg/file/file_test.go(4 hunks)server/pkg/integrationapi/types.gen.go(1 hunks)server/schemas/asset.graphql(5 hunks)server/schemas/integration.yml(4 hunks)
💤 Files with no reviewable changes (4)
- server/internal/adapter/gql/loader_integration.go
- server/internal/adapter/integration/item.go
- server/e2e/integration_asset_test.go
- server/e2e/integration_asset_project_test.go
✅ Files skipped from review due to trivial changes (1)
- server/internal/adapter/integration/server.gen.go
👮 Files not reviewed due to content moderation or server errors (4)
- server/e2e/integration_item_import_test.go
- server/internal/usecase/interactor/asset.go
- server/pkg/integrationapi/types.gen.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
🔇 Additional comments (27)
server/internal/adapter/gql/generated.go (5)
80-80: LGTM: Consistent field additions across related structs.The
ContentEncodingfield has been correctly added to theAsset,AssetFile, andCreateAssetUploadPayloadstructs with consistent types and positioning.Also applies to: 111-116, 170-175
1108-1114: LGTM: Proper complexity handling for the new field.The complexity calculations for
ContentEncodingare implemented consistently across all relevant types, with proper nil checks before invocation.Also applies to: 1262-1268, 1486-1492
10541-10580: LGTM: Properly implemented field resolvers.The field resolvers for
ContentEncodingare implemented correctly with proper error handling and null checks.Also applies to: 11328-11367, 12850-12889
5109-5109: Verify the content encoding validation.The schema allows any string value for
contentEncoding, but the comment specifically mentions "gzip". Consider adding an enum or validation to restrict the allowed values.Also applies to: 5127-5127, 5157-5158, 5169-5170, 5218-5218
Line range hint
36498-36546: Verify the order of fields in input unmarshaling.The field order in
fieldsInOrderarrays should match the schema definition to maintain consistency. Also, ensure that all required fields are properly validated.Also applies to: 36560-36594
server/pkg/asset/file_builder.go (1)
32-35: LGTM! Clean implementation of content encoding setter.The method follows the builder pattern consistently with other methods in the file.
server/pkg/file/file.go (1)
20-24: LGTM! File struct updated with content encoding support.The ContentEncoding field is properly added to track the encoding of file content.
server/pkg/asset/asset.go (1)
79-85: LGTM! Clean getter implementations.The getters are properly implemented and the code organization is improved with clear section comments.
server/internal/usecase/gateway/file.go (1)
28-32: LGTM! Consistent content encoding support across types.The ContentEncoding field is properly added to all relevant structs, maintaining consistency throughout the codebase.
Also applies to: 36-40, 44-48
server/pkg/asset/file.go (1)
48-53: LGTM! Well-implemented getter method.The
ContentEncoding()method follows the established pattern with proper nil checks.server/internal/adapter/gql/gqlmodel/convert_asset.go (1)
136-141: LGTM! Good use of EmptyableToPtr.The implementation correctly handles empty values for both
ContentTypeandContentEncodingusinglo.EmptyableToPtr.server/internal/infrastructure/fs/file.go (1)
99-101: Consider improving error message clarity.While the check for unsupported content encoding is good, consider enhancing the error message to specify which content encodings are supported (if any) or why the encoding was rejected.
Also, verify if this change aligns with the PR objective of implementing gzip upload support, as it currently rejects all content encodings.
✅ Verification successful
Consider improving error message clarity and documenting storage implementation differences
The check for unsupported content encoding is correct as this appears to be the local filesystem implementation. However, the error message should be enhanced to:
- Clarify that content encoding is not supported in local filesystem storage
- Indicate that gzip support is available when using GCP storage
This implementation aligns with the PR objective as gzip upload support is properly implemented in the GCP storage handler (
server/internal/infrastructure/gcp/file.go).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other implementations handling gzip content rg -l "ContentEncoding.*gzip" --type goLength of output: 128
Script:
#!/bin/bash # Check the implementation details in the found files and look for related interfaces echo "=== GCP File Implementation ===" rg -A 10 -B 10 "ContentEncoding.*gzip" server/internal/infrastructure/gcp/file.go echo -e "\n=== Asset Interactor Implementation ===" rg -A 10 -B 10 "ContentEncoding.*gzip" server/internal/usecase/interactor/asset.go echo -e "\n=== Looking for related interfaces ===" ast-grep --pattern 'type $_ interface { $$$ ContentEncoding $$$ }'Length of output: 2441
server/internal/infrastructure/mongo/mongodoc/asset.go (3)
36-41: LGTM! ContentEncoding field properly added to AssetFileDocument struct.The addition of the ContentEncoding field to the AssetFileDocument struct is well-placed and follows the existing field pattern.
146-151: LGTM! Proper initialization of ContentEncoding in NewFile method.The ContentEncoding field is correctly initialized from the asset.File instance.
172-174: LGTM! ContentEncoding properly included in Model conversion.The ContentEncoding field is correctly handled in the Model method when converting back to asset.File.
server/internal/adapter/integration/convert.go (1)
49-49: LGTM! Clean refactoring of unused parameters.Replacing unused named parameters with underscores improves code clarity by explicitly indicating which parameters are not used in the functions.
Also applies to: 201-201, 230-230
server/internal/adapter/integration/asset.go (2)
92-93: Address TODO comments regarding HTTP headers.The code includes TODO comments about checking HTTP headers for ContentType and ContentEncoding. This validation is important for security and data integrity.
Would you like me to help create an issue to track the implementation of HTTP header validation for ContentType and ContentEncoding?
174-178: LGTM! Proper handling of ContentEncoding in AssetUploadCreate.The ContentEncoding field is correctly included in the CreateAssetUploadParam structure, maintaining consistency with the asset creation flow.
server/internal/adapter/gql/resolver_asset.go (2)
61-64: LGTM! Improved error handling with context in file.FromURL.The addition of context to file.FromURL allows for better request handling and cancellation support.
138-142: LGTM! Consistent handling of ContentEncoding in GraphQL resolver.The CreateAssetUpload resolver properly includes ContentEncoding in the upload parameters, maintaining consistency with the REST API implementation.
server/pkg/file/file_test.go (1)
38-38: Verify context propagation in HTTP requests.The test should verify that the context is properly propagated to the HTTP requests.
Run this script to check if the context is used correctly in the HTTP client:
Also applies to: 51-51
✅ Verification successful
Context is properly propagated in HTTP requests
The implementation correctly propagates context using
http.NewRequestWithContext(). While the test could be enhanced by verifying context properties in the mock responder, the current implementation ensures proper context handling.Potential improvements for the test:
- Add context verification in the mock responder
- Test with context timeouts/deadlines
- Verify context cancellation behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for context usage in HTTP client creation and requests ast-grep --pattern 'http.NewRequestWithContext($ctx, $_, $_)'Length of output: 63
Script:
#!/bin/bash # Find FromURL implementation and context usage echo "=== FromURL implementation ===" ast-grep --pattern 'func FromURL($$$) {$$$}' echo -e "\n=== HTTP-related code ===" rg "http\." -A 5 echo -e "\n=== Context usage ===" rg "context\." -A 3Length of output: 86288
Script:
#!/bin/bash # Find test implementation and mock setup echo "=== Test implementation ===" rg -A 10 "func TestFromURL" server/pkg/file/file_test.go echo -e "\n=== Mock setup ===" rg -A 5 "httpmock" server/pkg/file/file_test.goLength of output: 1278
server/schemas/asset.graphql (3)
12-12: LGTM! Clear and consistent schema definition.The
contentEncodingfield is appropriately added to the Asset type, allowing for proper representation of encoded assets.
30-30: LGTM! Consistent field addition across types.The
contentEncodingfield is correctly added to the AssetFile type, maintaining consistency with the Asset type.
60-61: LGTM! Well-documented input field.The comment clearly explains the usage of gzip content encoding, helping API consumers understand how to properly use this feature.
server/schemas/integration.yml (3)
1403-1406: LGTM! Comprehensive API specification for asset creation.The addition of
contentTypeandcontentEncodingfields to the multipart/form-data schema properly supports the gzip upload functionality.
1422-1423: LGTM! Consistent field addition in JSON schema.The
contentEncodingfield is correctly added to the JSON request body schema, maintaining consistency with the multipart/form-data schema.
1458-1459: LGTM! Upload API properly supports content encoding.The
contentEncodingfield is appropriately added to the asset upload creation endpoint.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/internal/infrastructure/gcp/file.go (1)
242-244:⚠️ Potential issueFix incorrect ContentType assignment
The ContentType is incorrectly assigned to ContentEncoding.
- writer.ContentEncoding = getContentType(file.Name) + writer.ObjectAttrs.ContentType = getContentType(file.Name)
🧹 Nitpick comments (8)
server/schemas/integration.yml (4)
1403-1406: Add documentation for content encoding values.The new
contentTypeandcontentEncodingfields in the multipart/form-data schema lack documentation about their allowed values. ForcontentEncoding, it would be helpful to specify that it supports "gzip" encoding.Add enum or description to clarify the supported values:
contentType: type: string contentEncoding: type: string + description: Content encoding of the file (e.g., gzip) + enum: + - gzip
1422-1423: Maintain consistency with multipart/form-data schema.The
contentEncodingfield in the application/json schema should have the same documentation as its counterpart in the multipart/form-data schema.Add the same enum and description:
contentEncoding: type: string + description: Content encoding of the file (e.g., gzip) + enum: + - gzip
1445-1445: Fix grammatical error in description.The description "Issue an URL" contains a grammatical error. The article "an" is used before vowel sounds, but "URL" is pronounced as "you-ar-el", which starts with a consonant sound.
Apply this correction:
- description: Issue an URL and a token to upload an asset. + description: Issue a URL and a token to upload an asset.
1458-1459: Add documentation for contentEncoding in AssetUploadCreate.The
contentEncodingfield in the AssetUploadCreate operation should have consistent documentation with the AssetCreate operation.Add enum and description to match AssetCreate:
contentEncoding: type: string + description: Content encoding of the file (e.g., gzip) + enum: + - gzipserver/i18n/en.yml (2)
87-87: Consider adding supported formats to the content encoding error message.To improve user experience, consider making the error message more specific about which content encodings are supported.
-unsupported content encoding: "" +unsupported content encoding (supported: gzip): ""
Line range hint
87-95: Maintain consistent message formatting.The error messages use inconsistent formatting. Some end with ": ", some have no punctuation, and some have periods. Consider standardizing the format.
For example:
-unsupported content encoding: "" -unsupported entity: "" -unsupported geometry type: "" -unsupported operation: "" -uuid is required: "" -value is required: "" -views are not in the same model: "" -views length mismatch: "" -workspace id is required: "" +unsupported content encoding +unsupported entity +unsupported geometry type +unsupported operation +uuid is required +value is required +views are not in the same model +views length mismatch +workspace id is requiredserver/internal/infrastructure/gcp/file.go (2)
248-253: Enhance gzip content type handlingThe gzip content type handling could be more robust by:
- Moving the content type check before the gzip block
- Adding support for other common gzip content types
+ if writer.ObjectAttrs.ContentType == "" || + writer.ObjectAttrs.ContentType == "application/gzip" || + writer.ObjectAttrs.ContentType == "application/x-gzip" { + writer.ObjectAttrs.ContentType = "application/octet-stream" + } + if file.ContentEncoding == "gzip" { writer.ObjectAttrs.ContentEncoding = "gzip" - if writer.ObjectAttrs.ContentType == "" || writer.ObjectAttrs.ContentType == "application/gzip" { - writer.ObjectAttrs.ContentType = "application/octet-stream" - } }
331-336: LGTM! Consider future encoding support.The validation logic is correct. Consider making the supported encodings configurable for future extensibility (e.g., br, deflate).
+var supportedContentEncodings = map[string]bool{ + "": true, + "gzip": true, +} + func validateContentEncoding(ce string) error { - if ce != "" && ce != "gzip" { + if !supportedContentEncodings[ce] { return gateway.ErrUnsupportedContentEncoding } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/i18n/en.yml(1 hunks)server/i18n/ja.yml(1 hunks)server/internal/infrastructure/gcp/file.go(5 hunks)server/schemas/integration.yml(4 hunks)
🔇 Additional comments (6)
server/i18n/ja.yml (1)
87-89: LGTM! The translations are consistent and well-formatted.The new and modified translations:
- Follow the consistent style of ending messages with "です。"
- Maintain proper Japanese grammar and punctuation
- Align well with the PR's objective of implementing gzip upload support by adding appropriate error messages for unsupported content encoding
server/i18n/en.yml (2)
Line range hint
87-95: LGTM! The new error messages align well with the gzip upload feature.The added error messages provide good coverage for validation scenarios and unsupported operations.
90-90: Make the "unsupported operation" message more specific.The error message "unsupported operation" is too generic. Consider making it more specific to the context where it's used.
Let's check how this error message is used in the codebase:
server/internal/infrastructure/gcp/file.go (3)
123-123: LGTM!The method correctly delegates to the upload method and maintains proper error handling.
160-162: LGTM!The implementation correctly handles Content-Encoding for both the signed URL generation and response structure.
Also applies to: 169-173
255-255: Verify error handling for gzipped contentEnsure that errors during gzipped content handling (e.g., corrupted gzip data) are properly propagated to the caller.
✅ Verification successful
Error handling for gzipped content is properly implemented
The code has comprehensive error handling for gzipped content:
- Validates content encoding before upload
- Sets appropriate content type and encoding headers
- Propagates io.Copy errors with proper logging
- Handles writer close errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for gzip-related error handling in the codebase rg -A 5 'gzip.*err|err.*gzip' # Search for tests related to gzip error handling rg -A 5 'Test.*[Gg]zip.*Error|test.*[Gg]zip.*error'Length of output: 568
Script:
#!/bin/bash # Check the implementation file cat server/internal/infrastructure/gcp/file.go # Search for io.Copy error handling patterns rg -A 3 'io\.Copy.*err.*return|return.*err.*io\.Copy'Length of output: 8047
note: need to test
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Technical Updates