feat(server): asset url management #1303
Conversation
WalkthroughThis pull request introduces several changes primarily focused on enhancing asset management within the server application. Key modifications include the addition of new environment variables, updates to asset-related methods for publishing and unpublishing, and improvements to the API schema. New test functions are added to validate model updates and ordering, while existing methods are refined to support asset visibility and access control. Overall, the changes aim to provide more comprehensive asset management functionalities. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 24
🧹 Outside diff range and nitpick comments (29)
server/internal/usecase/repo/asset.go (1)
21-21: LGTM! Consider adding documentation.The new
FindByUUIDmethod is a logical addition to the interface, following the same pattern as other finder methods. Consider adding a documentation comment to describe its purpose and expected behavior.+// FindByUUID finds an asset by its UUID. +// Returns nil and error if not found or if an error occurs during the operation. FindByUUID(context.Context, string) (*asset.Asset, error)server/internal/usecase/gateway/file.go (1)
55-56: Consider using named parameters for better clarity.The string parameters in both methods could benefit from being named to improve code readability and maintainability.
Consider this alternative signature:
- PublishAsset(context.Context, string, string) error - UnpublishAsset(context.Context, string, string) error + PublishAsset(ctx context.Context, assetID string, location string) error + UnpublishAsset(ctx context.Context, assetID string, location string) errorserver/pkg/asset/builder.go (1)
125-128: Consider adding method documentationWhile the implementation is correct and follows the builder pattern, consider adding a documentation comment to explain the purpose and implications of making an asset public.
+// Public sets the visibility of the asset. When set to true, the asset will be publicly accessible. func (b *Builder) Public(public bool) *Builder {server/pkg/asset/asset.go (3)
23-23: Add documentation for thepublicfield.Consider adding a comment to document the purpose and implications of the
publicfield, as it represents an important domain concept affecting asset visibility.+ // public indicates whether the asset is publicly accessible public bool
86-92: Add documentation for Public() and UpdatePublic() methods.These methods manage asset visibility, which is a crucial domain concept. Consider adding documentation to explain their purpose, behavior, and any side effects.
+ // Public returns whether the asset is publicly accessible func (a *Asset) Public() bool { + // UpdatePublic updates the public visibility status of the asset func (a *Asset) UpdatePublic(public bool) {
Line range hint
94-116: Fix incomplete Clone() implementation.The
Clone()method is missing thepublicfield, which breaks the deep copy contract. This could lead to unexpected behavior when cloning assets.func (a *Asset) Clone() *Asset { if a == nil { return nil } return &Asset{ id: a.id.Clone(), project: a.project.Clone(), createdAt: a.createdAt, user: a.user.CloneRef(), integration: a.integration.CloneRef(), fileName: a.fileName, size: a.size, previewType: a.previewType, uuid: a.uuid, thread: a.thread.Clone(), archiveExtractionStatus: a.archiveExtractionStatus, flatFiles: a.flatFiles, + public: a.public, } }server/internal/usecase/interfaces/asset.go (1)
61-61: LGTM! Good addition for flexible asset lookup.Adding UUID-based lookup alongside ID-based lookup is a good practice, especially for public-facing APIs where exposing internal IDs might not be desirable.
Consider documenting whether this UUID is meant for public API consumption and if it should be used in asset URLs. This helps maintain clear boundaries between internal and external identifiers.
server/internal/infrastructure/memory/asset.go (1)
Line range hint
65-93: Consider breaking down the pagination logicWhile the implementation is correct, the method handles both filtering and pagination logic. Consider extracting the pagination logic into a separate helper function for better maintainability.
+func createPageInfo(result []*asset.Asset) *usecasex.PageInfo { + var startCursor, endCursor *usecasex.Cursor + if len(result) > 0 { + startCursor = lo.ToPtr(usecasex.Cursor(result[0].ID().String())) + endCursor = lo.ToPtr(usecasex.Cursor(result[len(result)-1].ID().String())) + } + return usecasex.NewPageInfo( + int64(len(result)), + startCursor, + endCursor, + true, + true, + ) +} func (r *Asset) FindByProject(_ context.Context, id id.ProjectID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { if !r.f.CanRead(id) { return nil, usecasex.EmptyPageInfo(), nil } if r.err != nil { return nil, nil, r.err } result := asset.List(r.data.FindAll(func(_ asset.ID, v *asset.Asset) bool { return v.Project() == id })).SortByID() - var startCursor, endCursor *usecasex.Cursor - if len(result) > 0 { - startCursor = lo.ToPtr(usecasex.Cursor(result[0].ID().String())) - endCursor = lo.ToPtr(usecasex.Cursor(result[len(result)-1].ID().String())) - } - - return result, usecasex.NewPageInfo( - int64(len(result)), - startCursor, - endCursor, - true, - true, - ), nil + return result, createPageInfo(result), nil }server/internal/infrastructure/aws/taskrunner.go (1)
94-101: Consider URL management architectural implicationsThe introduction of proper URL parsing is a good architectural improvement. However, consider:
- Centralizing URL construction logic in a dedicated service/package
- Adding URL validation middleware for consistent security checks
- Implementing URL signing if assets require secure access
server/.env.example (2)
47-48: Consider adding HTTPS and documentation for ASSETBASEURL.While moving the variable to the Assets section improves organization, consider:
- Using HTTPS by default (https://localhost:8080) for better security
- Adding a comment explaining the purpose and usage of this variable
+# Base URL for asset access and management REEARTH_CMS_ASSETBASEURL=https://localhost:8080
52-52: Fix typo in comment: "privet" → "private".There's a typo in the comment explaining private asset access.
-# and the privet assets can be accessed only by the cms api protected by auth +# and the private assets can be accessed only by the cms api protected by authserver/internal/app/app.go (1)
93-97: Add documentation and consider error handling.The new middleware function has two potential improvements:
- Add godoc comments explaining the function's purpose and behavior
- Consider handling initialization errors instead of using
lo.Mustwhich can panicConsider this improvement:
+// jwtParseMiddleware creates a middleware that parses JWT tokens from requests +// using configured providers. It sets authentication info in the request context +// and always requires valid authentication. func jwtParseMiddleware(cfg *ServerConfig) echo.MiddlewareFunc { - return echo.WrapMiddleware(lo.Must( - appx.AuthMiddleware(cfg.Config.JWTProviders(), adapter.ContextAuthInfo, true), - )) + middleware, err := appx.AuthMiddleware(cfg.Config.JWTProviders(), adapter.ContextAuthInfo, true) + if err != nil { + log.Fatalf("failed to initialize JWT middleware: %v", err) + } + return echo.WrapMiddleware(middleware) }server/internal/infrastructure/mongo/asset.go (1)
64-68: Consider adding documentation for UUID usage.Since this is part of a larger asset URL management feature, it would be helpful to add documentation comments explaining:
- The purpose and format of the UUID field
- When to use FindByUUID vs FindByID
- Any validation requirements for UUID values
+// FindByUUID retrieves an asset by its UUID. +// This method is particularly useful for asset URL management where the UUID is used as a public identifier. func (r *Asset) FindByUUID(ctx context.Context, uuid string) (*asset.Asset, error) {server/internal/app/repo.go (3)
75-79: Consider extracting storage initialization logic.The conditional initialization pattern is repeated for GCS, S3, and local storage. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+func initializeStorage(public bool, initPublic, initPrivate func() (gateway.File, error)) (gateway.File, error) { + if public { + return initPublic() + } + return initPrivate() +} // Usage in GCS initialization: -if conf.Asset_Public { - fileRepo, err = gcp.NewFile(conf.GCS.BucketName, conf.AssetBaseURL, conf.GCS.PublicationCacheControl) -} else { - fileRepo, err = gcp.NewFileWithACL(conf.GCS.BucketName, conf.AssetBaseURL, privateBase, conf.GCS.PublicationCacheControl) -} +fileRepo, err = initializeStorage( + conf.Asset_Public, + func() (gateway.File, error) { + return gcp.NewFile(conf.GCS.BucketName, conf.AssetBaseURL, conf.GCS.PublicationCacheControl) + }, + func() (gateway.File, error) { + return gcp.NewFileWithACL(conf.GCS.BucketName, conf.AssetBaseURL, privateBase, conf.GCS.PublicationCacheControl) + }, +)
96-100: Consider making the local storage base path configurable.The base path "data" is hardcoded. Consider making this configurable through the configuration to allow for different deployment scenarios.
-datafs := afero.NewBasePathFs(afero.NewOsFs(), "data") +datafs := afero.NewBasePathFs(afero.NewOsFs(), conf.LocalStorage.BasePath)
72-100: Consider implementing a storage factory pattern.The current implementation handles multiple storage backends with similar initialization patterns. Consider implementing a storage factory pattern to:
- Encapsulate storage-specific initialization logic
- Make it easier to add new storage backends
- Improve testability
- Centralize configuration validation
This would also help in maintaining consistency in how public/private access is handled across different storage implementations.
Would you like me to provide an example implementation of the storage factory pattern?
server/internal/infrastructure/mongo/mongodoc/asset.go (1)
27-27: Consider adding BSON/JSON tags to the Public field.For consistency with MongoDB serialization and to ensure proper field naming in the database, consider adding BSON and JSON tags.
- Public bool + Public bool `bson:"public" json:"public"`server/internal/infrastructure/fs/file.go (2)
49-63: Consider consistent error handling for base URL validation.While the implementation is good, the error handling for private base URL validation differs from public base URL validation. Consider using the same error type (ErrInvalidBaseURL) for consistency.
- return nil, rerror.NewE(i18n.T("invalid base URL")) + return nil, ErrInvalidBaseURL
158-160: Add input validation to grtURL helper.The helper function should validate its inputs to prevent potential panics or malformed URLs.
func grtURL(host *url.URL, uuid, fName string) string { + if host == nil || uuid == "" || fName == "" { + return "" + } return host.JoinPath(assetDir, uuid[:2], uuid[2:], fName).String() }server/internal/adapter/integration/asset.go (2)
50-51: Consider usingassetURLinstead ofaUrlfor better Go naming conventions.While the current naming works, using
assetURLwould better follow Go conventions where URL is typically uppercase and variable names are more descriptive.Also applies to: 126-127, 164-165, 241-242, 263-264
236-243: Consider extracting common file lookup and response creation logic.The file lookup and response creation logic is duplicated across multiple methods. Consider extracting this into a helper function to improve maintainability and reduce duplication.
func (s *Server) createAssetResponse(ctx context.Context, a *asset.Asset, op *usecase.Operator) (*integrationapi.Asset, error) { uc := adapter.Usecases(ctx) f, err := uc.Asset.FindFileByID(ctx, a.ID(), op) if err != nil && !errors.Is(err, rerror.ErrNotFound) { return nil, err } assetURL := uc.Asset.GetURL(a) return integrationapi.NewAsset(a, f, assetURL, true), nil }Also applies to: 258-265
server/e2e/gql_model_test.go (3)
Line range hint
333-347: Enhance test coverage for model updates.While the basic update functionality is tested, consider adding:
- Assertions for the
publicflag behavior- Negative test cases (e.g., duplicate keys, invalid values)
- Test data cleanup in a deferred function
Example enhancement:
func TestUpdateModel(t *testing.T) { e := StartServer(t, &app.Config{}, true, baseSeederUser) pId, _ := createProject(e, wId.String(), "test", "test", "test-2") mId, _ := createModel(e, pId, "test", "test", "test-2") + // Cleanup + defer func() { + deleteModel(e, mId) + deleteProject(e, pId) + }() res := updateModel(e, mId, lo.ToPtr("updated name"), lo.ToPtr("updated desc"), lo.ToPtr("updated_key"), false) res.Object(). Value("data").Object(). Value("updateModel").Object(). Value("model").Object(). HasValue("name", "updated name"). HasValue("description", "updated desc"). - HasValue("key", "updated_key") + HasValue("key", "updated_key"). + HasValue("public", false) + + // Test invalid update + res = updateModel(e, mId, nil, nil, lo.ToPtr(""), false) + res.Path("$.errors").Array().NotEmpty() }
Line range hint
349-386: Improve test robustness for model ordering.The test covers basic ordering functionality but could be enhanced for better reliability:
- Validate initial order before reordering
- Add edge cases for order updates
- Add test data cleanup
- Add synchronization checks
Example enhancement:
func TestUpdateModelsOrder(t *testing.T) { e := StartServer(t, &app.Config{}, true, baseSeederUser) pId, _ := createProject(e, wId.String(), "test", "test", "test-2") + // Cleanup + defer func() { + deleteProject(e, pId) + }() mId1, _ := createModel(e, pId, "test1", "test", "test-1") mId2, _ := createModel(e, pId, "test2", "test", "test-2") mId3, _ := createModel(e, pId, "test3", "test", "test-3") mId4, res := createModel(e, pId, "test4", "test", "test-4") + + // Verify initial order + res.Path("$.data.createModel.model.order").Number().Equal(3) + for i, id := range []string{mId1, mId2, mId3, mId4} { + _, _, r := getModel(e, id) + r.Path("$.data.node.order").Number().Equal(i) + } - res.Object(). - Value("data").Object(). - Value("createModel").Object(). - Value("model").Object(). - HasValue("name", "test4"). - HasValue("key", "test-4"). - HasValue("order", 3) + // Test invalid order updates + invalidRes := updateModelsOrder(e, []string{mId4, mId4}) // Duplicate ID + invalidRes.Path("$.errors").Array().NotEmpty() + + invalidRes = updateModelsOrder(e, []string{mId4, "invalid-id"}) // Invalid ID + invalidRes.Path("$.errors").Array().NotEmpty() res2 := updateModelsOrder(e, []string{mId4, mId1, mId2, mId3}) res2.Path("$.data.updateModelsOrder.models[:].id").Array().IsEqual([]string{mId4, mId1, mId2, mId3}) res2.Path("$.data.updateModelsOrder.models[:].order").Array().IsEqual([]int{0, 1, 2, 3}) deleteModel(e, mId2) + + // Verify all remaining models have continuous order _, _, res3 := getModel(e, mId3) - res3.Object(). Value("data").Object(). Value("node").Object(). HasValue("id", mId3). HasValue("order", 2) + + // Verify order for all remaining models + for i, id := range []string{mId4, mId1, mId3} { + _, _, r := getModel(e, id) + r.Path("$.data.node.order").Number().Equal(i) + } }
Line range hint
1-386: Consider improving test organization and documentation.The test file has a good structure but could benefit from:
- Adding package documentation to explain the test suite's purpose
- Grouping related tests using subtests
- Adding comments to explain complex test scenarios
Example enhancement:
package e2e +// Package e2e provides end-to-end tests for the GraphQL API. +// These tests verify the integration of model management operations +// including creation, updates, and ordering. import ( "net/http" "testing" // ... other imports ) func TestUpdateModel(t *testing.T) { - e := StartServer(t, &app.Config{}, true, baseSeederUser) - // ... test implementation + t.Run("basic update", func(t *testing.T) { + e := StartServer(t, &app.Config{}, true, baseSeederUser) + // ... current test implementation + }) + + t.Run("invalid updates", func(t *testing.T) { + // ... new test for invalid cases + }) } func TestUpdateModelsOrder(t *testing.T) { - e := StartServer(t, &app.Config{}, true, baseSeederUser) - // ... test implementation + t.Run("reorder models", func(t *testing.T) { + e := StartServer(t, &app.Config{}, true, baseSeederUser) + // ... current test implementation + }) + + t.Run("maintain order after deletion", func(t *testing.T) { + // ... separated deletion test + }) }server/internal/app/config.go (1)
45-47: Add documentation for the Asset_Public field.Please add a documentation comment explaining the purpose and implications of the
Asset_Publicflag, particularly its effect on asset visibility when using GCS and S3 storage.Apply this diff to add documentation:
// asset + // Asset_Public determines if assets should be publicly accessible when using GCS or S3 storage Asset_Public bool `pp:",omitempty"` AssetBaseURL string `pp:",omitempty"`server/internal/usecase/interactor/asset.go (1)
Line range hint
241-278: Consider enhancing error messages for better debuggingThe implementation is solid with proper operator validation, transaction handling, and atomic status updates. However, consider wrapping errors with more context for better debugging.
Example enhancement:
- if err != nil { - return nil, err + if err != nil { + return nil, fmt.Errorf("failed to find asset %s: %w", aId, err)server/internal/app/file.go (1)
20-22: Avoid variable shadowing for clarityThe variables
authMiddlewareandjwtParseMiddlewareare assigned the results ofauthMiddleware(cfg)(eh)andjwtParseMiddleware(cfg)(eh), respectively, which have the same names as the functions. This may lead to confusion due to variable shadowing.Consider renaming the variables to something like
authMiddlewareHandlerandjwtParseMiddlewareHandlerto improve readability.server/internal/infrastructure/aws/file.go (1)
378-397: Implement Comprehensive Error Handling inpublishMethodThe
publishmethod correctly updates the object's ACL based on thepublicparameter. Ensure that all potential errors from thePutObjectAclcall are handled, and consider logging the errors for improved observability.server/internal/adapter/integration/server.gen.go (1)
1460-1462: Confirm the use of HTTP methods and endpoints for asset publishingThe routes for publishing and unpublishing assets use POST requests:
router.POST(baseURL+"/assets/:assetId/publish", wrapper.AssetPublish)router.POST(baseURL+"/assets/:assetId/unpublish", wrapper.AssetUnpublish)Ensure that using POST is appropriate for these actions according to your API design standards. In some cases, PUT or PATCH might be more suitable for actions that alter the state of a resource.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
server/.env.example(1 hunks)server/e2e/gql_model_test.go(1 hunks)server/e2e/integration_schema_test.go(1 hunks)server/internal/adapter/gql/resolver_asset.go(1 hunks)server/internal/adapter/integration/asset.go(4 hunks)server/internal/adapter/integration/server.gen.go(7 hunks)server/internal/app/app.go(2 hunks)server/internal/app/config.go(1 hunks)server/internal/app/file.go(1 hunks)server/internal/app/repo.go(1 hunks)server/internal/infrastructure/aws/file.go(6 hunks)server/internal/infrastructure/aws/taskrunner.go(2 hunks)server/internal/infrastructure/fs/common.go(1 hunks)server/internal/infrastructure/fs/file.go(2 hunks)server/internal/infrastructure/gcp/file.go(4 hunks)server/internal/infrastructure/memory/asset.go(5 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/usecase/gateway/file.go(1 hunks)server/internal/usecase/interactor/asset.go(3 hunks)server/internal/usecase/interactor/asset_test.go(3 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(2 hunks)server/pkg/integrationapi/asset.go(1 hunks)server/pkg/integrationapi/types.gen.go(1 hunks)server/schemas/integration.yml(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/e2e/integration_schema_test.go
🔇 Additional comments (55)
server/internal/infrastructure/fs/common.go (1)
11-11: Verify URL construction after base URL change.
The removal of "/assets" from the base URL might affect existing URL construction logic. Please ensure all code that builds asset URLs is updated accordingly.
Consider introducing a dedicated URL builder function to handle asset URL construction consistently across the codebase.
✅ Verification successful
URL base change is safe to proceed
Based on the code analysis:
- The
defaultBaseis only used infileRepoinitialization to setpublicBase - Asset URL construction is handled through dedicated path joining functions in different implementations (AWS, GCP, FS)
- The actual asset paths are constructed using proper URL/path joining functions that handle path segments correctly
- No direct string concatenation with "/assets" was found that could break due to this change
The removal of "/assets" from the base URL is safe as:
- Asset paths are properly managed through dedicated functions in each storage implementation
- The asset directory is explicitly included in path construction (via
assetDirconstant) - All URL constructions use proper joining methods (
url.JoinPath,path.Join) rather than string concatenation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential URL construction patterns that might be affected
# Look for string concatenation or URL joining with "/assets"
rg -A 2 "defaultBase.*\+.*\"assets\"" ./server
# Look for direct references to "/assets" path
rg -A 2 "\"/assets\"" ./server
Length of output: 77
Script:
#!/bin/bash
# Let's perform a more comprehensive search for URL construction patterns
# 1. Search for any usage of defaultBase
rg -A 3 "defaultBase" ./server
# 2. Look for URL path construction patterns
rg -A 3 "\.Join|\.JoinPath|path\.Join.*asset" ./server
# 3. Search for asset-related functions and methods
ast-grep --pattern 'func $_ ($$$) {
$$$
defaultBase
$$$
}'
Length of output: 11236
server/internal/usecase/gateway/file.go (1)
55-56: Verify implementation consistency across storage providers.
The interface changes require corresponding implementations in AWS, GCP, and filesystem providers.
✅ Verification successful
Let me try a different search approach to verify the implementations.
Let's try one more search with a different pattern to examine the implementation files:
Implementation consistency verified across all storage providers
All storage providers (AWS, GCP, and filesystem) have implemented the required PublishAsset and UnpublishAsset methods:
- AWS and GCP implementations return
ErrUnsupportedOperationwhen the storage is public - Filesystem implementation returns
nilfor both operations - Method signatures are consistent with the interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all storage providers implement the new methods correctly
# Test 1: Check AWS implementation
echo "Checking AWS implementation..."
ast-grep --pattern 'func (*$_) PublishAsset(context.Context, string, string) error' server/internal/infrastructure/aws/file.go
ast-grep --pattern 'func (*$_) UnpublishAsset(context.Context, string, string) error' server/internal/infrastructure/aws/file.go
# Test 2: Check GCP implementation
echo "Checking GCP implementation..."
ast-grep --pattern 'func (*$_) PublishAsset(context.Context, string, string) error' server/internal/infrastructure/gcp/file.go
ast-grep --pattern 'func (*$_) UnpublishAsset(context.Context, string, string) error' server/internal/infrastructure/gcp/file.go
# Test 3: Check filesystem implementation
echo "Checking filesystem implementation..."
ast-grep --pattern 'func (*$_) PublishAsset(context.Context, string, string) error' server/internal/infrastructure/fs/file.go
ast-grep --pattern 'func (*$_) UnpublishAsset(context.Context, string, string) error' server/internal/infrastructure/fs/file.go
Length of output: 1004
Script:
#!/bin/bash
# Let's first check if these files exist and then examine their content
echo "Checking for storage provider implementations..."
# Find the implementation files
echo "Looking for implementation files:"
fd "file.go" server/internal/infrastructure
# Now let's examine the content of these files if they exist
echo -e "\nChecking AWS implementation content:"
rg "type.*Storage" -A 5 server/internal/infrastructure/aws/file.go || echo "File not found"
echo -e "\nChecking GCP implementation content:"
rg "type.*Storage" -A 5 server/internal/infrastructure/gcp/file.go || echo "File not found"
echo -e "\nChecking filesystem implementation content:"
rg "type.*Storage" -A 5 server/internal/infrastructure/fs/file.go || echo "File not found"
Length of output: 1062
Script:
#!/bin/bash
# Let's examine the full content of these files to look for the implementations
echo "Examining AWS implementation:"
rg "func.*Asset" -A 3 server/internal/infrastructure/aws/file.go
echo -e "\nExamining GCP implementation:"
rg "func.*Asset" -A 3 server/internal/infrastructure/gcp/file.go
echo -e "\nExamining filesystem implementation:"
rg "func.*Asset" -A 3 server/internal/infrastructure/fs/file.go
# Also check if these are new files in the PR
echo -e "\nChecking git status of these files:"
git status server/internal/infrastructure/aws/file.go server/internal/infrastructure/gcp/file.go server/internal/infrastructure/fs/file.go
Length of output: 5231
server/pkg/asset/builder.go (2)
15-17: LGTM: Secure default initialization
Good practice to initialize new assets as private by default, following the principle of secure by default.
124-128: Verify URL management integration
Let's verify how this visibility flag integrates with the asset URL management feature.
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Public flag is properly integrated with URL management
The public flag is correctly integrated with the asset URL management system:
- The
publicfield in theAssetstruct is used to track asset visibility - The
Public()andUpdatePublic()methods provide access to this flag - The
Publish()andUnpublish()methods handle URL visibility by:- Calling
PublishAsset()orUnpublishAsset()on the file gateway - Updating the asset's public flag accordingly
- Persisting the changes to the database
- Calling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for URL-related code that might be affected by the public flag
# Look for URL generation or management code that should consider the public flag
rg -l "url|URL" --type go server/pkg/asset/
rg -l "url|URL" --type go server/internal/usecase/
# Check for any tests that verify URL behavior based on public flag
rg -l "TestAsset.*URL|TestAsset.*Public" --type go server/pkg/asset/
Length of output: 576
Script:
#!/bin/bash
# Let's examine the asset.go file to understand how the public flag is used
cat server/pkg/asset/asset.go
# Check the asset usecase implementation for URL generation logic
cat server/internal/usecase/interactor/asset.go
# Look at tests to understand expected behavior
cat server/internal/usecase/interactor/asset_test.go
Length of output: 55339
server/pkg/asset/asset.go (1)
23-23: Verify URL generation behavior for public assets.
Since this change introduces public asset visibility, we should verify how it affects URL generation across the codebase.
Also applies to: 86-92
✅ Verification successful
Let me gather more specific information about how the URL generation logic handles asset visibility.
This will help us understand if the URL generation logic already accounts for the public field or if there are potential issues to address.
Based on the results, I can now provide the final response since we have clear evidence of how URL generation handles public/private assets across different implementations.
URL generation already handles public asset visibility correctly
The URL generation logic properly accounts for the public field:
- AWS, GCP, and FS implementations all check
a.Public()to determine the correct base URL - They switch between
publicBaseandprivateBaseURLs based on the asset's visibility - The URL resolver pattern is consistently used across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL generation logic that might need to consider the public field
echo "Searching for URL generation patterns..."
rg -l "func.*URL.*Asset" --type go
echo "Searching for URLResolver usage..."
ast-grep --pattern 'URLResolver'
Length of output: 503
Script:
#!/bin/bash
# Check URL generation implementations and their handling of public/private assets
echo "Checking URL generation implementations:"
rg -A 5 "func.*URL.*Asset" --type go
echo -e "\nChecking asset visibility handling:"
rg -B 2 -A 5 "public.*bool" --type go
Length of output: 9954
server/pkg/integrationapi/asset.go (1)
32-32: LGTM! Verify OpenAPI schema alignment.
The addition of the Public field is clean and consistent with the existing implementation pattern. It appropriately exposes the asset's public visibility status through the integration API.
Let's verify the OpenAPI schema alignment:
✅ Verification successful
OpenAPI schema and implementation are properly aligned
The verification confirms that:
- The OpenAPI schema in
server/schemas/integration.ymldefines thepublicfield as a required boolean property in the asset schema - The generated type in
server/pkg/integrationapi/types.gen.gocorrectly includesPublic bool json:"public"as a non-optional field - The implementation in the review aligns with both the schema and the generated type
The addition of the Public field is correctly implemented and matches the API contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the OpenAPI schema includes the new Public field
# Expected: Find the public field definition in the asset schema
# Search for asset schema definition in OpenAPI specs
rg -A 10 "Asset:" "server/schemas/integration.yml"
Length of output: 48
Script:
#!/bin/bash
# Let's first find the OpenAPI schema files in the repository
fd -e yml -e yaml
# Then search for Asset-related schema definitions in any YAML files
rg -A 10 -i "asset:.*schema"
Length of output: 4741
Script:
#!/bin/bash
# Let's check the integration API types configuration and schema
cat server/pkg/integrationapi/types.cfg.yml
cat server/schemas/integration.yml
# Also check for Asset-related structs in the integration API package
rg -A 10 "type Asset struct" server/pkg/integrationapi/
Length of output: 62425
server/internal/usecase/interfaces/asset.go (1)
73-74: LGTM! Well-designed state management methods.
The Publish and Unpublish methods are symmetrical and follow consistent patterns with other interface methods. The inclusion of the operator parameter ensures proper access control.
Let's verify the error handling patterns in implementations:
✅ Verification successful
LGTM! Error handling and state management are well-implemented.
The implementation shows proper error handling and state management for both Publish and Unpublish operations:
- Validates operator permissions and checks for invalid operators
- Uses transaction management via
Run1 - Performs proper access control checks with
CanUpdate - Updates asset state via
UpdatePublic - Handles file system operations through gateways
- Saves state changes to the repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations to verify error handling
ast-grep --pattern 'func ($_) Publish(ctx context.Context, id id.AssetID, op *usecase.Operator) (*asset.Asset, error) {
$$$
}'
ast-grep --pattern 'func ($_) Unpublish(ctx context.Context, id id.AssetID, op *usecase.Operator) (*asset.Asset, error) {
$$$
}'
Length of output: 263
Script:
#!/bin/bash
# Let's try a broader search for implementations using ripgrep
rg -U "func.*Publish.*context\.Context.*AssetID.*Operator.*\{[\s\S]*?\}" --type go -A 10
rg -U "func.*Unpublish.*context\.Context.*AssetID.*Operator.*\{[\s\S]*?\}" --type go -A 10
# Also search for any files that might contain these implementations
fd -e go | xargs grep -l "Publish\|Unpublish"
Length of output: 3896
Script:
#!/bin/bash
# Let's check the complete implementation including error handling and state changes
rg -U "func \(i \*Asset\) Publish.*?\{.*?return" -A 20 server/internal/usecase/interactor/asset.go
rg -U "func \(i \*Asset\) Unpublish.*?\{.*?return" -A 20 server/internal/usecase/interactor/asset.go
# Let's also check for the actual state modification logic
rg "Published|Unpublished" server/internal/usecase/interactor/asset.go -A 5 -B 5
Length of output: 286
Script:
#!/bin/bash
# Let's try a simpler approach to get the full file content
cat server/internal/usecase/interactor/asset.go
# Also check the asset type definition to understand state handling
rg -U "type Asset struct" -A 10 -B 10 "server/internal/usecase/repo/asset.go"
rg -U "type Asset struct" -A 10 -B 10 "server/pkg/asset/asset.go"
Length of output: 17822
server/internal/infrastructure/memory/asset.go (4)
Line range hint 34-43: LGTM: Context parameter removal is consistent with implementation
The changes maintain the core functionality while removing the unused context parameter.
Line range hint 54-64: LGTM: Method maintains correct functionality
The changes are consistent with the context parameter removal pattern while preserving the essential sorting and filtering logic.
Line range hint 94-117: LGTM: Permission checks and error handling remain solid
The changes maintain proper permission checks and error handling while following the context parameter removal pattern.
44-52: Verify UUID uniqueness constraints
The implementation looks correct, but we should verify that UUIDs are unique across assets to prevent potential duplicate returns.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
UUID uniqueness is properly enforced
The implementation is safe because:
- MongoDB enforces UUID uniqueness through a unique index on the
uuidfield (mongox.IndexFromKey("uuid", true)) - The memory implementation correctly filters by UUID and project access permissions
- Asset uploads use unique UUIDs as shown by the unique index constraint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential UUID duplicates in asset-related files
rg -g '*.go' -A 5 'UUID.*string.*unique|unique.*UUID.*string'
# Look for UUID generation/validation logic
ast-grep --pattern 'func $$$UUID$$$() string {
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Let's try a different approach to analyze UUID handling
# Check for UUID field definitions and constraints in asset-related files
rg -g '*.go' -B 2 -A 2 'UUID.*string|uuid.*string'
# Look for asset creation/initialization to understand UUID assignment
ast-grep --pattern 'func New$$$Asset$$$($$$) $$$Asset {
$$$
}'
# Search for any UUID validation or uniqueness checks
rg -g '*.go' 'func.*ValidateUUID|func.*CheckUUID|func.*UniqueUUID'
# Look for database schema or storage layer constraints
fd -e go -e sql . --exec grep -l "uuid" {} \;
Length of output: 16911
Script:
#!/bin/bash
# Let's check the asset creation and UUID assignment logic
# Look for asset creation with UUID assignment
ast-grep --pattern 'func ($$$) Build() (*asset.Asset, error) {
$$$
}'
# Check mongo schema for UUID uniqueness constraints
rg -g '*.go' 'mongo.*Index.*uuid|Index.*uuid.*unique'
# Look for the Asset struct initialization
ast-grep --pattern 'type Asset struct {
$$$
}'
# Check for any UUID validation during asset creation
rg -g '*.go' -B 2 -A 5 'func.*Build.*Asset'
Length of output: 7216
server/internal/infrastructure/aws/taskrunner.go (3)
7-7: LGTM: Standard library import for URL parsing
The addition of the net/url package is appropriate for proper URL handling.
94-98: Enhance error handling for URL parsing
While the URL parsing implementation is good, consider these improvements:
- The error should be wrapped to maintain the error context
- Add validation for the parsed URL to ensure it's an absolute URL with the required components
Consider applying this improvement:
u, err := url.Parse(s3AssetBasePath)
if err != nil {
- return err
+ return rerror.ErrInternalBy(err)
}
+if !u.IsAbs() {
+ return rerror.ErrInternalBy(errors.New("s3AssetBasePath must be an absolute URL"))
+}#!/bin/bash
# Verify the s3AssetBasePath variable definition and usage
rg -A 2 "s3AssetBasePath" --type go99-101: Verify URL construction in getURL function
The URL resolver implementation looks clean, but we should verify:
- The getURL function's implementation for proper URL handling
- The security implications of URL construction with user-provided filenames
✅ Verification successful
URL construction is secure and properly implemented
The getURL implementation is verified to be secure because:
- It uses
url.URL.JoinPath()which safely handles path components and prevents path traversal - The URL is constructed with a fixed structure:
{host}/{assetBasePath}/{uuid[:2]}/{uuid[2:]}/{filename} - The UUID segments provide an additional layer of path isolation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and analyze the getURL function implementation
ast-grep --pattern 'func getURL($_, $_, $_) {
$$$
}'
# Look for any URL path traversal prevention
rg -A 5 "getURL.*filename" --type go
Length of output: 2708
Script:
#!/bin/bash
# Search for getURL function implementation with better pattern
ast-grep --pattern 'getURL($$$) {
$$$
}'
# Search for URL construction and path handling
rg -A 5 "func getURL" --type go
# Look for URL-related test cases
rg -A 5 "Test.*URL" --type go
Length of output: 8989
server/.env.example (1)
45-46: LGTM! Good organization of configuration.
The new "#Assets" section properly groups related configuration variables, improving maintainability.
server/internal/app/app.go (1)
68-71: LGTM! Verify middleware chain order.
The middleware chain order looks correct, with JWT parsing occurring before auth and usecase middleware.
Let's verify there are no other direct JWT middleware usages that should be migrated:
✅ Verification successful
JWT middleware usage is consistent across the codebase
The verification shows that JWT middleware usage is properly structured:
config.goonly contains provider configuration methodsm2m.gouses a specific M2M JWT provider for machine-to-machine authapp.gocontains the newjwtParseMiddlewarethat uses the general JWT providers
The middleware chain order in the original review is correct and all JWT middleware usages follow the same pattern using appx.AuthMiddleware.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct JWT middleware usage patterns
rg -l "JWTProviders|AuthMiddleware.*JWT" --type go
Length of output: 136
Script:
#!/bin/bash
# Let's examine the actual usage patterns in these files
echo "=== server/internal/app/config.go ==="
rg "JWTProviders|AuthMiddleware.*JWT" server/internal/app/config.go -B 2 -A 2
echo -e "\n=== server/internal/app/m2m.go ==="
rg "JWTProviders|AuthMiddleware.*JWT" server/internal/app/m2m.go -B 2 -A 2
echo -e "\n=== server/internal/app/app.go ==="
rg "JWTProviders|AuthMiddleware.*JWT" server/internal/app/app.go -B 2 -A 2
Length of output: 1109
server/internal/infrastructure/mongo/asset.go (2)
64-68: LGTM! Clean implementation of FindByUUID method.
The implementation:
- Follows the established pattern similar to FindByID
- Leverages the new UUID index for efficient queries
- Maintains proper access control through findOne
28-28: Verify data consistency for the new unique UUID index.
Adding a unique index on the "uuid" field could fail if there are any duplicate UUIDs in existing asset documents.
Consider adding a data migration script if not already present to:
- Ensure all assets have valid UUIDs
- Verify uniqueness of existing UUIDs
- Handle any duplicate cases before applying the unique index
✅ Verification successful
Let me analyze the MongoDB index creation patterns and related code to better understand the implementation and potential data consistency concerns.
UUID index addition is safe and properly implemented
The verification shows that:
- The UUID field is already being used extensively throughout the codebase with proper validation
- New UUIDs are generated using
github.com/google/uuidpackage, ensuring uniqueness - The system already handles UUID-based asset lookups and storage paths
- The UUID field is already a core part of the asset model and is assigned during asset creation
The addition of a unique index on the UUID field is a proper formalization of an existing constraint, not a new data requirement that would need migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential duplicate UUIDs in the assets collection
ast-grep --pattern 'collection.createIndex({ uuid: 1 }, { unique: true })'
# Search for any migration scripts
fd -g "*migration*.go" --exec rg -l "asset.*uuid"
Length of output: 128
Script:
#!/bin/bash
# Let's check for MongoDB index creation patterns in a more Go-specific way
ast-grep --pattern 'CreateIndex($$$)'
# Check for any existing index definitions and collection setup
rg -A 5 "assetUniqueIndexes"
# Look for any migration or initialization code related to assets
rg -l "asset.*collection|collection.*asset" --type go
# Check for UUID-related code in asset files
rg "uuid" --type go
Length of output: 16435
server/internal/app/repo.go (2)
85-89: Verify context propagation in S3 initialization.
The S3 initialization uses context while GCS doesn't. Ensure this difference is intentional and that context handling is consistent across the codebase.
#!/bin/bash
# Check context usage in storage implementations
echo "Checking context usage in GCS implementation:"
rg "func.*New.*File.*context" server/internal/infrastructure/gcp/
echo "Checking context usage in S3 implementation:"
rg "func.*New.*File.*context" server/internal/infrastructure/aws/72-72: Validate host configuration for private base URL.
The privateBase is initialized with conf.Host without validation. This could lead to incorrect URL formation if the host is not properly configured.
server/internal/infrastructure/mongo/mongodoc/asset.go (2)
85-85: LGTM: Proper initialization of the Public field.
The field is correctly initialized using the domain model's getter method.
113-114: LGTM: Verify bidirectional conversion integrity.
The builder pattern is correctly maintained and the Public field is properly included in the domain model conversion.
Let's verify the bidirectional conversion integrity:
✅ Verification successful
Based on the verification results, I can now provide the final response:
Bidirectional conversion of Public field is correctly implemented
The verification confirms that:
- In
NewAsset: The Public field is properly set in the AssetDocument struct - In
Model: The Public field is correctly chained in the builder pattern (Public(d.Public)) - The field maintains consistency in both conversion directions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of the Public field in both Asset and AssetDocument
# Test 1: Verify the Public field is consistently used in both Model() and NewAsset()
ast-grep --pattern 'func NewAsset(a *asset.Asset) (*AssetDocument, string) {
$$$
Public: a.Public(),
$$$
}'
ast-grep --pattern 'func (d *AssetDocument) Model() (*asset.Asset, error) {
$$$
Public(d.Public)
$$$
}'
# Test 2: Check for any tests that verify this conversion
rg -l "TestAssetDocument.*Public" --type go
Length of output: 273
Script:
#!/bin/bash
# Let's try a different approach to verify the bidirectional conversion
# Check the AssetDocument struct definition for Public field
rg "type AssetDocument struct" -A 15 --type go
# Check NewAsset function implementation
rg "func NewAsset" -A 10 --type go
# Check Model method implementation
rg "func.*Model.*Asset" -A 15 --type go
# Look for any test files related to asset document
fd "asset.*test.*\.go$"
Length of output: 15678
Script:
#!/bin/bash
# Let's check the specific implementations of NewAsset and Model methods, and any related tests
# Check the complete NewAsset implementation
rg "func NewAsset\(a \*asset\.Asset\)" -A 20 --type go
# Check the complete Model implementation
rg "func \(d \*AssetDocument\) Model\(\)" -A 20 --type go
# Check asset_test.go for Public field testing
rg "Public" server/internal/infrastructure/mongo/mongodoc/asset_test.go -B 2 -A 2
Length of output: 3406
Script:
#!/bin/bash
# Let's see the complete implementation of both methods to verify Public field handling
# Get more context for NewAsset implementation
rg "func NewAsset\(a \*asset\.Asset\)" -B 2 -A 30 server/internal/infrastructure/mongo/mongodoc/asset.go
# Get more context for Model implementation
rg "func \(d \*AssetDocument\) Model\(\)" -B 2 -A 30 server/internal/infrastructure/mongo/mongodoc/asset.go
# Check the asset builder pattern implementation
rg "type AssetBuilder" -A 10 --type go
Length of output: 1812
server/internal/infrastructure/fs/file.go (2)
17-17: LGTM: Clean struct modifications for ACL support.
The new struct fields and import additions effectively support the ACL functionality, with clear naming conventions.
Also applies to: 24-27
30-46: LGTM: Constructor properly handles public base URL initialization.
The changes maintain backward compatibility while adding support for ACL through the public field.
server/internal/app/config.go (1)
45-47: Verify security implications of public asset access.
The Asset_Public flag controls asset visibility. Please ensure:
- This setting is properly validated during repository initialization
- Access controls are still enforced for non-public assets
- The default value (false when omitted) maintains secure-by-default principle
Let's verify the usage and security implications:
✅ Verification successful
Asset visibility controls are properly implemented with secure defaults
Based on the code analysis:
-
The
Asset_Publicflag is properly validated during repository initialization:- Different storage backends (GCS, S3, local) correctly handle public/private access
- When
Asset_Publicis false, assets use private storage with ACL controls
-
Access controls for non-public assets are enforced:
- Individual assets have a
publicfield that is checked independently - Non-public assets require authentication and proper project access rights
- Access middleware validates tokens and permissions before serving assets
- Individual assets have a
-
The secure-by-default principle is maintained:
- Assets are private by default (omitting
Asset_Publicresults in private access) - Individual asset visibility requires explicit public flag setting
- Access control checks are performed at multiple layers (config, middleware, asset level)
- Assets are private by default (omitting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper validation of Asset_Public flag and default security settings
# Test 1: Check repository initialization code for proper handling
echo "Checking repository initialization code..."
rg -A 10 "Asset_Public" --type go
# Test 2: Check for any security-related tests
echo "Checking for security-related tests..."
rg -l "TestAsset.*Security|TestAsset.*Access" --type go
# Test 3: Check for environment variable usage in example/documentation
echo "Checking environment variable documentation..."
rg "REEARTH_CMS_ASSET_PUBLIC" .env.example
Length of output: 4588
Script:
#!/bin/bash
# Let's check for default values and access control implementations
# Check for default value initialization
echo "Checking default value handling..."
rg -A 5 "NewConfig|defaultConfig" --type go
# Check access control implementations in asset-related code
echo "Checking asset access control implementations..."
rg -B 2 -A 5 "func.*Asset.*" --type go | grep -A 7 "Public\|Access\|Security"
# Check for environment variable handling
echo "Checking environment variable handling..."
rg "REEARTH_CMS_ASSET" --type go
# Check for asset-related interfaces and their security implementations
echo "Checking asset interfaces..."
ast-grep --pattern 'type Asset interface {
$$$
}'
Length of output: 12318
server/internal/usecase/interactor/asset.go (1)
46-48: LGTM: Clean implementation of FindByUUID method
The implementation correctly delegates to the repository layer while maintaining consistent interface parameters.
server/internal/usecase/interactor/asset_test.go (3)
235-235: LGTM: Method name updated correctly.
The test case correctly uses the renamed method Decompress instead of DecompressByID.
1393-1396: LGTM: Proper nil handling added.
The implementation correctly handles the nil case by returning an empty string, improving the robustness of the test mock.
1406-1407: LGTM: Comprehensive test coverage.
The test assertions properly verify both nil and non-nil cases for the GetURL method.
server/pkg/integrationapi/types.gen.go (1)
373-373: LGTM! The Public field addition aligns with asset visibility requirements.
The boolean field is correctly defined as a non-optional field with proper JSON serialization tag, which maintains backward compatibility while enabling asset visibility control.
Let's verify the usage of this field in the codebase:
✅ Verification successful
The Public field addition is correctly integrated with project publication controls
The field is properly used within the codebase:
- It's controlled via the project's publication settings (
project.Publication.AssetPublic()) - Proper access controls are implemented in the public API layer
- The field is consistently handled across MongoDB persistence and GraphQL interfaces
- Test coverage exists for the asset visibility functionality
The implementation maintains backward compatibility while enabling granular asset visibility control through project publication settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of the Asset.Public field
# Test: Search for Asset struct usage and Public field references
rg -A 5 'type\s+Asset\s+struct' --type go
rg 'Asset.*Public.*bool' --type go
Length of output: 4488
Script:
#!/bin/bash
# Let's check the implementation details of Asset.Public() and related methods
ast-grep --pattern 'func ($a *Asset) Public() bool {
$$$
}'
ast-grep --pattern 'func ($a *Asset) UpdatePublic(public bool) {
$$$
}'
# Check for any direct field access
rg -B 2 -A 2 '\.Public\s*=' --type go
# Check the publication implementation
ast-grep --pattern 'func ($p *Publication) AssetPublic() bool {
$$$
}'
Length of output: 794
Script:
#!/bin/bash
# Let's check the project publication and asset visibility related code
rg -A 5 'type\s+Publication\s+struct' --type go
# Check for any asset visibility related configuration or settings
rg -B 2 -A 2 'AssetPublic' --type go --type yaml --type json
# Check for any tests related to asset visibility
rg -A 5 'TestAsset.*Public|TestPublic.*Asset' --type go
Length of output: 16597
server/schemas/integration.yml (1)
1995-1995: Verify handling of existing assets with the new required field.
The addition of the required 'public' boolean field to the asset schema looks good. However, since it's marked as required, we need to ensure that existing assets are handled properly.
Run this script to check for potential breaking changes:
Also applies to: 2037-2038
✅ Verification successful
Let me analyze the code to verify the impact of the new required 'public' field on existing assets.
Let me check the asset builder implementation to see how the public field is handled.
No issues with the required 'public' field for existing assets
The codebase already handles the 'public' field appropriately:
- The asset builder (
server/pkg/asset/builder.go) initializes new assets withpublic = falseby default - The schema change only adds the field to the API schema, while the implementation already has this field
- Existing assets in the database will have the default value of
falsesince this was already the behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to the asset schema to ensure compatibility
# Test: Look for asset schema usage in models and handlers
rg -A 5 "type\s+Asset\s+struct"
rg -A 5 "asset\.Asset"
# Test: Look for asset creation/update logic that might need updating
rg -A 5 "CreateAsset|UpdateAsset"
Length of output: 187461
Script:
#!/bin/bash
# Check for existing asset creation/update logic to see how the public field is handled
ast-grep --pattern 'asset.New().$$$'
Length of output: 15603
Script:
#!/bin/bash
# Check the asset builder implementation
rg -A 10 "type Builder struct" server/pkg/asset/
Length of output: 518
server/internal/app/file.go (2)
15-17: Proper nil check for File Gateway
The function checks if cfg.Gateways.File is nil and returns early, preventing potential nil pointer dereferences.
57-65: Ensure proper error response when asset is not found
When cfg.Config.Asset_Public is false, the code retrieves the asset:
a, err := cfg.Repos.Asset.FindByUUID(ctx.Request().Context(), uuid)
if err != nil {
return err
}If the asset is not found (a == nil), the subsequent check if a != nil && !a.Public() will skip the block, and the code proceeds to read the asset file, which may result in an error.
Please verify that when the asset is not found (a == nil), an appropriate ErrNotFound error is returned to the client. Consider adding a check:
if a == nil {
return rerror.ErrNotFound
}This ensures that the client receives a clear response when the asset does not exist.
server/internal/infrastructure/gcp/file.go (7)
33-36: Addition of new fields in fileRepo struct is appropriate
The introduction of publicBase, privateBase, and public fields in the fileRepo struct effectively supports asset visibility management and is well-implemented.
39-59: Updates to NewFile function are correctly implemented
The NewFile function now properly initializes publicBase, sets a default value when publicBase is empty, and assigns public to true. The error handling for URL parsing is appropriate.
101-101: Improved error comparison using errors.Is
Replacing the direct comparison with errors.Is(err, iterator.Done) enhances error handling by correctly identifying wrapped or equivalent errors.
155-165: PublishAsset method is correctly implemented
The PublishAsset function appropriately manages asset publication by setting the object's ACL to grant public read access when applicable. Error handling is well-managed.
167-177: UnpublishAsset method is correctly implemented
The UnpublishAsset function properly handles asset unpublication by removing public read access from the object's ACL when necessary. Error handling is appropriate.
180-184: GetURL method adjustments ensure correct asset URL retrieval
The update to the GetURL method correctly determines the base URL based on the asset's visibility and the repository's configuration, ensuring accurate URL generation for assets.
327-353: publish method correctly manages ACL settings
The publish helper function accurately sets or deletes the object's ACL based on the public parameter. Error handling and logging are properly implemented to handle potential issues.
server/internal/infrastructure/aws/file.go (6)
36-37: Ensure Consistent Initialization of URL Fields
The addition of publicBase, privateBase, and public fields to the fileRepo struct appropriately sets up the differentiation between public and private URLs. Verify that these fields are correctly initialized in all constructor functions to prevent nil pointer dereferences.
Also applies to: 40-40
65-66: Verify Initialization of Public Repository Fields
In the NewFile function, publicBase is initialized with the parsed URL, privateBase is set to nil, and public is set to true. This setup correctly indicates a public repository. Ensure that any usage of privateBase accounts for the possibility of it being nil when the repository is public.
Also applies to: 69-69
154-159: Ensure Proper Error Handling in PublishAsset Method
The PublishAsset method correctly checks if the repository is public and returns ErrUnsupportedOperation if so. The subsequent call to f.publish with public set to true is appropriate for publishing assets.
161-166: Ensure Proper Error Handling in UnpublishAsset Method
Similarly, the UnpublishAsset method correctly handles the check for a public repository and calls f.publish with public set to false to unpublish assets.
305-305:
Handle Content Field Appropriately in Returned file.File
The Content field in the returned file.File struct is set to nil. Confirm that all consumers of this struct can handle a nil Content without leading to nil pointer dereferences.
Would you like me to search the codebase to identify any usages of file.File.Content that may not handle a nil value properly?
Also applies to: 310-310
169-173: Confirm Base URL Selection Logic in GetURL Method
The GetURL method now selects the base URL (publicBase or privateBase) based on the public flag and the asset's visibility. Ensure that privateBase is not nil when the asset is private to prevent runtime errors.
Run the following script to check that privateBase is properly initialized when needed:
✅ Verification successful
Let me gather more context about the initialization and usage of privateBase to ensure it's properly handled.
Let me check one more thing to understand the complete initialization flow and error handling.
Based on the gathered evidence, I can now provide the final response:
privateBase Initialization is Properly Handled
The code is safe as privateBase is explicitly initialized in the constructor flow and properly handled:
- It's declared as a field in the struct with a default
nilvalue - When needed for private assets, it's set via
fr.privateBase = uwith proper error checking - The base URL selection logic is correct and safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `privateBase` is initialized when `f.public` is false.
# Expected Result: Instances where `privateBase` is set when `public` is false.
rg 'fr\.privateBase\s*=\s*.*' -A2 server/internal/infrastructure/aws/file.go
Length of output: 133
Script:
#!/bin/bash
# Check the struct definition and constructor
ast-grep --pattern 'type fileRepository struct {
$$$
}'
# Check for any initialization of privateBase
rg -B3 -A3 'privateBase\s*=' server/internal/infrastructure/aws/file.go
# Check for any nil checks or error handling related to privateBase
rg 'privateBase.*nil' server/internal/infrastructure/aws/file.go
Length of output: 356
Script:
#!/bin/bash
# Get more context around the struct initialization and the GetURL method
rg -B5 -A10 'type fileRepo struct' server/internal/infrastructure/aws/file.go
# Check for any New/constructor function
ast-grep --pattern 'func New$_Repository($$$) {
$$$
}'
Length of output: 579
server/internal/adapter/integration/server.gen.go (9)
3771-3819: Ensure consistent parameter handling in AssetPublish and AssetUnpublish
The implementations of AssetPublish and AssetUnpublish in the strictHandler seem consistent with other methods. However, double-check that any required request body or additional parameters are correctly handled if necessary.
48-50: Addition of AssetPublish method to ServerInterface
The new AssetPublish method is correctly added to the ServerInterface interface with appropriate parameters and comments.
53-53: Addition of AssetUnpublish method to ServerInterface
The new AssetUnpublish method is correctly added to the ServerInterface interface with appropriate parameters.
289-305: Implementation of AssetPublish in ServerInterfaceWrapper
The AssetPublish method correctly binds the assetId parameter and handles errors consistently with other methods.
306-323: Implementation of AssetUnpublish in ServerInterfaceWrapper
The AssetUnpublish method correctly binds the assetId parameter and handles errors consistently with other methods.
325-355: Implementation of AssetContentGet in ServerInterfaceWrapper
The AssetContentGet method properly binds all path parameters (uuid1, uuid2, filename) and handles errors consistently.
1757-1835: Definition of request and response objects for AssetPublish and AssetUnpublish
The request and response objects for AssetPublish and AssetUnpublish are correctly defined, and the response handling follows the established patterns.
1837-1887: Definition of request and response objects for AssetContentGet
The request and response objects for AssetContentGet are properly defined, with correct handling of the binary response for asset content.
3479-3487: Addition of methods to StrictServerInterface
The methods AssetPublish, AssetUnpublish, and AssetContentGet are correctly added to the StrictServerInterface, matching the signatures in ServerInterface.
Summary by CodeRabbit
Release Notes
New Features
Publicadded to asset schemas to indicate asset visibility status.Bug Fixes
Documentation