chore(server): add UploadAssetFromURL on gcs file implementation #1379
Conversation
|
Warning Rate limit exceeded@soneda-yuya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 5 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 enhancements to the file upload and storage infrastructure, primarily focusing on Google Cloud Storage (GCS) functionality. The changes include adding a development mode for GCS interactions, implementing a new method for uploading assets from URLs, and updating the testing infrastructure. A new Fake GCS Server is integrated into the CI workflow and Docker Compose configuration, providing a more robust local development and testing environment. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FileGateway
participant GCSRepo
participant GCSClient
Client->>FileGateway: UploadAssetFromURL(context, url)
FileGateway->>GCSRepo: UploadAssetFromURL(context, url)
GCSRepo->>GCSRepo: Validate URL
GCSRepo->>GCSClient: Fetch URL content
GCSRepo->>GCSClient: Upload to Storage
GCSClient-->>GCSRepo: Upload result
GCSRepo-->>FileGateway: Return uploaded asset URL
FileGateway-->>Client: Return uploaded asset details
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
soneda-yuya
force-pushed
the
chore/new-method-on-gcs
branch
9 times, most recently
from
2796ac3 to
b43f1a0
Compare
soneda-yuya
force-pushed
the
chore/new-method-on-gcs
branch
from
b43f1a0 to
364daa4
Compare
soneda-yuya
force-pushed
the
chore/new-method-on-gcs
branch
from
2582d68 to
d512050
Compare
✅ Deploy Preview for reearth-web 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: 2
🧹 Nitpick comments (8)
server/internal/infrastructure/gcs/file.go (2)
35-35: Development mode flag detection
Adding theisDev boolfield is an effective approach to differentiate between production and development environments.If you anticipate future expansion (multiple dev endpoints, different environment flags), consider grouping such env-specific settings into a dedicated struct. This ensures the code remains extensible.
41-43: Hardcoded local dev endpoint
DefiningdevBaseURL = "http://localhost:4443/storage/v1/b"works for local testing, but it might be more flexible to read from configuration or environment variables.-const ( - devBaseURL = "http://localhost:4443/storage/v1/b" -) +const devBaseURLDefault = "http://localhost:4443/storage/v1/b" +func DevBaseURL() string { + if env := os.Getenv("GCS_DEV_ENDPOINT"); env != "" { + return env + } + return devBaseURLDefault +}server/internal/infrastructure/gcs/file_test.go (4)
14-15: Cloud/Hyperscaler-related imports
The new imports for"cloud.google.com/go/storage","github.com/google/uuid", and"google.golang.org/api/iterator"are all required to manage GCS buckets, unique IDs, and object iteration.Keep a watchful eye on version updates to maintain compatibility and avoid known issues.
Also applies to: 16-16, 18-18
22-50: Helper functionuploadTestData
This function is an effective convenience method for populating test data in a GCS bucket. A few considerations:
- File path: Hardcoded
"testdata/geojson.json"is fine for now, but dynamic path resolution may help if the file moves.- Error messages: Consider returning the error instead of
panicfor better test control.
52-84: Bucket creation and teardown helpers
createBucketanddeleteBucketWithObjectsabstract away GCS complexities well. Usingpanicis okay for test setup, but returning errors could give more granular test control.
102-134: TestTestUploadAssetFromURL
This test thoroughly checks the end-to-end logic of uploading an asset from one bucket (source) to another (destination). The usage of a separate dev-based URL, ephemeral bucket names, and a random test file name is solid.Consider adding assertions for file size or verifying the final content in the destination bucket if feasible.
server/internal/infrastructure/s3/s3.go (1)
106-109: Enhance error message for better context.The error message could be more descriptive to help with debugging and maintenance.
Apply this diff to improve the error message:
func (f *fileRepo) UploadAssetFromURL(ctx context.Context, u *url.URL) (*url.URL, int64, error) { - // Note: not implemented - return nil, 0, errors.New("not implemented") + return nil, 0, errors.New("UploadAssetFromURL: not implemented for S3 storage") }.github/workflows/ci_server.yml (1)
33-39: Add container health check and cleanup.The Fake GCS Server setup could be improved with health checking and proper cleanup.
Apply this diff to enhance the setup:
- name: Start Fake GCS Server run: | docker run -d --name fake-gcs-server \ -p 4443:4443 \ -v /tmp/gcs:/storage \ fsouza/fake-gcs-server:1.52.1 \ - -scheme http + -scheme http && \ + timeout 30s bash -c 'until curl -s http://localhost:4443 > /dev/null; do sleep 1; done' + - name: Cleanup Fake GCS Server + if: always() + run: docker rm -f fake-gcs-server
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci_server.yml(1 hunks)docker-compose.yml(1 hunks)server/internal/app/repo.go(1 hunks)server/internal/infrastructure/fs/file.go(1 hunks)server/internal/infrastructure/gcs/file.go(6 hunks)server/internal/infrastructure/gcs/file_test.go(2 hunks)server/internal/infrastructure/gcs/testdata/geojson.json(1 hunks)server/internal/infrastructure/s3/s3.go(1 hunks)server/internal/usecase/gateway/file.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/internal/infrastructure/gcs/testdata/geojson.json
- docker-compose.yml
🔇 Additional comments (9)
server/internal/infrastructure/gcs/file.go (4)
8-8: Additional HTTP dependency looks good
The newly added import"net/http"is appropriate for the new remote URL fetching logic. No issues detected.
22-22: Required Google API options import
The introduction of the"google.golang.org/api/option"import aligns with the new client options. This is necessary to handle authentication and endpoint overrides. Looks fine.
289-292: Dev-specific client configuration
Conditionally constructing thestorage.NewClientwithWithoutAuthentication()and custom endpoint is a clean approach, ensuring production credentials remain separate. This looks good.
Line range hint
45-62: Constructor signature change
The updated signatureNewFile(isDev bool, bucketName, base string, cacheControl string)makes sense to support a development endpoint. However, it might break existing call sites.Please run a quick search to confirm that all invocations of
NewFileare updated accordingly:✅ Verification successful
All call sites are properly updated to use the new signature
The constructor change only affects the GCS implementation, and both the production usage inserver/internal/app/repo.goand test usage inserver/internal/infrastructure/gcs/file_test.goare using the correct signature with all required parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for all references to NewFile across the repo rg -A 5 $'NewFile('Length of output: 87
Script:
#!/bin/bash # Search using ripgrep with escaped parentheses echo "=== Searching with ripgrep ===" rg -A 5 "NewFile\(" # Search using ast-grep for function calls echo -e "\n=== Searching with ast-grep ===" ast-grep --pattern 'NewFile($$$)'Length of output: 18280
server/internal/usecase/gateway/file.go (1)
24-24: Interface extension
AddingUploadAssetFromURLto theFileinterface correctly abstracts the asset-fetching logic. Ensure other storage backends implement this method (even as a no-op or future placeholder) to maintain interface consistency.✅ Verification successful
Interface consistency verified across all storage backends
All three File interface implementers (GCS, S3, and filesystem) have defined theUploadAssetFromURLmethod, with GCS having a full implementation while S3 and filesystem have placeholder implementations for future work.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that all other File interface implementers define UploadAssetFromURL ast-grep --pattern $'type $_ struct { $$$ }\n\nfunc ($_ *$_) UploadAssetFromURL($_,$_)'Length of output: 86
Script:
#!/bin/bash # Find the File interface definition echo "=== File Interface Definition ===" ast-grep --pattern 'type File interface { $$$ }' # Find struct types and their UploadAssetFromURL implementations echo -e "\n=== Struct Implementations ===" rg "type.*struct" -A 20 server/internal/usecase/gateway/Length of output: 3437
Script:
#!/bin/bash # Find all implementations of UploadAssetFromURL echo "=== UploadAssetFromURL Implementations ===" rg "func.*UploadAssetFromURL" -A 2 # Find implementations of other File interface methods to identify implementers echo -e "\n=== File Interface Implementers ===" rg "func.*UploadAsset.*\(context\.Context, \*file\.File\)" -B 5Length of output: 1356
server/internal/infrastructure/gcs/file_test.go (3)
4-7: Added imports for URL-based tests
Imports for"context","errors","fmt", and"io"are consistent with the newly introduced tests. This is standard for extended coverage.
9-10: Additional path and OS usage
Imports"os"and"path"seem necessary for handling file-based test data and path manipulations.
101-101: Empty line
No comment needed.server/internal/app/repo.go (1)
112-112: LGTM: Development mode parameter added correctly.The addition of
conf.Devparameter enables proper configuration for development environments, which is essential for testing with the Fake GCS Server.
pyshx
changed the title
Overview
we need to save url as asset on each storage service
What I've done
UploadAssetFromURLmethod on gcs storage infrastructure.What I haven't done
UploadAssetFromURLmethod on local storage infrastructure.UploadAssetFromURLmethod on storage s3 infrastructure.How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Infrastructure
Testing