feat(server): Import or Export Project Functionality (File resource) #1151
Conversation
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe changes introduce significant modifications to the project's GraphQL API and related functionalities, focusing on the export and import of project data. The Changes
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
|
hexaforce
force-pushed
the
feat/project-import-export-file-resource
branch
2 times, most recently
from
2824545
to
11fed7c
Compare
hexaforce
requested review from
airslice,
mkumbobeaty and
pyshx
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range and nitpick comments (8)
server/e2e/gql_import_export_test.go (1)
49-53: Note: Potential future implementation.The commented-out section suggests a potential future implementation where the response body is downloaded and processed. As mentioned in the AI-generated summary, this functionality is not currently active.
Consider removing the commented-out code if it's not planned for implementation in the near future, or add a TODO comment to clarify the intended functionality and timeline.
server/internal/usecase/interactor/plugin.go (2)
Line range hint
112-162: Simplify plugin import by batching repository operationsIn the
ImportPluginsfunction, each plugin is being saved individually within the loop, which can be inefficient due to multiple database operations. Consider collecting all non-system plugins and saving them in a batch after the loop to improve performance.Refactor the code as follows:
var pluginsToSave []*plugin.Plugin for _, pluginJSON := range pluginsJSON { // ... [existing code] ... if !p.ID().System() { - if err := i.pluginRepo.Save(ctx, p); err != nil { - return nil, err - } + pluginsToSave = append(pluginsToSave, p) } } + // Save all plugins in a batch + if len(pluginsToSave) > 0 { + if err := i.pluginRepo.SaveAll(ctx, pluginsToSave); err != nil { + return nil, err + } + }This change reduces the number of database calls and can improve the performance of the import operation.
62-107: Handle errors more explicitly in theExportPluginsfunctionWhile the current error handling passes the error up the call stack, adding context to the errors can make debugging easier. Use
fmt.Errorfor wrap errors to provide more detailed messages.For example:
plgs, err := i.pluginRepo.FindByIDs(ctx, filteredPluginIDs) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to find plugins by IDs: %w", err) } // Similarly for other errors within the function zipEntry, err := zipWriter.Create(zipEntryPath) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create zip entry '%s': %w", zipEntryPath, err) }This provides more context if an error occurs, aiding in troubleshooting.
server/internal/usecase/interactor/project.go (1)
622-625: Re-fetching the project may be unnecessaryAfter importing and saving the project, fetching it again using
FindByIDmight be redundant if there are no additional changes or side effects that need to be captured. Consider returning the project object directly to improve performance.server/internal/usecase/interactor/nlslayer.go (2)
Line range hint
846-868: Potential conflict when creating layers with existing IDsThe function creates new
NLSLayerinstances using IDs parsed from the input JSON without checking if those IDs already exist in the repository. This could lead to overwriting existing layers or unexpected behavior if anNLSLayerwith the same ID already exists. Consider adding a check to determine if a layer with the same ID exists before creating a new instance or handle the possibility of updating existing layers.
879-883: Unnecessary repository query after saving layersAfter saving the new
NLSLayerinstances, the function queries the repository to fetch them again using their IDs. Since the layers are already available in thenlayersslice, this may introduce unnecessary overhead. Consider returning thenlayersdirectly unless there's a specific reason to refetch from the repository.server/internal/usecase/interactor/scene.go (2)
862-878: Usedeferto ensurestreamis closed properlyCurrently,
stream.Close()is called manually afterio.Copy, and in one error case. Usingdeferguarantees thatstream.Close()is called, even if an error occurs duringio.Copy.Modify the code to use
defer:stream, err := i.file.ReadAsset(ctx, fileName) if err != nil { return err } + defer stream.Close() zipEntryPath := fmt.Sprintf("assets/%s", fileName) zipEntry, err := zipWriter.Create(zipEntryPath) if err != nil { return err } _, err = io.Copy(zipEntry, stream) if err != nil { return err } - if err := stream.Close(); err != nil { - return err - }
826-829: Redundant scene fetching after importAfter importing the scene, you are fetching it again from the repository at lines 826-829. Since you already have the
sceneobject, this may not be necessary unless you need to refresh its state.Consider removing the redundant fetch to optimize performance:
- scene, err = i.sceneRepo.FindByID(ctx, sceneID) - if err != nil { - return nil, err - }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (23)
- server/e2e/gql_import_export_test.go (2 hunks)
- server/gql/project.graphql (1 hunks)
- server/internal/adapter/gql/generated.go (8 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (1 hunks)
- server/internal/adapter/gql/resolver_mutation_project.go (4 hunks)
- server/internal/app/file.go (1 hunks)
- server/internal/infrastructure/fs/common.go (1 hunks)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/infrastructure/gcs/file.go (4 hunks)
- server/internal/infrastructure/s3/s3.go (4 hunks)
- server/internal/usecase/gateway/file.go (2 hunks)
- server/internal/usecase/interactor/asset.go (3 hunks)
- server/internal/usecase/interactor/nlslayer.go (3 hunks)
- server/internal/usecase/interactor/plugin.go (3 hunks)
- server/internal/usecase/interactor/project.go (3 hunks)
- server/internal/usecase/interactor/scene.go (7 hunks)
- server/internal/usecase/interactor/storytelling.go (1 hunks)
- server/internal/usecase/interactor/style.go (3 hunks)
- server/internal/usecase/interfaces/asset.go (2 hunks)
- server/internal/usecase/interfaces/plugin.go (2 hunks)
- server/internal/usecase/interfaces/project.go (2 hunks)
- server/internal/usecase/interfaces/scene.go (2 hunks)
- web/src/services/gql/gen/graphql.ts (5 hunks)
Additional context used
Learnings (1)
server/e2e/gql_import_export_test.go (2)
Learnt from: hexaforce PR: reearth/reearth-visualizer#1141 File: server/e2e/gql_import_export_test.go:48-80 Timestamp: 2024-09-13T01:19:11.267Z Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.Learnt from: hexaforce PR: reearth/reearth-visualizer#1141 File: server/e2e/gql_import_export_test.go:12-44 Timestamp: 2024-09-13T01:19:05.869Z Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
Additional comments not posted (35)
server/internal/adapter/gql/generated.go (8)
342-342: LGTM!The field name change from
ProjectDatatoProjectDataPathin theExportProjectPayloadstruct is consistent with the PR objectives and the AI-generated summary. The field type remains unchanged, indicating that the complexity calculation logic for the field is unaffected.
2591-2596: LGTM!The changes in the complexity calculation logic align with the field name change from
ProjectDatatoProjectDataPathin theExportProjectPayloadstruct. The complexity calculation for the field remains unchanged.
9614-9614: LGTM!The field name change from
projectDatatoprojectDataPathand the field type change fromJSON!toString!in theExportProjectPayloadtype are consistent with the PR objectives and the AI-generated summary.
19093-19094: LGTM!The addition of the new resolver function
_ExportProjectPayload_projectDataPathfor theprojectDataPathfield in theExportProjectPayloadtype is consistent with the field name change. The function follows the same pattern as other resolver functions in the file.
19107-19107: LGTM!The resolver function
_ExportProjectPayload_projectDataPathreturns the value ofobj.ProjectDataPath, which is consistent with the field name change in theExportProjectPayloadstruct.
19119-19121: LGTM!The changes in the resolver function
_ExportProjectPayload_projectDataPath, including casting the resolved value to a string, assigning it tofc.Result, and marshaling it using themarshalNString2stringfunction, are consistent with the field type change fromJSON!toString!in theExportProjectPayloadtype.The addition of the new field context function
fieldContext_ExportProjectPayload_projectDataPathfor theprojectDataPathfield is consistent with the field name change. The function correctly returns an error if the field has child fields, as the field type isString.Also applies to: 19124-19131
31822-31823: LGTM!The changes in the switch statement, including the removal of the case for
"projectData"and the addition of the case for"projectDataPath", are consistent with the field name change in theExportProjectPayloadtype. The new case correctly calls thefieldContext_ExportProjectPayload_projectDataPathfunction to retrieve the field context for theprojectDataPathfield.
65749-65750: LGTM!The changes in the switch statement, including the removal of the case for
"projectData"and the addition of the case for"projectDataPath", are consistent with the field name change in theExportProjectPayloadtype. The new case correctly calls the_ExportProjectPayload_projectDataPathfunction to retrieve the value of theprojectDataPathfield.web/src/services/gql/__gen__/graphql.ts (5)
559-566: LGTM!The new
ExportProjectInputandExportProjectPayloadtypes are well-defined and follow the GraphQL schema design principles. They enable the project export functionality as described in the PR objectives.
621-628: LGTM!The new
ImportProjectInputandImportProjectPayloadtypes are well-defined and follow the GraphQL schema design principles. They enable the project import functionality as described in the PR objectives.
1040-1044: LGTM!The new
exportProjectandimportProjectmutation fields are correctly added to theMutationtype. They expose the project export and import functionalities through the GraphQL API, with the return types matching the respective payload types.
1288-1290: LGTM!The new
MutationExportProjectArgstype correctly defines the input arguments for theexportProjectmutation. It expects anExportProjectInputobject as input and follows the GraphQL schema design principles.
1308-1310: LGTM!The new
MutationImportProjectArgstype correctly defines the input arguments for theimportProjectmutation. It expects anImportProjectInputobject as input and follows the GraphQL schema design principles.server/internal/infrastructure/fs/common.go (1)
8-8: LGTM! The addition ofexportDirconstant enhances the file system structure.The new constant
exportDirwith the value"export"is a straightforward addition that does not alter existing logic or functionality. It represents a directory path for export-related functionalities, suggesting an enhancement in the file system structure for better organization and management of exported resources.This change is unlikely to have any negative impact on the existing codebase as it is a new addition and does not modify existing code.
server/internal/usecase/interfaces/plugin.go (1)
25-25: LGTM!The
ExportPluginsmethod signature aligns with the PR objective of exporting plugins associated with a given scene. The parameters and return values are appropriate for the intended functionality.server/internal/usecase/interfaces/asset.go (2)
4-4: LGTM!The import of the
archive/zippackage is valid and necessary for the changes introduced in this file.
39-39: Looks good! This addition enhances the Asset interface.The new
UploadAssetFilemethod is a valuable addition to theAssetinterface, as it enables the uploading of asset files, which is crucial for the import/export functionality being introduced in this PR.The method signature and return types are appropriate:
- The
context.Contextparameter allows for request-scoped values and cancellation.- The
stringparameter likely represents the file name or path.- The
*zip.Fileparameter indicates that the method will handle operations related to zip file handling, aligning with the goal of using ZIP files for project data import/export.- The
*url.URLreturn type suggests that the method will return a URL pointing to the uploaded asset, providing a way to access the uploaded file.- The
int64return type likely represents the file size, which can be useful metadata.- The
errorreturn type allows for proper error handling.Overall, this change expands the capabilities of the
Assetinterface, enabling more complex asset management operations and supporting the import/export functionality being introduced in this PR.server/e2e/gql_import_export_test.go (2)
29-29: LGTM!The change in the GraphQL mutation query aligns with the shift in the expected response structure, as mentioned in the AI-generated summary. The test now focuses on validating the path to the exported project data rather than the data itself.
Line range hint
35-47: LGTM!The change in the test's response handling aligns with the shift in the expected response structure, as mentioned in the AI-generated summary. The test now extracts a string value from
projectDataPathinstead of an object fromprojectData.Note: As per the learnings, some assertions in the test functions are disabled due to problems with GraphQL. Keep this in mind when reviewing the test results.
server/internal/usecase/gateway/file.go (3)
8-8: LGTM!The import statement for the
ospackage is correctly placed and suggests that the code might involve file or directory related operations.
40-40: LGTM!The
ReadExportProjectZipmethod is a good addition to theFileinterface. It expands the interface's capabilities to support reading of project export ZIP files. The method signature is clear, follows Go conventions, and appropriately usescontext.Contextfor request-scoped data.
41-42: LGTM!The
UploadExportProjectZipandRemoveExportProjectZipmethods are good additions to theFileinterface. They expand the interface's capabilities to support uploading and removing of project export ZIP files. The method signatures are clear, follow Go conventions, and appropriately usecontext.Contextfor request-scoped data. Using*os.Fileas a parameter inUploadExportProjectZipis a good choice as it allows access to file metadata if needed.server/internal/app/file.go (1)
49-61: LGTM!The new route handler for exporting project files looks good. It follows the standard pattern of using a
fileHandlerfunction to stream the file response. The error handling is done correctly by returningErrNotFoundif the file is not found. The removal of the file after reading is a good practice to manage the lifecycle of exported files and ensure they are downloaded only once. The content type of the response is determined based on the file extension, which is a common approach.Overall, the code segment is well-structured, follows best practices, and enhances the server's capabilities by allowing clients to download exported project files.
server/internal/usecase/interfaces/scene.go (2)
4-4: LGTM!The
archive/zippackage import is necessary to support the newExportScenemethod.
35-35: Approve the addition ofExportScenemethod to theSceneinterface.The method signature is well-defined and follows the existing convention in the interface. The addition of this method enhances the functionality of the
Sceneinterface by enabling the exportation of scene data.Please ensure that the implementation of this method is thoroughly reviewed to verify correctness, error handling, and performance.
server/gql/project.graphql (1)
119-119: Verify the impact on clients and provide necessary documentation.The change from
projectDatatoprojectDataPathaligns with the PR objective of exporting project data as a ZIP file. However, this change may introduce breaking changes for clients that expect the project data directly in the response.Please ensure that:
- All clients consuming this GraphQL API are updated to handle the new response format and retrieve the project data from the provided file path.
- Necessary documentation or migration guides are provided to assist clients in adapting to this change.
server/internal/usecase/interactor/asset.go (1)
130-152: LGTM! The newUploadAssetFilefunction is a valuable addition.The
UploadAssetFilefunction enhances the functionality of theAssetstruct by enabling the upload of files directly from a zip archive. This streamlines asset management processes by allowing users to upload assets in bulk.The function is well-implemented with proper error handling and resource management. It follows the existing coding conventions and integrates seamlessly with the rest of the codebase.
Some key points:
- It handles errors when opening the zip file entry.
- It defers the closing of the read stream to ensure proper resource management.
- It detects the content type using the
http.DetectContentTypefunction.- It constructs a
file.Fileobject with the necessary properties.- It calls the
UploadAssetmethod and returns the result.Great job on this addition!
server/internal/usecase/interactor/style.go (1)
Line range hint
214-244: LGTM! The changes improve the reliability of the imported styles.The introduction of the
styleIDsvariable to collect style IDs during the import process and the subsequent retrieval of the saved styles by their IDs usingi.styleRepo.FindByIDsis a good approach.This change ensures that the function returns the styles that were actually saved to the repository, rather than the initially created
styleList. This allows for additional processing or validation of the saved styles if needed in the future.The altered control flow, which now includes the additional step of fetching the saved styles, improves the reliability of the function's output.
server/internal/infrastructure/fs/file.go (1)
128-130: LGTM!The
ReadExportProjectZipmethod implementation looks good:
- It constructs the file path correctly by joining the
exportDirand sanitizedfilename.- It uses the
readhelper method to handle file reading and error checking.- The
filenameparameter is properly sanitized usingsanitize.Pathto prevent path traversal attacks.server/internal/infrastructure/s3/s3.go (2)
32-32: LGTM!The constant
exportBasePathis appropriately named and defined for handling export-related files.
217-223: LGTM!The
ReadExportProjectZipmethod is implemented correctly:
- It sanitizes the input to prevent potential security issues.
- It handles the case when the
nameis empty by returning an appropriate error.- It constructs the correct file path using the
exportBasePathconstant.- It calls the
readmethod with the correct file path to read the ZIP file.server/internal/infrastructure/gcs/file.go (1)
208-214: LGTM!The
ReadExportProjectZipfunction is implemented correctly:
- It sanitizes the input
nameparameter to prevent potential path traversal attacks.- It handles the case when the sanitized name is empty by returning a not found error.
- It constructs the correct path for the export project ZIP file.
- It delegates the actual reading of the file to the
readhelper function.server/internal/usecase/interactor/storytelling.go (1)
1127-1130: Excellent improvement to theImportStoryfunction!Retrieving the story from the repository after saving it is a great way to ensure data integrity. This change enhances the reliability of the function by:
Verifying that the story was successfully persisted and can be fetched from the database accurately. It protects against silent or partial failures during the save operation.
Returning the retrieved story object instead of the original ensures that any database generated fields (like timestamps) are populated correctly in the response.
Failing early if the retrieval doesn't succeed, indicating a problem with the save operation or the repository itself.
Overall, this makes the function more robust and fail-safe. Great work!
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
566-566: ModifyExportProjectPayloadto return project data file path instead of JSON data.The
ProjectDatafield in theExportProjectPayloadstruct has been changed fromJSONtype tostringtype and renamed toProjectDataPath. This suggests that instead of directly including the project data in the GraphQL response, the response will now contain a file path or URL pointing to the location of the exported project data file (likely in ZIP format).This change will require updates to the client-side code to handle downloading and extracting the project data from the file, rather than directly accessing it from the GraphQL response. The server-side implementation will need to ensure that the project data is correctly exported to a file and the
ProjectDataPathis set to a valid and accessible location.server/internal/usecase/interactor/plugin.go (1)
157-163: Confirm that all imported plugins are properly retrievedAfter importing plugins, you fetch them using
FindByIDs. Ensure that all plugins are successfully retrieved and handle cases where some plugins might not be found.Run the following script to confirm that all plugin IDs retrieved match the ones intended to import:
This script assumes you have access to commands that can list the plugin IDs from the repository. Adjust it accordingly to fit your environment.
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (1)
server/pkg/scene/builder/decoder.go (1)
153-154: Remove commented-out code to enhance code cleanlinessThe commented line
// Links(flinks).appears to be obsolete or unused.Removing such commented-out code improves readability and reduces confusion for future maintainers.
Apply this diff to remove the commented code:
field := property.NewField(*fieldID). Value(ov). - // Links(flinks). Build()
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- server/internal/adapter/gql/resolver_mutation_project.go (3 hunks)
- server/internal/usecase/interactor/scene.go (10 hunks)
- server/internal/usecase/interactor/style.go (3 hunks)
- server/internal/usecase/interfaces/scene.go (2 hunks)
- server/pkg/file/zip.go (2 hunks)
- server/pkg/scene/builder/decoder.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/internal/usecase/interactor/style.go
Additional comments not posted (7)
server/internal/usecase/interfaces/scene.go (2)
4-10: LGTM!The new imports
archive/zipandgithub.com/reearth/reearth/server/pkg/pluginhave been added correctly. This aligns with the PR objective of introducing ZIP file export functionality and indicates that the code will likely involve working with ZIP files and plugins.
36-37: Verify the method implementations.The new
ExportScenemethod and the updatedImportScenemethod signatures in theSceneinterface look good and align with the PR objectives.However, please ensure that:
- The actual implementations of these methods correctly handle the parameters and return values as per the updated signatures.
- The
ImportScenemethod implementations are updated to handle the new[]*plugin.Pluginparameter, as this is a breaking change.Run the following script to verify the method implementations:
Verification successful
Method implementations verified and aligned with interface.
The ExportScene and ImportScene method implementations have been found in
server/internal/usecase/interactor/scene.go. Both implementations align with the interface signatures we reviewed earlier:
ExportScene:
func (i *Scene) ExportScene(ctx context.Context, prj *project.Project, zipWriter *zip.Writer) (*scene.Scene, map[string]interface{}, error)ImportScene:
func (i *Scene) ImportScene(ctx context.Context, prj *project.Project, plgs []*plugin.Plugin, sceneData map[string]interface{}) (*scene.Scene, error)The ImportScene method has been correctly updated to include the new
[]*plugin.Pluginparameter, addressing the breaking change we noted earlier.While the signatures match the interface, I recommend a manual review of the implementation details to ensure correct handling of parameters and return values, especially for the new plugin parameter in ImportScene.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the ExportScene and ImportScene method implementations. # Test 1: Search for the ExportScene method implementation. # Expect: The implementation should exist and align with the interface signature. ast-grep --lang go --pattern $'func $_($_, $_, $_) ($_, $_, $_) { $$$ }' | rg --type go $'ExportScene' # Test 2: Search for the ImportScene method implementation. # Expect: The implementation should exist and align with the updated interface signature. ast-grep --lang go --pattern $'func $_($_, $_, $_, $_) ($_, $_) { $$$ }' | rg --type go $'ImportScene'Length of output: 205
Script:
#!/bin/bash # Description: Broader search for ExportScene and ImportScene implementations # Test 1: Search for any occurrences of ExportScene in Go files echo "Searching for ExportScene:" rg --type go "ExportScene" # Test 2: Search for any occurrences of ImportScene in Go files echo "Searching for ImportScene:" rg --type go "ImportScene" # Test 3: List Go files in the project echo "Listing Go files:" fd -e go # Test 4: Search for potential implementation files echo "Searching for potential implementation files:" fd -e go | rg "scene|usecase"Length of output: 35601
server/internal/adapter/gql/resolver_mutation_project.go (2)
131-138: ** Issue: Deleting the exported ZIP file before it can be downloaded**The deferred function removes the ZIP file immediately after
ExportProjectreturns. Since theProjectDataPathreturned points to this ZIP file, deleting it prematurely will prevent clients from accessing the file.To fix this issue, consider the following options:
Option 1: Remove the
os.Remove(zipFile.Name())call from the deferred function to allow the file to persist. Implement a separate cleanup mechanism, such as a scheduled task, to remove old export files after a certain period.Option 2: Move the file deletion logic to occur after the file has been successfully downloaded by the client.
247-257: ** Ensure transactions are properly rolled back on import errors**After importing the scene, NLS layers, styles, and storytelling data, if any of these operations fail, the transaction may still be committed, potentially leaving the database in an inconsistent state.
Modify the error handling to roll back the transaction if any import operation fails:
if err != nil { tx.End(ctx) // Rollback return nil, err } // Commit only if all operations succeed if err := tx.Commit(); err != nil { return nil, err }This ensures data integrity by preventing partial imports.
server/internal/usecase/interactor/scene.go (2)
598-716: LGTM!The
ExportScenefunction looks good and follows a clear logic to export a scene to a ZIP file. It retrieves all the necessary data, builds a JSON representation using a builder pattern, and adds asset files to the ZIP archive. Error handling is done consistently throughout the function.The introduction of this feature for exporting scenes as ZIP files could be useful for various purposes such as backups, sharing, or migration.
Line range hint
718-841: LGTM!The modifications to the
ImportScenefunction look good. The addition of theplgsparameter allows for including plugins during scene import, which enhances the functionality and flexibility. The parsing of the scene JSON, creation of scene entities, and saving of the imported scene are handled properly. Error handling is done consistently throughout the function.The changes don't seem to have any negative impact on existing functionalities.
server/pkg/scene/builder/decoder.go (1)
177-184: New functionparsePropertyOptionalValueenhances modularityThe addition of
parsePropertyOptionalValuecorrectly encapsulates the logic for parsing optional property values, improving the modularity of the code.
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
server/internal/adapter/gql/resolver_mutation_project.go (2)
Line range hint
224-270: Improve transaction management and error handlingThe current implementation doesn't roll back the transaction if errors occur during the import of scene, NLS layers, styles, or storytelling data. Additionally, type assertions are used without checks, which could lead to runtime panics.
- Improve transaction management:
tx, err := usecases(ctx).Project.BeginImportTransaction(ctx) if err != nil { return nil, err } defer tx.Rollback(ctx) // Ensure rollback on any error // Perform all import operations if err := tx.Commit(); err != nil { return nil, err }
- Use safer type assertions:
projectData, ok := jsonData["project"].(map[string]interface{}) if !ok { return nil, fmt.Errorf("invalid project data format") } pluginsData, ok := jsonData["plugins"].([]interface{}) if !ok { return nil, fmt.Errorf("invalid plugins data format") } // Similar checks for other data typesThese changes will improve the robustness and safety of the import process.
Line range hint
1-270: Summary: Solid implementation with areas for improvementThe export and import functionality for projects is well-implemented overall. However, there are several areas that could be improved to enhance security, robustness, and maintainability:
- Reconsider the immediate deletion of exported ZIP files to ensure successful downloads.
- Improve the file name replacement mechanism during import to prevent unintended replacements.
- Enhance transaction management to ensure proper rollback on errors during the import process.
- Implement safer type assertions to prevent potential runtime panics.
- Consider adding logging statements to facilitate debugging and monitoring.
Addressing these points will significantly improve the quality and reliability of the code.
server/internal/usecase/interactor/project.go (1)
500-532: LGTM! Consider enhancing error handling in the deferred closure.The refactored
ExportProjectfunction looks good. It now focuses on exporting project assets, specifically the project image, which aligns well with the PR objectives. The error handling is appropriate, and the use of a deferred closure for closing the stream is a good practice.Consider enhancing the error handling in the deferred closure. While printing the error is helpful for debugging, you might want to log it or handle it more robustly. Here's a suggestion:
defer func() { if cerr := stream.Close(); cerr != nil { - fmt.Printf("Error closing file: %v\n", cerr) + // Log the error or handle it more robustly + log.Printf("Error closing file: %v", cerr) } }()
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- server/internal/adapter/gql/resolver_mutation_project.go (3 hunks)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/infrastructure/s3/s3.go (4 hunks)
- server/internal/usecase/interactor/project.go (3 hunks)
- server/internal/usecase/interactor/scene.go (10 hunks)
- server/pkg/file/zip.go (2 hunks)
- server/pkg/scene/builder/decoder.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/internal/infrastructure/fs/file.go
- server/internal/infrastructure/s3/s3.go
- server/pkg/file/zip.go
- server/pkg/scene/builder/decoder.go
Additional comments not posted (4)
server/internal/adapter/gql/resolver_mutation_project.go (2)
139-176: LGTM: Well-structured export functionalityThe export functionality is well-implemented, with proper error handling and resource management. The use of deferred functions for closing writers and handling errors is a good practice.
Consider adding logging statements at key points in the export process to facilitate debugging and monitoring.
Line range hint
180-270: LGTM: Comprehensive import functionality with room for improvementThe import functionality is well-structured and handles various components of the project data. It appropriately processes assets, plugins, and other project-related data.
Consider implementing the suggested improvements for security, transaction management, and type safety to further enhance the robustness of the import process.
server/internal/usecase/interactor/scene.go (2)
45-48: LGTM: New fields added to the Scene structThe addition of
nlsLayerRepo,layerStyles,storytellingRepo, andtagRepoto theScenestruct is appropriate. These new fields will be used in theExportScenefunction to retrieve necessary data for the scene export.
618-618: Handle potential nil pointer dereferenceThe code assumes that
storyListalways contains at least one element. This could lead to a panic if the list is empty.Consider adding a nil check before accessing the first element:
- story := (*storyList)[0] + var story *storytelling.Story + if len(*storyList) > 0 { + story = (*storyList)[0] + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
server/internal/usecase/interactor/project.go (1)
501-533: LGTM! Improved project export functionality.The refactored
ExportProjectfunction now focuses on exporting the project image, which is a good separation of concerns. The error handling and resource management have been improved, particularly with the use ofdeferfor closing the stream.Consider improving error handling when closing the stream.
While using
deferto close the stream is good practice, the error from closing is only being printed. Consider wrapping the original error with the close error if it occurs.You could modify the deferred function like this:
defer func() { if cerr := stream.Close(); cerr != nil { - fmt.Printf("Error closing file: %v\n", cerr) + err = fmt.Errorf("error exporting project (close error: %v): %w", cerr, err) } }()This ensures that any error during stream closing is properly propagated and combined with any existing error.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/infrastructure/gcs/file.go (4 hunks)
- server/internal/infrastructure/s3/s3.go (3 hunks)
- server/internal/usecase/gateway/file.go (2 hunks)
- server/internal/usecase/interactor/project.go (4 hunks)
- server/internal/usecase/interfaces/project.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/internal/infrastructure/gcs/file.go
- server/internal/infrastructure/s3/s3.go
- server/internal/usecase/gateway/file.go
- server/internal/usecase/interfaces/project.go
Additional context used
GitHub Check: ci-server / ci-server-lint
server/internal/infrastructure/fs/file.go
[failure] 135-135:
SA9003: empty branch (staticcheck)
[failure] 134-134:
SA4006: this value ofokis never used (staticcheck)
Additional comments not posted (2)
server/internal/infrastructure/fs/file.go (2)
128-130: LGTM: Secure implementation of ReadExportProjectZipThe method correctly sanitizes the input filename using
sanitize.Path()to prevent path traversal attacks. It also leverages the existingreadmethod, which handles error cases appropriately.
142-144: LGTM: Secure implementation of RemoveExportProjectZipThe method correctly sanitizes the input filename using
sanitize.Path()to prevent path traversal attacks. It also leverages the existingdeletemethod, which handles error cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
server/internal/usecase/interactor/scene_test.go (2)
Line range hint
145-236: Enhance test coverage and improve test structureWhile the test comprehensively checks the output structure, it may not fully validate the impact of the new parameter added to the
ImportScenefunction. Additionally, the test structure could be improved for better readability and maintainability.Consider the following improvements:
- Split the test into multiple sub-tests using
t.Run()to test different scenarios, including the new parameter's impact.- Use a test helper function to reduce duplication in JSON processing and comparison.
- Consider using a library like
github.com/stretchr/testify/requirefor clearer assertions.- Add explicit checks for the impact of the new parameter, if applicable.
Example refactor:
func TestImportScene(t *testing.T) { // Common setup code... tests := []struct { name string additionalParam interface{} expectedJSON string }{ {"NilAdditionalParam", nil, `{...}`}, // Current test case {"NonNilAdditionalParam", &SomeStruct{}, `{...}`}, // New test case } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result, err := ifs.ImportScene(ctx, prj, tt.additionalParam, sceneData) require.NoError(t, err) require.NotNil(t, result) actualJSON := processResultToJSON(t, result) require.JSONEq(t, tt.expectedJSON, actualJSON) }) } } func processResultToJSON(t *testing.T, result *Scene) string { // Helper function to process result and return JSON string // Move the JSON processing logic here }This structure allows for easier addition of new test cases and improves overall test readability.
Line range hint
1-236: Summary of review findingsThe changes in this test file reflect an update to the
ImportScenefunction signature, adding a new parameter. While the test has been updated to accommodate this change, there are opportunities to enhance the test coverage and structure.Key points:
- The new parameter is currently passed as
nil, which may not fully test its impact.- The test structure could be improved for better readability and maintainability.
- Additional test cases could be added to ensure comprehensive coverage of the updated function.
Next steps:
- Implement the suggested improvements to the test structure, including splitting into sub-tests and using helper functions.
- Add test cases that pass non-nil values for the new parameter to ensure proper handling.
- Review the
ImportScenefunction implementation (not visible in this file) to ensure that the new parameter is being used correctly and that this test adequately covers its functionality.- Update the test documentation to reflect the changes in the
ImportScenefunction signature and purpose.These improvements will help ensure that the
ImportScenefunction is thoroughly tested and that future changes can be more easily accommodated in the test suite.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/usecase/interactor/scene_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/internal/infrastructure/fs/file.go
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
==========================================
- Coverage 15.18% 14.46% -0.73%
==========================================
Files 642 570 -72
Lines 63207 60753 -2454
Branches 792 634 -158
==========================================
- Hits 9600 8788 -812
+ Misses 53607 51965 -1642
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
server/internal/adapter/gql/resolver_mutation_project.go (4)
123-146: LGTM: Robust implementation of ZIP file creation and handling.The ExportProject function effectively creates a ZIP file with a unique name, handles file operations using afero, and properly manages resources with deferred closures. The removal of the ZIP file after saving to storage is a good practice to avoid cluttering the file system.
Consider adding error logging for the deferred operations to capture any issues during cleanup:
defer func() { if cerr := zipFile.Close(); cerr != nil { log.Printf("Error closing ZIP file: %v", cerr) if err == nil { err = cerr } } if cerr := os.Remove(zipFile.Name()); cerr != nil { log.Printf("Error removing ZIP file: %v", cerr) if err == nil { err = cerr } } }()This will help in identifying any issues that occur during the cleanup process.
148-176: LGTM: Well-structured project export process.The export process is logically organized, exporting the project, scene, and plugins sequentially. The use of separate usecase methods for each component maintains good separation of concerns. Error handling is appropriately implemented at each step.
Consider potential optimization:
If the project, scene, and plugin data are large, you might want to write them to the ZIP file as they are generated, rather than storing them in memory. This could be achieved by passing thezipWriterto the usecase methods directly.Example:
err := usecases(ctx).Project.ExportProject(ctx, pid, zipWriter, getOperator(ctx)) if err != nil { return nil, err } err = usecases(ctx).Scene.ExportScene(ctx, prj, zipWriter) if err != nil { return nil, err } // ... and so onThis approach could potentially reduce memory usage for large projects.
Line range hint
182-272: LGTM: Comprehensive project import process with good error handling.The ImportProject function effectively handles the extraction and processing of the ZIP file contents, including assets, plugins, and project data. The use of a transaction ensures data consistency across the import process.
Suggestions for improvement:
Consider adding more granular error handling for asset and plugin imports. Currently, if one asset or plugin fails to import, the entire process fails. You might want to collect errors and continue importing, then return a summary of successes and failures.
Enhance security by validating file types and sizes before importing assets and plugins. This can prevent potential security risks from malicious files.
The file name replacement using
bytes.Replacecould potentially cause issues if the file names appear in other contexts within the data. Consider using a more robust method for updating file references, such as parsing the JSON structure and updating specific fields.Example for point 1:
var assetErrors []error for fileName, file := range assets { // ... existing code ... if err != nil { assetErrors = append(assetErrors, fmt.Errorf("failed to import asset %s: %v", fileName, err)) continue } // ... rest of the code ... } if len(assetErrors) > 0 { // Handle or return the errors }[security]
215-217: LGTM: File name replacement process.The current implementation using
bytes.Replacefor updating file names is acceptable given the context that "file names are unique and will be replaced uniformly", as confirmed by the project maintainer.For future consideration, if the uniqueness of file names cannot be guaranteed or if more complex data structures are involved, a more robust approach could be:
- Parse the JSON data into a structured format.
- Traverse the structure, updating file names in specific fields where they are expected.
- Re-encode the updated structure back to JSON.
This would provide more control over where replacements occur and reduce the risk of unintended replacements.
Example:
var jsonData map[string]interface{} if err := json.Unmarshal(data, &jsonData); err != nil { return nil, err } updateFileNames(jsonData, changedFileName) updatedData, err := json.Marshal(jsonData) if err != nil { return nil, err }Where
updateFileNamesis a recursive function that updates file names in specific fields of the JSON structure.server/internal/usecase/interactor/project.go (2)
500-532: LGTM! Consider adding a comment explaining the function's new purpose.The refactored
ExportProjectfunction looks good. It now focuses on exporting the project image to the zip file, which is a clear and specific responsibility. The error handling and resource management are appropriate.Consider adding a brief comment at the beginning of the function to explain its new purpose, e.g.:
// ExportProject exports the project's image to the provided zip writer.
534-562: LGTM! Consider improving error handling in the deferred function.The new
UploadExportProjectZipfunction is well-implemented. It correctly handles creating the project.json file, closing the zip writer, and uploading the zip file. The use ofafero.Fileis a good practice for testability.In the deferred function that closes the zip file, consider returning the error instead of just printing it. This way, the caller can handle the error if needed:
defer func() { if err := zipFile.Close(); err != nil { - fmt.Println("Failed to close zip file:", err) + err = fmt.Errorf("failed to close zip file: %w", err) } }()
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- server/internal/adapter/gql/resolver_mutation_project.go (3 hunks)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/usecase/interactor/project.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/internal/infrastructure/fs/file.go
Additional context used
Learnings (1)
server/internal/adapter/gql/resolver_mutation_project.go (2)
Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/adapter/gql/resolver_mutation_project.go:215-217 Timestamp: 2024-09-25T01:09:25.962Z Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/adapter/gql/resolver_mutation_project.go:0-0 Timestamp: 2024-09-25T00:58:40.590Z Learning: In the project export functionality, the ZIP file is deleted after saving to storage.
Additional comments not posted (3)
server/internal/adapter/gql/resolver_mutation_project.go (2)
4-21: LGTM: New imports are appropriate for the implemented functionality.The added imports for ZIP handling, file system operations, and ULID generation are consistent with the new export and import functionalities implemented in this file.
Line range hint
1-272: Overall: Well-implemented export and import functionality with room for minor improvements.The changes in this file introduce robust export and import functionality for projects, including handling of assets, plugins, and various project components. The implementation demonstrates good error handling, use of transactions for data consistency, and separation of concerns through the use of usecase methods.
Key strengths:
- Unique file naming using ULID for exports
- Proper resource management with deferred closures
- Comprehensive import process covering all aspects of a project
Areas for potential improvement:
- Enhanced error logging in deferred operations
- Optimization of memory usage during export for large projects
- More granular error handling during import, especially for assets and plugins
- Additional security measures for validating imported files
- Consideration of a more robust file name replacement strategy for complex data structures
These suggestions aim to further enhance the reliability, efficiency, and security of the implementation. Overall, the code is well-structured and achieves its intended functionality effectively.
server/internal/usecase/interactor/project.go (1)
623-626: Consider adding a comment explaining the additional query.As mentioned in a previous review comment, the additional
FindByIDquery at the end of theImportProjectfunction might impact performance. However, if it's necessary to ensure the most up-to-date data, consider adding a comment explaining why this extra query is needed.Add a comment explaining the rationale for the additional query:
// Fetch the project again to ensure we have the most up-to-date data prj, err = i.projectRepo.FindByID(ctx, prj.ID()) if err != nil { return nil, nil, err }
hexaforce
force-pushed
the
feat/project-import-export-file-resource
branch
from
3c253c6
to
6887490
Compare
hexaforce
force-pushed
the
feat/project-import-export-file-resource
branch
from
6887490
to
ed93458
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (6)
server/internal/adapter/gql/generated.go (1)
19119-19131: LGTM: Result handling and field context updated for projectDataPathThe changes correctly handle the new String type for
projectDataPath:
- The result is now treated as a string instead of JSON.
- The marshaling function has been updated to
marshalNString2string.- A new field context function for
projectDataPathhas been added with appropriate error handling for a String field.These changes are consistent with the field renaming and type change.
Consider updating the error message in the child function to be more specific:
- return nil, errors.New("field of type String does not have child fields") + return nil, errors.New("field 'projectDataPath' of type String does not have child fields")server/e2e/gql_import_export_test.go (2)
Line range hint
1-53: Summary of changes and recommendationsThe changes in this file align with the PR objectives of modifying the export functionality to return a download path for a ZIP file. However, there are several areas that need attention:
- The test doesn't fully verify the exported data by downloading and inspecting the ZIP file contents.
- There are no assertions on the returned
projectDataPath.- The import test functionality has been removed without explanation.
To improve the robustness of this test:
- Implement the download and verification of the ZIP file contents.
- Add assertions for the
projectDataPath.- Either reinstate the import test (if the functionality still exists) or update the PR description to explain its removal.
These changes will ensure that the test comprehensively covers the new export functionality and maintains consistency with the overall system functionality.
Removal of
TestCallImportProjectLeaves Import Functionality UntestedThe
TestCallImportProjectfunction has been removed, but import-related functionalities still exist in the codebase. This removal results in untested import features, potentially leading to undetected issues.
- Import functions found in:
server/internal/usecase/interactor/project_test.goserver/internal/usecase/interactor/scene.goserver/internal/usecase/interactor/project.goserver/internal/adapter/gql/resolver_mutation_project.goserver/internal/adapter/gql/generated.goAnalysis chain
Line range hint
1-53: Removal ofTestCallImportProjectfunctionThe
TestCallImportProjectfunction has been completely removed from this file. This is a significant change that isn't explicitly mentioned in the PR objectives.Could you please clarify the reason for removing the import test? If the import functionality is still part of the system, it should be tested. If it has been removed or significantly changed, the PR description should be updated to reflect this.
To verify the status of the import functionality, you can run the following script:
This will help determine if the import functionality still exists in the codebase and whether it needs testing.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for import-related functions in the codebase echo "Searching for import-related functions:" rg --type go 'func.*Import.*Project' echo "\nSearching for import-related GraphQL mutations:" rg --type go 'mutation.*Import.*Project'Length of output: 2598
server/internal/adapter/gql/resolver_mutation_project.go (1)
123-146: Improve error handling in deferred functionsThe error handling in the deferred functions is good, but it can be improved. Currently, if multiple errors occur in the deferred functions, only the last one will be returned. Consider using a helper function to combine multiple errors.
Here's a suggestion to improve error handling:
import "github.com/hashicorp/go-multierror" // At the beginning of the function var merr *multierror.Error // In deferred functions defer func() { if cerr := zipFile.Close(); cerr != nil { merr = multierror.Append(merr, cerr) } if cerr := os.Remove(zipFile.Name()); cerr != nil { merr = multierror.Append(merr, cerr) } }() // At the end of the function if merr.ErrorOrNil() != nil { return nil, merr.ErrorOrNil() }This approach will collect all errors that occur during cleanup.
server/internal/usecase/interactor/scene.go (2)
45-48: LGTM! Consider adding documentation for new fields.The new fields
nlsLayerRepo,layerStyles,storytellingRepo, andtagRepoare good additions to support the new export functionality.Consider adding brief comments to explain the purpose of these new fields, especially for developers who might not be familiar with the export process.
Line range hint
724-847: Improve error handling and consider refactoring for better organizationThe
ImportScenefunction has been updated to handle plugin imports, which is a good addition. However, there are a few areas that could be improved:
- Consider extracting the plugin processing logic (lines 734-741) into a separate function for better readability.
- Improve error handling by wrapping errors with more context.
- Consider using a builder pattern for constructing the
scene.Sceneobject to improve readability.Example of extracting plugin processing:
func (i *Scene) processPlugins(plgs []*plugin.Plugin) *scene.Plugins { plugins := scene.NewPlugins([]*scene.Plugin{ scene.NewPlugin(id.OfficialPluginID, nil), }) for _, plg := range plgs { if plg.ID().String() != "reearth" { plugins.Add(scene.NewPlugin(plg.ID(), nil)) } } return plugins }Then use it in
ImportScene:plugins := i.processPlugins(plgs)This refactoring will make the main function more readable and easier to maintain.
Also, consider wrapping errors with more context, for example:
if err := i.sceneRepo.Save(ctx, scene); err != nil { return nil, fmt.Errorf("saving imported scene: %w", err) }This will provide more informative error messages for debugging.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
Files selected for processing (25)
- server/e2e/gql_import_export_test.go (2 hunks)
- server/gql/project.graphql (1 hunks)
- server/internal/adapter/gql/generated.go (8 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (1 hunks)
- server/internal/adapter/gql/resolver_mutation_project.go (3 hunks)
- server/internal/app/file.go (1 hunks)
- server/internal/infrastructure/fs/common.go (1 hunks)
- server/internal/infrastructure/fs/file.go (1 hunks)
- server/internal/infrastructure/gcs/file.go (4 hunks)
- server/internal/infrastructure/s3/s3.go (3 hunks)
- server/internal/usecase/gateway/file.go (2 hunks)
- server/internal/usecase/interactor/asset.go (3 hunks)
- server/internal/usecase/interactor/nlslayer.go (3 hunks)
- server/internal/usecase/interactor/plugin.go (3 hunks)
- server/internal/usecase/interactor/project.go (4 hunks)
- server/internal/usecase/interactor/scene.go (10 hunks)
- server/internal/usecase/interactor/scene_test.go (3 hunks)
- server/internal/usecase/interactor/storytelling.go (1 hunks)
- server/internal/usecase/interactor/style.go (3 hunks)
- server/internal/usecase/interfaces/asset.go (2 hunks)
- server/internal/usecase/interfaces/plugin.go (2 hunks)
- server/internal/usecase/interfaces/project.go (2 hunks)
- server/internal/usecase/interfaces/scene.go (2 hunks)
- server/pkg/file/zip.go (2 hunks)
- server/pkg/scene/builder/decoder.go (3 hunks)
Files skipped from review as they are similar to previous changes (21)
- server/gql/project.graphql
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/app/file.go
- server/internal/infrastructure/fs/common.go
- server/internal/infrastructure/fs/file.go
- server/internal/infrastructure/gcs/file.go
- server/internal/infrastructure/s3/s3.go
- server/internal/usecase/gateway/file.go
- server/internal/usecase/interactor/asset.go
- server/internal/usecase/interactor/nlslayer.go
- server/internal/usecase/interactor/plugin.go
- server/internal/usecase/interactor/project.go
- server/internal/usecase/interactor/scene_test.go
- server/internal/usecase/interactor/storytelling.go
- server/internal/usecase/interactor/style.go
- server/internal/usecase/interfaces/asset.go
- server/internal/usecase/interfaces/plugin.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interfaces/scene.go
- server/pkg/file/zip.go
- server/pkg/scene/builder/decoder.go
Additional context used
Learnings (3)
server/e2e/gql_import_export_test.go (2)
Learnt from: hexaforce PR: reearth/reearth-visualizer#1141 File: server/e2e/gql_import_export_test.go:12-44 Timestamp: 2024-09-13T01:19:05.869Z Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.Learnt from: hexaforce PR: reearth/reearth-visualizer#1141 File: server/e2e/gql_import_export_test.go:48-80 Timestamp: 2024-09-13T01:19:11.267Z Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.server/internal/adapter/gql/resolver_mutation_project.go (3)
Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/adapter/gql/resolver_mutation_project.go:247-257 Timestamp: 2024-09-25T02:17:47.497Z Learning: The `tx.Commit()` method does not return an error. Transaction errors are handled via `tx.End(ctx)` in a deferred function.Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/adapter/gql/resolver_mutation_project.go:215-217 Timestamp: 2024-09-25T01:09:25.962Z Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/adapter/gql/resolver_mutation_project.go:0-0 Timestamp: 2024-09-25T00:58:40.590Z Learning: In the project export functionality, the ZIP file is deleted after saving to storage.server/internal/usecase/interactor/scene.go (1)
Learnt from: hexaforce PR: reearth/reearth-visualizer#1151 File: server/internal/usecase/interactor/scene.go:672-686 Timestamp: 2024-09-25T01:19:40.264Z Learning: In the `ExportScene` method, for NLS layers, `data["url"]` is of type `*url.URL`.
Additional comments not posted (12)
server/internal/adapter/gql/generated.go (7)
342-342: LGTM: Complexity function updated for ProjectDataPathThe change correctly updates the complexity function for the renamed field
ProjectDataPathin theExportProjectPayloadstruct. This is consistent with the schema changes and ensures proper complexity calculation for GraphQL queries.
2591-2596: LGTM: Complexity calculation updated for projectDataPathThe switch case for complexity calculation has been correctly updated to handle "ExportProjectPayload.projectDataPath". This change is consistent with the field renaming and maintains the existing complexity calculation logic.
19093-19094: LGTM: Resolver function updated for projectDataPathThe resolver function has been correctly renamed to
_ExportProjectPayload_projectDataPath. This change is consistent with the field renaming, and the function signature and error handling remain appropriate.
19107-19107: LGTM: Resolver middleware updated for ProjectDataPathThe resolver middleware has been correctly updated to return
obj.ProjectDataPath. This change is consistent with the field renaming and maintains the expected behavior of the resolver.
31822-31823: LGTM: Field resolution updated for projectDataPathThe switch statement for field resolution has been correctly updated to handle "projectDataPath". The field context function call is also properly updated to
fieldContext_ExportProjectPayload_projectDataPath. These changes are consistent with the field renaming.
65749-65750: LGTM: Field value assignment updated for projectDataPathThe field value assignment for
ExportProjectPayloadhas been correctly updated to handle "projectDataPath". The function call to_ExportProjectPayload_projectDataPathis properly updated to get the field value. These changes are consistent with the field renaming.
9614-9614: LGTM: Schema updated for projectDataPathThe GraphQL schema has been correctly updated to define
projectDataPathasString!. This change is consistent with the renaming and type change fromJSON!toString!.Please ensure that the client-side code is updated to handle the new
String!type forprojectDataPathinstead of the previousJSON!type. Run the following script to check for any remaining references to the old field name or type:Verification successful
Verified: No remaining references to
projectDataorJSON!foundThe search confirmed that there are no remaining references to
projectDataorJSON!in the codebase related toExportProjectPayload. The schema change has been successfully applied and integrated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'projectData' or 'JSON!' in relation to ExportProjectPayload rg --type typescript --type javascript 'ExportProjectPayload.*projectData|ExportProjectPayload.*JSON!'Length of output: 139
Script:
#!/bin/bash # Search for any remaining references to 'projectData' or 'JSON!' in relation to ExportProjectPayload in TypeScript and JavaScript files rg --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 'ExportProjectPayload.*projectData|ExportProjectPayload.*JSON!'Length of output: 126
server/e2e/gql_import_export_test.go (1)
29-29: GraphQL query updated to requestprojectDataPathThe change in the GraphQL query from requesting
projectDatatoprojectDataPathaligns with the PR objectives. This modification reflects the new functionality of returning a download path for the ZIP file instead of JSON data.server/internal/adapter/gql/resolver_mutation_project.go (4)
4-5: LGTM: New imports are appropriate for the changesThe new imports are relevant to the functionality added in this file. Good job on using the
github.com/spf13/aferopackage as requested in a previous comment.Also applies to: 8-11, 13-13, 16-16, 20-21
148-178: LGTM: ExportProject implementationThe implementation of the ExportProject function looks good. It correctly exports the project, scene, and plugins data into a ZIP file. The use of ULID for generating unique file names is a good practice.
Line range hint
182-272: LGTM: ImportProject implementationThe overall implementation of the ImportProject function looks good. It correctly handles the import process for various components (project, plugins, scene, NLS layers, styles, and storytelling data). The use of a transaction for database operations is a good practice to ensure data consistency.
Line range hint
1-272: Overall: Good implementation with minor improvements suggestedThe changes to the ExportProject and ImportProject functions are well-implemented. The code demonstrates good practices such as using ULID for unique file names, proper transaction management, and comprehensive error handling. A few minor improvements have been suggested:
- Enhance error handling in deferred functions to capture all potential errors.
- Improve the file name replacement mechanism in the ImportProject function for better security.
These changes will further improve the robustness and security of the implementation.
Overview
Changed from the previous JSON data-only output to downloading as a ZIP file that includes file resources.
What I've done
Contents of the ZIP file:
*project.json is the previous JSON file.
Add the export domain to the ZIP file storage.
*Files will be deleted simultaneously with the download.
Change the response from JSON to the download path of the ZIP file.
Added processing to save file resources.
File names will be updated.
Modified the response upon import to return the database data for verification.
Refactor part of the previous code.
In ExportProject(), data for Scenes and Plugins was being obtained, but I have now created ExportScene() and ExportPlugins() to separate the processing.
What I haven't done
The permission checks, such as access rights for each process, are not sufficient.
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes