feat(server): implement public api for assets #1277
Conversation
WalkthroughThe changes introduced in this pull request enhance the public API of the application by adding a new endpoint for retrieving project assets and modifying an existing asset retrieval endpoint. The test coverage for these endpoints has been improved, particularly in handling private project assets. Additionally, the API request handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant AssetRepo
Client->>API: GET /api/p/{project}/assets
API->>Controller: GetAssets(pKey, pagination)
Controller->>AssetRepo: FindByProject(pKey, filters)
AssetRepo-->>Controller: Return assets
Controller->>API: Return ListResult with assets
API-->>Client: Respond with JSON assets
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
server/internal/adapter/publicapi/api.go (3)
75-76: LGTM: Improved string handling, but consider error handling.The use of
mKeyandstrings.Cut()improves consistency and efficiency. However, consider handling the third return value ofstrings.Cut()to check if the cut was successful.You could improve error handling slightly:
- mKey, resType, _ = strings.Cut(mKey, ".") + var found bool + mKey, resType, found = strings.Cut(mKey, ".") + if !found { + // Handle the case where "." is not present in mKey + }
82-87: LGTM: Improved logic for handling assets and items.The introduction of the
resvariable and the conditional logic for assets vs. items aligns well with the PR objective. This change simplifies the code and makes it more consistent.Consider adding specific error handling for the
GetAssetscall:if mKey == "assets" { res, err = ctrl.GetAssets(ctx, pKey, p) + if err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, "Failed to get assets: "+err.Error()) + } } else { res, _, err = ctrl.GetItems(ctx, pKey, mKey, p) }
103-105: LGTM: Unified response handling, but consider removing duplication.The use of
resfor both JSON and default cases is consistent with earlier changes and simplifies response handling.Consider removing the duplication in the switch statement:
switch resType { case "csv": return toCSV(c, vi, s) case "geojson": return toGeoJSON(c, vi, s) -case "json": - return c.JSON(http.StatusOK, res) default: return c.JSON(http.StatusOK, res) }This change removes the redundant "json" case since it's identical to the default case.
server/internal/adapter/publicapi/types.go (1)
Ensure All Usages of
FlattenChildrenAre UpdatedThe analysis found that the
FlattenChildrenmethod is still used in the following files:
server/pkg/asset/file_builder_test.goserver/pkg/asset/file.goserver/internal/infrastructure/mongo/asset_file.goPlease review these instances to ensure they are updated to use the new
Files()method, maintaining consistency across the codebase.🔗 Analysis chain
Line range hint
180-184: Approve changes with suggestions for improvementThe modification to use
f.Files()and generate URLs for each file aligns well with the PR objective of implementing a public API for assets. This change provides more detailed information about each file in the asset, which is beneficial for API consumers.However, consider the following suggestions:
- Add error handling for URL generation to ensure robustness.
- Document this change in behavior for API consumers, as it might affect existing integrations.
- Consider performance implications for assets with a large number of files. You might want to implement pagination or limit the number of files returned if this could be an issue.
To assess the impact of this change, please run the following script:
This script will help identify if there are any large assets that might cause performance issues, if there are any remaining usages of the old
FlattenChildrenmethod, and if there's existing pagination logic for asset retrieval.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential large assets and usage of the old method # Check for potentially large assets echo "Checking for potentially large assets:" rg -i 'new.*asset.*file.*count.*>.*100' --type go # Check for any remaining usage of FlattenChildren echo "Checking for remaining usage of FlattenChildren:" rg 'FlattenChildren' --type go # Check for existing pagination in asset retrieval echo "Checking for existing pagination in asset retrieval:" rg -i 'asset.*pagination' --type goLength of output: 3169
server/internal/adapter/publicapi/asset.go (1)
42-77: Add unit tests for the newGetAssetsmethodTo ensure the new
GetAssetsmethod functions correctly, please add unit tests covering various scenarios, including:
- Projects with public and non-public assets.
- Cases where no assets are available.
- Pagination parameters and their effects.
This will help maintain the reliability and robustness of the public API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- server/e2e/publicapi_test.go (2 hunks)
- server/internal/adapter/publicapi/api.go (2 hunks)
- server/internal/adapter/publicapi/asset.go (2 hunks)
- server/internal/adapter/publicapi/types.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
server/internal/adapter/publicapi/api.go (2)
67-68: LGTM: Improved variable naming and reduced redundancy.The introduction of
mKeyandpKeyvariables enhances code readability and reduces redundancy. This change is a good practice for maintaining clean and understandable code.
92-94: LGTM: Improved error handling for versioned items.The addition of the
mKey != "assets"condition in the error check is consistent with the new asset handling logic. This ensures that errors are only returned for non-asset models when retrieving versioned items.server/e2e/publicapi_test.go (2)
206-224: Tests for the new assets endpoint are correctly implementedThe added test cases effectively verify the
GET /api/p/{project}/assetsendpoint and ensure the response structure and data are as expected.
437-437: Proper handling of asset file cloning before savingCloning the asset file before saving with
af.Clone()prevents unintended side effects, ensuring data integrity during tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
server/internal/infrastructure/memory/asset_file.go (1)
44-67: LGTM with optimization suggestionThe implementation of
FindByIDsis correct and consistent with the existingFindByIDmethod. It properly handles error checking, file retrieval, and associated file setting.For potential optimization, consider using a batch retrieval approach if the underlying data structure supports it. This could improve performance for large sets of IDs. For example:
func (r *AssetFile) FindByIDs(ctx context.Context, ids id.AssetIDList) (map[id.AssetID]*asset.File, error) { if r.err != nil { return nil, r.err } filesMap := make(map[id.AssetID]*asset.File) missingIDs := make([]id.AssetID, 0) // Batch retrieve all files r.data.Range(func(key id.AssetID, value *asset.File) bool { if ids.Has(key) { filesMap[key] = value.Clone() } return true }) // Check for missing files and set associated files for _, id := range ids { if f, ok := filesMap[id]; ok { fs := r.files.Find(func(key id.AssetID, value []*asset.File) bool { return key == id }) if len(fs) > 0 { f.SetFiles(fs) } } else { missingIDs = append(missingIDs, id) } } if len(missingIDs) > 0 { return nil, rerror.Errorf(rerror.ErrNotFound, "asset files not found for IDs: %v", missingIDs) } return filesMap, nil }This approach reduces the number of individual lookups and provides more detailed error information.
server/internal/adapter/publicapi/asset.go (1)
65-72: Approve implementation with minor suggestionsThe current implementation efficiently retrieves all asset files in a single call, which is good for performance. However, consider the following minor improvements:
- Error handling: Add a check for empty
fileMapto avoid potential issues if file retrieval fails silently.- Optimization: Consider using
asset.Files()instead offileMap[a.ID()]if the asset already contains file information.Here's a suggested implementation with these improvements:
fileMap, err := c.usecases.Asset.FindFilesByIDs(ctx, util.Map(al, func(a *asset.Asset) id.AssetID { return a.ID() }), nil) if err != nil { return ListResult[Asset]{}, err } if len(fileMap) == 0 { return ListResult[Asset]{}, errors.New("failed to retrieve asset files") } return NewListResult(util.Map(al, func(a *asset.Asset) Asset { file := a.Files() if file == nil { file = fileMap[a.ID()] } return NewAsset(a, file, c.assetUrlResolver) }), pi, p.Pagination), nilserver/internal/infrastructure/mongo/asset_file.go (1)
77-121: LGTM! TheFindByIDsmethod is well-implemented.The new
FindByIDsmethod effectively retrieves multiple asset files by their IDs. It maintains consistency with the existingFindByIDmethod while providing batch retrieval functionality. The implementation handles flat files and children appropriately.Consider improving error handling for specific cases.
On line 95, the error handling could be more specific. Instead of returning a general
ErrNotFoundwhen a file is nil, consider returning a more informative error that includes the specific asset ID that wasn't found.Here's a suggested improvement:
- if f == nil { - return nil, rerror.ErrNotFound - } + if f == nil { + return nil, fmt.Errorf("asset file not found for ID %s: %w", assetID, rerror.ErrNotFound) + }Add comments to improve code documentation.
Consider adding comments to explain the purpose and functionality of the
FindByIDsmethod. This will enhance code readability and maintainability.Here's a suggested comment to add at the beginning of the method:
// FindByIDs retrieves multiple asset files by their IDs. // It returns a map of asset IDs to asset files, handling both flat files and nested structures. // If any asset file is not found, it returns an error. func (r *AssetFile) FindByIDs(ctx context.Context, ids id.AssetIDList) (map[id.AssetID]*asset.File, error) { // ... (existing implementation) }server/internal/usecase/interactor/asset.go (1)
72-84: New function for batch retrieval of asset filesThe new
FindFilesByIDsfunction is a valuable addition that enables efficient batch retrieval of asset files. It follows the same pattern asFindFileByIDby first verifying the assets' existence before fetching the files.Consider the following optimization:
You could potentially optimize this function by combining the asset and file retrieval into a single database query, depending on your database structure and ORM capabilities. This would reduce the number of database round-trips. For example:
func (i *Asset) FindFilesByIDs(ctx context.Context, ids id.AssetIDList, _ *usecase.Operator) (map[id.AssetID]*asset.File, error) { - _, err := i.repos.Asset.FindByIDs(ctx, ids) - if err != nil { - return nil, err - } - - files, err := i.repos.AssetFile.FindByIDs(ctx, ids) + files, err := i.repos.AssetFile.FindByIDsWithAssetCheck(ctx, ids) if err != nil { return nil, err } return files, nil }This suggestion assumes you can implement a
FindByIDsWithAssetCheckmethod in yourAssetFilerepository that performs both the asset existence check and file retrieval in a single operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- server/internal/adapter/publicapi/asset.go (2 hunks)
- server/internal/infrastructure/memory/asset_file.go (1 hunks)
- server/internal/infrastructure/mongo/asset_file.go (1 hunks)
- server/internal/usecase/interactor/asset.go (1 hunks)
- server/internal/usecase/interfaces/asset.go (1 hunks)
- server/internal/usecase/repo/asset.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
server/internal/usecase/repo/asset.go (1)
28-28: LGTM! Consider verifying implementations.The new
FindByIDsmethod is a valuable addition to theAssetFileinterface. It allows for efficient batch retrieval of asset files, which can improve performance when multiple assets need to be fetched simultaneously.To ensure consistency across the codebase, please verify that this method has been implemented in all structs that implement the
AssetFileinterface. Run the following script to check for implementations:server/internal/adapter/publicapi/asset.go (2)
43-50:⚠️ Potential issueConsider returning
ErrForbiddeninstead ofErrNotFoundwhen access is deniedWhen the project is not public or assets are not publicly accessible, returning
rerror.ErrNotFoundmight be misleading to the API consumers. It's more appropriate to returnrerror.ErrForbiddento indicate that access is denied due to insufficient permissions.Apply this diff to return the correct error:
if prj.Publication().Scope() != project.PublicationScopePublic || !prj.Publication().AssetPublic() { - return ListResult[Asset]{}, rerror.ErrNotFound + return ListResult[Asset]{}, rerror.ErrForbidden }
58-63:⚠️ Potential issueReturn an empty list when no assets are found
Currently, if no assets are found, the method returns a
rerror.ErrNotFounderror. It's better to return an empty list to indicate that no assets are available, rather than reporting it as an error. This approach provides a smoother API experience for clients consuming the endpoint.Apply this diff to adjust the error handling:
if err != nil { - if errors.Is(err, rerror.ErrNotFound) { - return ListResult[Asset]{}, rerror.ErrNotFound - } return ListResult[Asset]{}, err } + if errors.Is(err, rerror.ErrNotFound) { + // No assets found, return empty list + res := []Asset{} + return NewListResult(res, nil, p.Pagination), nil + }server/internal/usecase/interfaces/asset.go (1)
64-64: Excellent addition of theFindFilesByIDsmethodThe new
FindFilesByIDsmethod is a valuable addition to theAssetinterface. It allows for efficient batch retrieval of asset files, which can significantly improve performance when multiple files need to be fetched. The method signature is consistent with other interface methods, and the use of a map for the return value provides an easy way to look up files by their asset IDs.This addition enhances the flexibility and efficiency of the asset management system, particularly for operations that need to handle multiple asset files simultaneously.
server/internal/usecase/interactor/asset.go (2)
Line range hint
52-65: Improved error handling in FindFileByIDThe changes to this function enhance its robustness by first verifying the asset's existence before attempting to retrieve the associated file. This approach prevents unnecessary database queries and provides clearer error handling.
Line range hint
1-84: Overall assessment of changesThe modifications to this file significantly enhance the asset management capabilities:
- Improved error handling in
FindFileByIDensures more robust single file retrieval.- The new
FindFilesByIDsfunction enables efficient batch retrieval of asset files, which aligns well with the PR objective of implementing a public API for assets.These changes provide a solid foundation for the new asset-related endpoints and improve the overall reliability of asset operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/e2e/publicapi_test.go (1)
381-381: Enhance readability by formatting chained method callsThe construction of the asset file on line 381 is quite lengthy and can be difficult to read. Consider splitting the chained method calls over multiple lines to improve readability.
Apply this diff to format the code:
-af := asset.NewFile().Name("aaa.zip").Path("aaa.zip").ContentType("application/zip").Size(1).Children([]*asset.File{asset.NewFile().Name("bbb.txt").Path("aaa/bbb.txt").Build()}).Build() +af := asset.NewFile(). + Name("aaa.zip"). + Path("aaa.zip"). + ContentType("application/zip"). + Size(1). + Children([]*asset.File{ + asset.NewFile(). + Name("bbb.txt"). + Path("aaa/bbb.txt"). + Build(), + }). + Build()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/e2e/publicapi_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
server/e2e/publicapi_test.go (2)
206-227: LGTMThe test case for the
GET /api/p/{project}/assetsendpoint is correctly implemented. The expected JSON response includes the newcontentTypeandfilesfields, which aligns with the enhancements made to the API.
234-237: LGTMThe test for the
GET /api/p/{project}/assets/{assetid}endpoint accurately checks the inclusion of the newcontentTypefield in the response. The implementation aligns with the modifications to the API specification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/publicapi/api.go (2)
62-80: LGTM: Function updated to handle both items and assets.The changes improve the function's flexibility by handling both items and assets, which aligns with the PR objectives. The use of a single
resvariable simplifies the code structure.Consider using a switch statement for better readability and extensibility:
- if m == "assets" { - res, err = ctrl.GetAsset(ctx, p, i) - } else { - res, err = ctrl.GetItem(ctx, p, m, i) - } + switch m { + case "assets": + res, err = ctrl.GetAsset(ctx, p, i) + default: + res, err = ctrl.GetItem(ctx, p, m, i) + }
Line range hint
1-109: Overall assessment: Changes align with PR objectives and improve code quality.The modifications to this file successfully implement the public API for assets while maintaining support for existing item-related functionality. The changes improve code clarity, flexibility, and align well with the PR objectives. Minor suggestions for further improvements have been provided in the individual function reviews.
As the public API grows, consider implementing a versioning strategy (e.g., /v1/api/...) to ensure backwards compatibility for future changes. This will allow for easier evolution of the API without breaking existing integrations.
server/e2e/publicapi_test.go (2)
206-228: LGTM! Consider adding more test cases for pagination.The new test case for the GET /api/p/{project}/assets endpoint looks good. It verifies the correct response structure, content, and pagination parameters.
To improve test coverage, consider adding test cases for:
- Pagination with different limit and offset values.
- Edge cases like empty result sets or last page scenarios.
383-384: LGTM! Consider adding a test case for nested file structure.The modifications to the asset file creation in the
publicAPISeederfunction align well with the changes in the API response structure. The addition of the "contentType" field and the nested "children" array for sub-files provides a more detailed representation of asset files.To further improve test coverage, consider adding a test case that specifically verifies the correct representation of the nested file structure in the API response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- server/e2e/publicapi_test.go (2 hunks)
- server/internal/adapter/publicapi/api.go (3 hunks)
- server/internal/adapter/publicapi/types.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/adapter/publicapi/types.go
🧰 Additional context used
🔇 Additional comments (3)
server/internal/adapter/publicapi/api.go (2)
36-36: LGTM: Route updated to use the new handler function.The route update aligns with the renaming of the handler function to
PublicApiItemOrAssetList(), which now handles both items and assets. This change is consistent with the PR objectives.
Line range hint
62-109: LGTM: Function updated to handle both items and assets with improved clarity.The changes significantly improve the function's flexibility and readability. The introduction of
mKeyandpKeyvariables, the conditional check for "assets", and the use of a singleresvariable all contribute to better code organization and align with the PR objectives.Consider the following minor improvements:
- Use a constant for the "assets" string to avoid magic strings:
const assetModelKey = "assets" // Then use it in the condition: if mKey == assetModelKey { // ... }
- Consider extracting the response type handling into a separate function for better modularity:
func handleResponseType(c echo.Context, resType string, vi interface{}, s interface{}, res interface{}) error { switch resType { case "csv": return toCSV(c, vi, s) case "geojson": return toGeoJSON(c, vi, s) case "json", "": return c.JSON(http.StatusOK, res) default: return c.JSON(http.StatusBadRequest, "Invalid response type") } }
- The error handling for
GetVersionedItemsseems to be redundant with the previous error check. Consider combining them or removing the redundant check.To ensure that the changes don't introduce any regressions, please run the following verification script:
This script will help identify any remaining references to the old function name that might have been missed during the refactoring.
✅ Verification successful
Verification Successful: All calls to
PublicApiItemOrAssetListhave been correctly updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to PublicApiItemOrAssetList are updated rg "PublicApiItemList" --type goLength of output: 32
Script:
#!/bin/bash # Verify that all calls to PublicApiItemOrAssetList are updated rg "PublicApiItemOrAssetList" --type goLength of output: 227
server/e2e/publicapi_test.go (1)
235-241: LGTM! Test case updated correctly for the modified asset retrieval endpoint.The test case for GET /api/p/{project}/assets/{assetid} has been appropriately updated to include the new "contentType" field in the expected response. This change aligns well with the modifications made to the API response structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/pkg/asset/list.go (1)
32-37: Consider adding nil check for better robustness.While the current implementation works, it might be worth adding explicit nil checks to prevent potential panics if the list contains nil assets.
func (l List) IDs() (ids id.AssetIDList) { for _, a := range l { + if a == nil { + continue + } ids = ids.Add(a.ID()) } return }server/pkg/asset/list_test.go (1)
56-63: Consider adding edge cases to strengthen test coverageThe test effectively validates the happy path for the
IDs()method. To make it more robust, consider adding test cases for:
- Empty list
- Nil list
- List with duplicate assets
Example addition:
func TestList_IDs(t *testing.T) { pid := NewProjectID() uid := accountdomain.NewUserID() a1 := New().NewID().Project(pid).CreatedByUser(uid).Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() a2 := New().NewID().Project(pid).CreatedByUser(uid).Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() + t.Run("should return IDs for valid list", func(t *testing.T) { al := List{a1, a2} assert.Equal(t, al.IDs(), id.AssetIDList{a1.ID(), a2.ID()}) + }) + + t.Run("should handle empty list", func(t *testing.T) { + al := List{} + assert.Equal(t, al.IDs(), id.AssetIDList{}) + }) + + t.Run("should handle nil list", func(t *testing.T) { + var al List + assert.Equal(t, al.IDs(), id.AssetIDList{}) + }) + + t.Run("should handle duplicates", func(t *testing.T) { + al := List{a1, a1, a2} + assert.Equal(t, al.IDs(), id.AssetIDList{a1.ID(), a1.ID(), a2.ID()}) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- server/internal/adapter/publicapi/asset.go (2 hunks)
- server/pkg/asset/list.go (2 hunks)
- server/pkg/asset/list_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/adapter/publicapi/asset.go
🧰 Additional context used
🔇 Additional comments (3)
server/pkg/asset/list.go (2)
32-37: LGTM! Clean implementation of the IDs collector.The implementation is straightforward and efficient, properly utilizing the
id.AssetIDListtype.
32-37: Verify usage in the public API context.Since this method was added as part of the public API implementation, let's verify its integration.
✅ Verification successful
Let me try a broader search to understand the usage of this method across the codebase.
This will help us understand:
- Where this method is being used across the codebase
- The context of its usage to determine if it's part of the public API or internal implementation
Based on the search results, I can now provide a final response:
The
IDs()method is properly integrated and used across the codebaseThe method is being used in several important contexts:
- Public API: Used in
server/internal/adapter/publicapi/asset.gofor finding files by asset IDs- Internal use cases: Utilized in request and item interactors for various operations
- Infrastructure layer: Used in memory implementation for request filtering
- Tests: Properly tested in
list_test.goThe implementation is particularly well-integrated in the public API context through
server/internal/adapter/publicapi/asset.go, where it's used to fetch files by asset IDs, which aligns with its intended purpose.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the IDs method is used in the public API implementation # Expected: Should find usage in handlers/endpoints related to asset retrieval # Search for references to the IDs method in the public API related files rg -l "\.IDs\(\)" server/internal/app/publicapiLength of output: 119
Script:
#!/bin/bash # Search for references to the IDs method across the entire codebase rg -g '!{vendor,third_party}/**' -l "\.IDs\(\)" . # Get more context around the usage rg -g '!{vendor,third_party}/**' -B 2 -A 2 "\.IDs\(\)" .Length of output: 5175
server/pkg/asset/list_test.go (1)
6-6: LGTM: Import addition is appropriateThe added import for the
idpackage is necessary for the new test function and properly organized.
Overview
This PR introduces the implementation of a public API for assets.
What I have done
GET /api/p/{project}/assets.Summary by CodeRabbit
Summary by CodeRabbit
New Features
GET /api/p/{project}/assets.Bug Fixes
contentTypefield for improved data accuracy.Refactor