feat(server): project import export external url #1205
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce a new context key, 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
|
✅ Deploy Preview for reearth-web canceled.
|
hexaforce
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/context.go (2)
53-55: Consider validating the currentHost parameter.
While the implementation follows the established pattern, consider adding validation to prevent empty host strings from being stored in the context.
func AttachCurrentHost(ctx context.Context, currentHost string) context.Context {
+ if currentHost == "" {
+ return ctx
+ }
return context.WithValue(ctx, contextCurrentHost, currentHost)
}131-138: Add documentation for the CurrentHost function.
Consider adding a doc comment to explain the function's purpose and return value semantics.
+// CurrentHost retrieves the current host from the context.
+// Returns an empty string if the host is not set or invalid.
func CurrentHost(ctx context.Context) string {server/internal/usecase/interactor/project.go (1)
620-620: Define host patterns as constants.
The host patterns are hardcoded in the condition. These should be defined as constants to improve maintainability.
Consider adding constants:
+const (
+ localhostAddr = "localhost:8080"
+ reearthDevSuffix = ".reearth.dev"
+ reearthIOSuffix = ".reearth.io"
+)
-if p.ImageURL.Host == "localhost:8080" || strings.HasSuffix(p.ImageURL.Host, ".reearth.dev") || strings.HasSuffix(p.ImageURL.Host, ".reearth.io") {
+if p.ImageURL.Host == localhostAddr || strings.HasSuffix(p.ImageURL.Host, reearthDevSuffix) || strings.HasSuffix(p.ImageURL.Host, reearthIOSuffix) {server/internal/usecase/interactor/nlslayer.go (1)
Line range hint 874-894: Consider refactoring property creation logic.
The property creation and import logic is duplicated between the main infobox and infobox blocks. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+func (i *NLSLayer) createAndImportProperty(ctx context.Context, schema *property.Schema, propertyJSON interface{}, sceneID id.SceneID, filter repo.SceneFilter) (*property.Property, error) {
+ prop, err := property.New().NewID().Schema(schema.ID()).Scene(sceneID).Build()
+ if err != nil {
+ return nil, err
+ }
+
+ prop, err = builder.AddItemFromPropertyJSON(ctx, prop, schema, propertyJSON)
+ if err != nil {
+ return nil, err
+ }
+
+ if err = i.propertyRepo.Filtered(filter).Save(ctx, prop); err != nil {
+ return nil, err
+ }
+
+ return prop, nil
+}Then use it in both places:
- schema := builtin.GetPropertySchema(builtin.PropertySchemaIDBetaInfobox)
- prop, err := property.New().NewID().Schema(schema.ID()).Scene(sceneID).Build()
- if err != nil {
- return nil, nil, err
- }
- prop, err = builder.AddItemFromPropertyJSON(ctx, prop, schema, nlsLayerJSON.Infobox.Property)
- if err != nil {
- return nil, nil, err
- }
- if err = i.propertyRepo.Filtered(writableFilter).Save(ctx, prop); err != nil {
- return nil, nil, err
- }
+ schema := builtin.GetPropertySchema(builtin.PropertySchemaIDBetaInfobox)
+ prop, err := i.createAndImportProperty(ctx, schema, nlsLayerJSON.Infobox.Property, sceneID, writableFilter)
+ if err != nil {
+ return nil, nil, err
+ }And similarly for the infobox blocks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- server/internal/adapter/context.go (3 hunks)
- server/internal/app/auth_client.go (1 hunks)
- server/internal/usecase/interactor/nlslayer.go (2 hunks)
- server/internal/usecase/interactor/project.go (2 hunks)
- server/internal/usecase/interactor/scene.go (7 hunks)
- server/internal/usecase/interactor/storytelling.go (3 hunks)
- server/pkg/scene/builder/decoder.go (5 hunks)
🔇 Additional comments (17)
server/internal/adapter/context.go (2)
17-22: LGTM! Constants are well-organized and follow conventions.
The new constant contextCurrentHost and its formatting align well with existing constants.
Line range hint 17-138: Verify the usage of CurrentHost in URL handling.
Let's ensure the new context functionality is properly integrated with URL handling across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the usage of CurrentHost is clear and well-integrated across the codebase.
CurrentHost context is properly integrated with URL handling
The context value is consistently used across the codebase for URL manipulation:
- In
scene/builder/decoder.go: Used to set host in URL values with proper HTTP/HTTPS scheme handling - In
usecase/interactor/project.go: Applied to project image URLs with scheme detection - In
usecase/interactor/scene.go: Handles URL schemes based on host detection - In
app/auth_client.go: Properly attached to context from configuration
The implementation follows a consistent pattern of:
- Getting host from context
- Stripping protocol prefixes
- Setting appropriate scheme based on host
- Applying host to URLs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how CurrentHost is used for URL handling
# Find all files that import this package
echo "Files importing this package:"
rg -l "github.com/reearth/reearth/server/internal/adapter"
# Find all usages of CurrentHost
echo -e "\nUsages of CurrentHost:"
rg "CurrentHost\(" -A 3
# Find URL-related code near CurrentHost usage
echo -e "\nURL handling near CurrentHost usage:"
rg -B 3 -A 3 "(?i)(url|path).*CurrentHost"
Length of output: 7377
server/internal/app/auth_client.go (2)
116-117: LGTM! Verify host configuration usage.
The addition of AttachCurrentHost is correctly placed after user/operator context attachments but before setting the request context. This ensures that the host information is available for subsequent URL processing in the import/export functionalities.
Let's verify the host configuration usage:
#!/bin/bash
# Description: Verify the host configuration usage across the codebase
# Expected: Find references to ensure the host is being used appropriately for URL processing
# Test 1: Check for AttachCurrentHost usage
echo "=== AttachCurrentHost Usage ==="
rg "AttachCurrentHost" -A 2
# Test 2: Check for CurrentHost retrieval
echo "=== CurrentHost Retrieval ==="
rg "CurrentHost\(" -A 2
# Test 3: Check for Config.Host usage to ensure consistent host values
echo "=== Config.Host Usage ==="
rg "Config\.Host" -A 2116-116: Consider validating host configuration.
While using cfg.Config.Host is secure, consider adding validation to ensure the host configuration is properly formatted and contains expected values (e.g., valid URL format, allowed domains) to prevent potential security issues in URL processing.
Let's check for existing host validation:
server/internal/usecase/interactor/nlslayer.go (2)
874-877: LGTM: Context propagation for property import.
The context is now correctly passed to AddItemFromPropertyJSON for the main infobox property, which is necessary for proper URL handling during import.
891-894: LGTM: Context propagation for infobox blocks.
The context is properly passed to AddItemFromPropertyJSON for each infobox block property, maintaining consistency with the URL handling requirements.
server/internal/usecase/interactor/storytelling.go (4)
1038-1041: LGTM: Block property import with context.
The context is correctly propagated to the block property import, maintaining proper error handling and repository interaction.
1067-1070: LGTM: Page property import with context.
The context is correctly propagated to the page property import, maintaining proper error handling and repository interaction.
1104-1107: LGTM: Story property import with context.
The context is correctly propagated to the story property import, maintaining proper error handling and repository interaction.
1038-1038: Context parameter properly propagated to property import calls.
The changes consistently add the context parameter to all AddItemFromPropertyJSON calls, which aligns with the PR's objective of enhancing context-aware operations for file import/export functionality.
Let's verify that all calls to AddItemFromPropertyJSON in the codebase have been updated with the context parameter:
Also applies to: 1067-1067, 1104-1104
✅ Verification successful
All AddItemFromPropertyJSON calls correctly use context parameter
The verification confirms that all calls to AddItemFromPropertyJSON across the codebase (scene.go, storytelling.go, and nlslayer.go) consistently use the context parameter. The function definition in builder/decoder.go also shows it's designed to accept the context parameter. No inconsistencies or missing context parameters were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining calls to AddItemFromPropertyJSON without context
# Expect: No matches without ctx parameter
# Search for function calls without ctx
ast-grep --pattern 'builder.AddItemFromPropertyJSON($x, $y, $z)'
# Search for function calls with ctx to confirm the pattern
ast-grep --pattern 'builder.AddItemFromPropertyJSON(ctx, $x, $y, $z)'
Length of output: 138
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all occurrences of AddItemFromPropertyJSON
rg "AddItemFromPropertyJSON" -A 2 -B 2
Length of output: 3133
server/pkg/scene/builder/decoder.go (3)
Line range hint 112-200: Consistent propagation of context.Context parameter
The addition of ctx context.Context to functions enhances context-aware operations. Verify that all functions that require context have been updated accordingly and that ctx is passed consistently throughout recursive calls.
Run the following script to ensure consistency:
#!/bin/bash
# Description: Verify that all relevant functions include the context parameter.
# Test: Search for function definitions and calls missing the `ctx` parameter.
# Expect: No functions related to property parsing should be missing `ctx`.
# Find function definitions without `ctx`
rg 'func\s+(parseProperty\w+)\s*\(' ./server/pkg/scene/builder -A 1 | grep -v 'ctx context.Context'
# Find function calls without `ctx`
rg '(parseProperty\w+)\s*\([^,]*\)' ./server/pkg/scene/builder | grep -v '(ctx,'198-199:
Replace fmt.Printf with proper logging for unreadable properties
Consistently handle error logging throughout the codebase by replacing fmt.Printf with the appropriate logging mechanism. This ensures that all logs are captured uniformly and can be managed or formatted as needed.
Apply this diff to update the logging:
- fmt.Printf("property is unreadable %v\n", value)
+ log.Printf("property is unreadable: %v\n", value)Likely invalid or redundant comment.
178-179: Handle potential errors from CurrentHost retrieval
Ensure that adapter.CurrentHost(ctx) does not return an empty string or an invalid URL. Add error handling to manage scenarios where the current host is unavailable or malformed, preventing potential runtime errors.
Run the following script to verify the behavior:
server/internal/usecase/interactor/scene.go (4)
13-13: LGTM!
The import statement for the adapter package is necessary for the changes introduced in this pull request.
647-653: Verify the impact of the convertReearthEnv function on existing URLs.
The introduction of the convertReearthEnv function modifies the scheme and host of URLs that match certain conditions. While the changes seem reasonable, it's crucial to ensure that this transformation doesn't break any existing functionality or integrations.
To verify the impact, consider the following:
-
Identify all the places where the modified URLs are used or stored, such as in the database, configuration files, or external services.
-
Retrieve a representative sample of existing URLs from these sources and test them against the
convertReearthEnvfunction to ensure they are transformed as expected. -
Verify that the modified URLs work correctly with the associated functionality, such as loading assets, making API calls, or interacting with external services.
-
If there are any third-party integrations or services that rely on the original URL format, reach out to the respective teams or vendors to discuss the impact of the URL changes and coordinate any necessary updates.
By thoroughly testing and validating the impact of the convertReearthEnv function, you can ensure a smooth transition and minimize the risk of introducing bugs or breaking changes.
731-745: LGTM!
The convertReearthEnv function looks good and handles the conversion of URLs based on the specified conditions. The use of the adapter.CurrentHost function to retrieve the current host from the context is a nice touch.
776-776: Verify the impact of passing the context to AddItemFromPropertyJSON.
The AddItemFromPropertyJSON function signature has been updated to include the ctx parameter. While this change allows for context-aware operations, it's important to assess the impact on existing code and ensure compatibility.
To verify the impact, consider the following:
-
Identify all the places where the
AddItemFromPropertyJSONfunction is called and ensure that thectxparameter is properly passed. -
Review the implementation of the
AddItemFromPropertyJSONfunction and any dependent functions to ensure they handle the context correctly and don't introduce any unintended side effects. -
Test the functionality related to property JSON processing thoroughly to ensure that the addition of the context doesn't break existing behavior.
-
If the
AddItemFromPropertyJSONfunction is part of a public API or used by external packages, consider the impact on backward compatibility and whether a version update or migration guide is necessary.
By carefully reviewing and testing the code that uses the updated AddItemFromPropertyJSON function, you can ensure a smooth integration of the context parameter and maintain the expected behavior of the system.
server/pkg/scene/builder/decoder.go
Outdated
| fmt.Printf("invalid url: %v\n", err.Error()) | ||
| return nil, false |
There was a problem hiding this comment.
Use proper logging instead of fmt.Printf for error messages
Using fmt.Printf for logging errors directly to standard output is not recommended, as it may not be thread-safe and does not provide log levels or structured logging capabilities. It's better to use the project's logging framework or the standard log package for consistent and thread-safe logging.
Apply this diff to replace fmt.Printf with proper logging:
- fmt.Printf("invalid url: %v\n", err.Error())
+ log.Printf("invalid url: %v\n", err)Committable suggestion was skipped due to low confidence.
| isReearth, u := convertReearthEnv(ctx, u) | ||
| if isReearth { | ||
| if err := i.addZipAsset(ctx, zipWriter, u.Path); err != nil { | ||
| return nil, nil, errors.New("Fail addZipAsset :" + err.Error()) | ||
| } |
There was a problem hiding this comment.
Ensure error handling for asset loading and ZIP creation.
While the code attempts to load assets and add them to the ZIP file, it's important to handle potential errors gracefully. Consider the following:
-
Check if the
addZipAssetfunction returns an error and handle it appropriately. Instead of returning the error directly, you may want to log the error and continue with the export process for other assets. -
Verify that the ZIP file is created successfully and that the assets are added correctly. You can check the integrity of the ZIP file and ensure that the expected assets are present.
-
Consider adding retry logic or fallback mechanisms in case of temporary failures during asset loading or ZIP creation. This can help improve the reliability of the export process.
-
Provide clear error messages or log entries to aid in troubleshooting if issues occur during the export process.
By implementing proper error handling and validation, you can make the export process more resilient and provide a better user experience.
| isReearth, u := convertReearthEnv(ctx, u) | ||
| if isReearth { | ||
| if err := i.addZipAsset(ctx, zipWriter, u.Path); err != nil { | ||
| return nil, nil, errors.New("Fail addZipAsset :" + err.Error()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating the asset loading and ZIP creation logic.
The code for loading assets and adding them to the ZIP file is repeated in multiple places (e.g., for NLS layers, widget button icons, and page block sources). To improve code maintainability and reusability, consider the following:
-
Extract the common logic for loading assets and adding them to the ZIP file into a separate function. This function can take the necessary parameters, such as the context, ZIP writer, and URL, and handle the asset loading and ZIP creation process.
-
Replace the repeated code blocks with calls to the extracted function, passing in the appropriate parameters.
-
Ensure that the extracted function handles errors appropriately and returns meaningful error messages or codes.
-
Update the error handling in the calling code to handle any errors returned by the extracted function.
By consolidating the asset loading and ZIP creation logic, you can reduce code duplication, improve readability, and make the code easier to maintain and update in the future.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/context.go (2)
53-55: Consider adding input validation
While the implementation is correct, consider validating the currentHost parameter to ensure it's not empty and follows a valid URL format. This would prevent potential issues with invalid host values being stored in the context.
func AttachCurrentHost(ctx context.Context, currentHost string) context.Context {
+ if currentHost == "" {
+ return ctx
+ }
+ // Optional: Add URL validation if needed
+ // if _, err := url.Parse(currentHost); err != nil {
+ // return ctx
+ // }
return context.WithValue(ctx, contextCurrentHost, currentHost)
}17-22: Consider the lifecycle of the host value
The current host is being stored in the request context. While this works, consider whether this value might be better managed through configuration or environment variables if it's relatively static for the application's lifecycle. This would depend on whether the host value needs to change per-request or remains constant.
server/internal/usecase/interactor/project.go (1)
622-623: Consolidate URL scheme trimming logic.
The consecutive string trimming operations could be consolidated into a single operation for better readability and maintainability.
Consider using a helper function:
-currentHost = strings.TrimPrefix(currentHost, "https://")
-currentHost = strings.TrimPrefix(currentHost, "http://")
+currentHost = trimURLScheme(currentHost)
+func trimURLScheme(host string) string {
+ return strings.TrimPrefix(strings.TrimPrefix(host, "https://"), "http://")
+}server/internal/usecase/interactor/storytelling.go (1)
Line range hint 1037-1105: Architectural improvement in ImportStory function
The consistent propagation of context throughout the import process enhances the function's ability to handle environment-specific configurations, such as external URLs and file paths, as intended by the PR objectives. This architectural change ensures that all property-related operations have access to the necessary context information.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- server/internal/adapter/context.go (3 hunks)
- server/internal/app/auth_client.go (1 hunks)
- server/internal/usecase/interactor/nlslayer.go (2 hunks)
- server/internal/usecase/interactor/project.go (2 hunks)
- server/internal/usecase/interactor/scene.go (7 hunks)
- server/internal/usecase/interactor/storytelling.go (3 hunks)
- server/pkg/scene/builder/decoder.go (5 hunks)
🔇 Additional comments (20)
server/internal/adapter/context.go (2)
22-22: LGTM: Context key follows established patterns
The new context key follows the existing naming conventions and type patterns.
131-138: LGTM: Verify usage in URL handling
The implementation correctly retrieves and type-asserts the host value from the context. The error handling follows the established pattern in the codebase.
Let's verify how this function is used for URL handling across the codebase:
✅ Verification successful
LGTM: CurrentHost is correctly used in URL handling
The function is properly integrated into the URL handling workflow across the codebase:
- Used in
scene/builder/decoder.go,interactor/scene.go, andinteractor/project.gowith consistent URL sanitization - Host value is attached via
AttachCurrentHostin auth client middleware - Consistent pattern of handling localhost vs production URLs
- Proper scheme selection based on host value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CurrentHost usage to ensure it's properly integrated with URL handling
rg -A 5 "CurrentHost\("
# Look for URL manipulation patterns near CurrentHost usage
ast-grep --pattern 'CurrentHost($_) {
$$$
url.$$$
$$$
}'
Length of output: 2774
server/internal/app/auth_client.go (1)
116-117: Consider adding host validation.
The addition of host information to the context is well-placed and aligns with the PR objectives for handling external URLs. However, consider adding validation to ensure the host value is properly configured.
Let's verify the host configuration:
server/internal/usecase/interactor/project.go (2)
621-621:
Add error handling for CurrentHost.
The adapter.CurrentHost(ctx) call should include error handling to ensure the function behaves predictably when the host cannot be determined.
Consider modifying the code as follows:
-currentHost := adapter.CurrentHost(ctx)
+currentHost, err := adapter.CurrentHost(ctx)
+if err != nil {
+ return nil, nil, fmt.Errorf("failed to determine current host: %w", err)
+}Likely invalid or redundant comment.
620-630: Verify URL modification impact.
The URL modification logic affects existing projects during import. Let's verify that this change doesn't impact any existing project imports.
server/internal/usecase/interactor/nlslayer.go (1)
874-877: LGTM! Context parameter properly added to AddItemFromPropertyJSON calls.
The context parameter has been correctly added to both instances of AddItemFromPropertyJSON, maintaining proper error handling within the transaction scope.
Let's verify that context is consistently propagated in other calls to AddItemFromPropertyJSON:
Also applies to: 891-894
✅ Verification successful
Context parameter is consistently used across all AddItemFromPropertyJSON calls
The verification shows that all calls to AddItemFromPropertyJSON across the codebase (in nlslayer.go, storytelling.go, and scene.go) correctly include the context parameter ctx as the first argument. The function is also properly defined in builder/decoder.go with the context parameter. No inconsistencies found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to AddItemFromPropertyJSON that might not include context
ast-grep --pattern 'AddItemFromPropertyJSON($$$)'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all occurrences of AddItemFromPropertyJSON
rg "AddItemFromPropertyJSON" -A 2 -B 2
Length of output: 3133
server/internal/usecase/interactor/storytelling.go (1)
1038-1038: Context propagation looks good!
The changes consistently propagate the context parameter to builder.AddItemFromPropertyJSON calls across different levels of the storytelling structure (blocks, pages, and stories). This aligns with the PR's objective of improving URL handling by making the operations context-aware.
Let's verify the builder package's implementation of the context parameter:
Also applies to: 1067-1067, 1104-1104
✅ Verification successful
Context parameter is properly implemented in the builder package
The verification confirms that AddItemFromPropertyJSON in server/pkg/scene/builder/decoder.go correctly accepts and utilizes the context parameter. The context is properly propagated to helper functions like parsePropertyValue (line 124) and parsePropertyOptionalValue (line 150), indicating proper context handling throughout the property building process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the builder package's implementation of AddItemFromPropertyJSON
# Test: Check if the builder package properly handles the context parameter
ast-grep --pattern 'func AddItemFromPropertyJSON(ctx context.Context, $_, $_, $_) ($_, $_)'
Length of output: 3687
server/pkg/scene/builder/decoder.go (7)
7-8: LGTM!
The addition of the net/url and strings imports is necessary for the URL parsing and manipulation functionality introduced in this change.
10-10: LGTM!
The import of github.com/reearth/reearth/server/internal/adapter is required to access the CurrentHost function for retrieving the current host from the context.
124-124: LGTM!
The parsePropertyValue function now accepts the ctx parameter to enable context-aware parsing of property values.
150-150: LGTM!
The parsePropertyOptionalValue function now accepts the ctx parameter and passes it to parsePropertyValue, ensuring consistent context handling during property parsing.
167-192: Verify the URL parsing and modification logic.
The changes introduce URL parsing and modification logic within the parsePropertyValue function. If the property type is "url", the function attempts to parse the value as a URL and modifies the host and scheme based on certain conditions.
To ensure the correctness and reliability of this logic, consider the following:
- Verify that the URL parsing logic handles various URL formats correctly, including those with and without schemes, ports, paths, and query parameters.
- Confirm that the host modification logic accurately identifies and updates the host based on the specified conditions (
localhost:8080,.reearth.dev, or.reearth.io). - Ensure that the scheme adjustment logic correctly sets the scheme to "http" for
localhost:8080and "https" for other hosts. - Validate that the modified URL is correctly reassembled and returned as a string.
- Consider adding unit tests to cover different URL scenarios and edge cases to ensure the robustness of the parsing and modification logic.
199-200: LGTM!
The parsePropertyOptionalValue function now accepts the ctx parameter and passes it to parsePropertyValue, ensuring consistent context handling during optional property parsing.
112-112: Verify the impact of the context parameter addition.
The AddItemFromPropertyJSON function now accepts a context.Context parameter. Ensure that all callers of this function have been updated to provide the appropriate context.
Run the following script to verify the function usage:
✅ Verification successful
All callers have been updated to provide the context parameter
The search results show that all callers of AddItemFromPropertyJSON are correctly passing the ctx context parameter. The function is called in the following locations, and all of them provide the context:
server/internal/usecase/interactor/storytelling.go(3 occurrences)server/internal/usecase/interactor/scene.go(2 occurrences)server/internal/usecase/interactor/nlslayer.go(2 occurrences)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all callers of `AddItemFromPropertyJSON` provide the context parameter.
# Test: Search for the function usage. Expect: All occurrences to have the context argument.
rg -A 5 $'AddItemFromPropertyJSON'
Length of output: 4135
server/internal/usecase/interactor/scene.go (6)
684-688: ****
The changes here are similar to the previous code segment (lines 647-653) and the same review comment applies.
714-718: ****
The changes here are similar to the previous code segments (lines 647-653 and lines 684-688) and the same review comment applies.
731-745: The convertReearthEnv function looks good!
The function correctly handles the conversion of URLs based on the current environment. It checks for specific host patterns and updates the URL scheme and host accordingly.
820-820: ****
The changes here are similar to the previous code segment (line 776) and the same review comment applies.
776-776: Verify if the AddItemFromPropertyJSON function handles the context parameter correctly.
The changes update the AddItemFromPropertyJSON function call to include the ctx parameter. This change aligns with the updated function signature in other files.
However, it's important to verify that the AddItemFromPropertyJSON function correctly utilizes the ctx parameter and that there are no issues or inconsistencies in how it's used across the codebase.
Run the following script to analyze the usage of AddItemFromPropertyJSON:
✅ Verification successful
Context parameter is correctly handled in AddItemFromPropertyJSON
The function AddItemFromPropertyJSON in server/pkg/scene/builder/decoder.go correctly accepts and propagates the context parameter. The function is consistently called with the context parameter across multiple files:
server/internal/usecase/interactor/scene.goserver/internal/usecase/interactor/storytelling.goserver/internal/usecase/interactor/nlslayer.go
All callers properly pass the context parameter, maintaining consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `AddItemFromPropertyJSON` function.
# Test: Search for the function definition and analyze its implementation.
ast-grep --pattern $'func AddItemFromPropertyJSON($_, $_, $_, $_) {
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Let's try a different approach to find the function definition and its usage
# Search for the function definition using ripgrep
echo "=== Function definition ==="
rg "func.*AddItemFromPropertyJSON" -A 10
# Search for function calls
echo -e "\n=== Function calls ==="
rg "AddItemFromPropertyJSON\(" -A 2
Length of output: 3174
647-653: Verify the impact of the URL conversion on the codebase.
The changes introduce a new function convertReearthEnv to handle the conversion of URLs based on the current environment. This change is used in multiple places within the ExportScene method.
While the changes look good, it's important to verify that the URL conversion doesn't introduce any unintended side effects or break existing functionality.
Run the following script to analyze the impact:
✅ Verification successful
Let me gather more information about the addZipAsset function and its usage since the previous search didn't show its implementation.
Based on the gathered information, I can now provide the final response:
URL conversion implementation is safe and properly scoped
The convertReearthEnv function has a clear and limited scope:
- It only modifies URLs from specific hosts (localhost:8080, *.reearth.dev, *.reearth.io)
- The conversion preserves the URL path which is the only part used by
addZipAsset - The function is consistently used across all URL processing in the scene export logic
- The
addZipAssetfunction safely extracts filenames from paths and doesn't depend on host/scheme
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of the URL conversion on the codebase.
# Test 1: Search for potential issues related to the `convertReearthEnv` function.
rg -A 5 $'convertReearthEnv' -g '*.go'
# Test 2: Verify if the `addZipAsset` function handles the converted URLs correctly.
ast-grep --pattern $'func addZipAsset($_, $_, $$$) {
$$$
}'
Length of output: 2225
Script:
#!/bin/bash
# Search for the implementation and usage of addZipAsset
rg -A 10 "func.*addZipAsset" -g '*.go'
# Search for any other URL conversions or manipulations in the scene package
rg -A 5 "url\.(URL|Parse)" server/internal/usecase/interactor/scene.go
# Look for the full implementation of convertReearthEnv
rg -A 15 "^func convertReearthEnv" server/internal/usecase/interactor/scene.go
Length of output: 2467
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
server/e2e/gql_import_export_test.go (1)
95-95: Remove debug print statement.
The fmt.Println statement should be removed or replaced with proper logging.
server/internal/usecase/interactor/plugin.go (1)
Line range hint 98-103: Consider enhancing error handling with defer
The current error handling approach for the file copy operation is correct but could be improved for better resource management.
Consider using a defer statement for the Close operation to ensure cleanup happens regardless of the copy operation's outcome:
- if _, err = io.Copy(zipEntry, stream); err != nil {
- _ = stream.Close()
- return nil, nil, err
- }
-
- if err := stream.Close(); err != nil {
- return nil, nil, err
- }
+ defer func() {
+ if cerr := stream.Close(); cerr != nil && err == nil {
+ err = fmt.Errorf("failed to close stream: %w", cerr)
+ }
+ }()
+
+ if _, err = io.Copy(zipEntry, stream); err != nil {
+ return nil, nil, fmt.Errorf("failed to copy plugin file: %w", err)
+ }This change:
- Uses
deferto ensure the stream is always closed - Preserves the original error if one occurs during copy
- Adds more descriptive error messages
- Follows Go's error handling best practices
server/internal/usecase/interactor/scene.go (1)
Line range hint 645-714: Consider consolidating URL and ZIP asset handling logic.
The code for handling URLs and adding assets to the ZIP file is repeated in three places (NLS layers, widget button icons, and page blocks). This repetition makes the code harder to maintain and more prone to inconsistencies.
Consider extracting this logic into a dedicated helper function:
func (i *Scene) processAndAddURLToZip(ctx context.Context, zipWriter *zip.Writer, u *url.URL) error {
if u == nil {
return nil
}
if isReearth, convertedURL := convertReearthEnv(ctx, u); isReearth {
if err := i.addZipAsset(ctx, zipWriter, convertedURL.Path); err != nil {
return fmt.Errorf("failed to add zip asset: %w", err)
}
}
return nil
}This would simplify the calling code and ensure consistent error handling across all URL processing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- server/e2e/gql_import_export_test.go (3 hunks)
- server/internal/usecase/interactor/plugin.go (1 hunks)
- server/internal/usecase/interactor/scene.go (7 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_import_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (4)
server/e2e/gql_import_export_test.go (1)
4-11: LGTM: Import changes align with new functionality.
The new imports support file operations and enhanced test assertions.
server/internal/usecase/interactor/plugin.go (1)
Line range hint 421-451: Verify URL handling compatibility with external URLs
Given that this PR adds support for external URLs, please verify if the ImporPluginFile method needs to be updated to handle external URL paths correctly. The current implementation assumes local file paths, which might need adaptation for external URL support.
Let's check for any URL-related changes in other files that might affect this implementation:
server/internal/usecase/interactor/scene.go (2)
711-714: Same improvements needed as above.
772-772: LGTM: Context parameter properly added.
The context parameter is correctly passed to AddItemFromPropertyJSON calls, improving context propagation.
Also applies to: 816-816
server/e2e/gql_import_export_test.go
Outdated
| func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { | ||
| requestBody := GraphQLRequest{ | ||
| OperationName: "CreateProject", | ||
| Query: `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { | ||
| createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { | ||
| project { | ||
| id | ||
| __typename | ||
| } | ||
| __typename | ||
| } | ||
| }`, | ||
| Variables: map[string]any{ | ||
| "name": name, | ||
| "description": "abc", | ||
| "imageUrl": "https://test.com/aaaaa.jpg", | ||
| "teamId": wID.String(), | ||
| "visualizer": "CESIUM", | ||
| "coreSupport": true, | ||
| }, | ||
| } | ||
|
|
||
| res := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithJSON(requestBody). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
|
|
||
| return res.Path("$.data.createProject.project.id").Raw().(string) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test data and error handling.
A few suggestions for improvement:
- Consider using a constant or test configuration for the image URL instead of hardcoding
- Add test cases for invalid image URLs
- Add error handling for failed requests
Example improvement:
- "imageUrl": "https://test.com/aaaaa.jpg",
+ "imageUrl": testConfig.ExternalImageURL, // Define in test configCommittable suggestion was skipped due to low confidence.
server/e2e/gql_import_export_test.go
Outdated
| downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| Body().Raw() | ||
|
|
||
| // downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", projectDataPath)). | ||
| // Expect(). | ||
| // Status(http.StatusOK). | ||
| // Body() | ||
| // fmt.Println(downloadResponse) | ||
| fileName := "project_data.zip" | ||
| err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) | ||
| assert.Nil(t, err) | ||
| return fileName |
There was a problem hiding this comment.
Several improvements needed in download handling.
- The hardcoded localhost URL should be replaced with a configurable base URL
- File permissions (os.ModePerm) are too permissive
- Consider adding cleanup for partial downloads on failure
Apply these improvements:
- downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)).
+ downloadResponse := e.GET(fmt.Sprintf("%s%s", config.ServerURL, downloadPath)).
Expect().
Status(http.StatusOK).
Body().Raw()
fileName := "project_data.zip"
- err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm)
+ err := os.WriteFile(fileName, []byte(downloadResponse), 0600)
assert.Nil(t, err)
+ if err != nil {
+ os.Remove(fileName) // Cleanup on failure
+ }
return fileName📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | |
| Expect(). | |
| Status(http.StatusOK). | |
| Body().Raw() | |
| // downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", projectDataPath)). | |
| // Expect(). | |
| // Status(http.StatusOK). | |
| // Body() | |
| // fmt.Println(downloadResponse) | |
| fileName := "project_data.zip" | |
| err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) | |
| assert.Nil(t, err) | |
| return fileName | |
| downloadResponse := e.GET(fmt.Sprintf("%s%s", config.ServerURL, downloadPath)). | |
| Expect(). | |
| Status(http.StatusOK). | |
| Body().Raw() | |
| fileName := "project_data.zip" | |
| err := os.WriteFile(fileName, []byte(downloadResponse), 0600) | |
| assert.Nil(t, err) | |
| if err != nil { | |
| os.Remove(fileName) // Cleanup on failure | |
| } | |
| return fileName |
server/e2e/gql_import_export_test.go
Outdated
| pID := createProjectWithExternalImage(e, "test") | ||
|
|
||
| _, _, sID := createScene(e, pID) | ||
|
|
||
| createStory(e, sID, "test", 0) | ||
|
|
||
| fileName := exporProject(t, e, pID) | ||
|
|
||
| defer func() { | ||
| err := os.Remove(fileName) | ||
| assert.Nil(t, err) | ||
| }() | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the examination of the test file, I can now provide the final response:
Test coverage needs improvement for export functionality
The current test only verifies that:
- A project with external image can be created
- The export endpoint returns a successful response
- A ZIP file is created and cleaned up
Missing critical verifications:
- The structure and content of the exported ZIP file
- Proper handling of external image URLs in the export
- Validation of scene and story data in the export
This is the only export-related test in the e2e suite, and it lacks comprehensive assertions for the exported content.
🔗 Analysis chain
Consider enhancing test assertions.
While the cleanup is good, consider adding assertions to verify:
- The exported ZIP file structure and content
- The handling of external image URLs in the exported project
Let's verify if similar tests exist:
Let me check the content of the test file to see if there are other export-related assertions.
Let me check if there are any import-related tests that might show how the exported content is verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other export-related test assertions
rg -l "TestCall.*Export" "server/e2e"
Length of output: 74
Script:
#!/bin/bash
# Check the content of the export test file
cat server/e2e/gql_import_export_test.go
Length of output: 2900
Script:
#!/bin/bash
# Search for import-related test assertions
rg -A 10 "import.*Project" server/e2e/gql_import_export_test.go
Length of output: 65
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
server/e2e/gql_import_export_test.go (2)
14-15: Enhance test setup documentation.
Consider improving the setup instructions to include all prerequisites and environment variables needed for running the tests.
-// export REEARTH_DB=mongodb://localhost
-// go test -v -run TestCallExportProject ./e2e/...
+// Test Prerequisites:
+// 1. Running MongoDB instance
+// 2. Required environment variables:
+// export REEARTH_DB=mongodb://localhost
+//
+// Run test:
+// go test -v -run TestCallExportProject ./e2e/...Line range hint 102-134: Address security and error handling concerns.
Several issues need attention:
- Remove debug print statement
- Use more restrictive file permissions
- Add proper error handling for HTTP requests
- Consider using temporary directory for test files
Apply these fixes:
func exporProject(t *testing.T, e *httpexpect.Expect, p string) string {
+ tmpDir := t.TempDir() // Use test's temporary directory
requestBody := GraphQLRequest{
OperationName: "ExportProject",
Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }",
Variables: map[string]any{
"projectId": p,
},
}
r := e.POST("/api/graphql").
WithHeader("Origin", "https://example.com").
WithHeader("authorization", "Bearer test").
WithHeader("X-Reearth-Debug-User", uID.String()).
WithHeader("Content-Type", "application/json").
WithJSON(requestBody).
Expect().
Status(http.StatusOK).
JSON().
Object()
- fmt.Println(r) // Remove debug print
+
+ // Check for GraphQL errors
+ errors := r.Value("errors").Array()
+ assert.Empty(t, errors.Raw(), "GraphQL errors should be empty")
downloadPath := r.
Value("data").Object().
Value("exportProject").Object().
Value("projectDataPath").String().Raw()
+ assert.NotEmpty(t, downloadPath, "Download path should not be empty")
downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)).
Expect().
Status(http.StatusOK).
Body().Raw()
- fileName := "project_data.zip"
+ fileName := filepath.Join(tmpDir, "project_data.zip")
- err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm)
+ err := os.WriteFile(fileName, []byte(downloadResponse), 0600)
assert.Nil(t, err)
return fileName
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- server/e2e/gql_import_export_test.go (3 hunks)
- server/internal/usecase/interactor/scene.go (8 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_import_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (1)
server/internal/usecase/interactor/scene.go (1)
773-773: LGTM: Context parameter correctly added.
The context parameter has been properly added to the AddItemFromPropertyJSON calls, improving context propagation throughout the application.
Also applies to: 817-817
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
server/internal/usecase/interactor/nlslayer.go (1)
Line range hint 861-986: Consider refactoring ImportNLSLayers for better maintainability.
The function is handling multiple concerns and is quite long. Consider breaking it down into smaller, focused functions:
- URL processing logic
- Property handling
- Layer building
Example refactoring:
func (i *NLSLayer) processConfigURL(ctx context.Context, config *nlslayer.Config) error {
// URL processing logic
}
func (i *NLSLayer) buildInfobox(ctx context.Context, sceneID idx.ID[id.Scene], infoboxJSON *builder.InfoboxJSON) (*nlslayer.Infobox, error) {
// Infobox building logic
}
func (i *NLSLayer) buildSketchInfo(sketchInfoJSON *builder.SketchInfoJSON) (*nlslayer.SketchInfo, error) {
// SketchInfo building logic
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- server/internal/usecase/interactor/nlslayer.go (5 hunks)
- server/pkg/scene/builder/decoder.go (5 hunks)
🔇 Additional comments (6)
server/pkg/scene/builder/decoder.go (4)
6-7: LGTM: Import statements are appropriate.
The new imports support the added URL parsing and string manipulation functionality.
Also applies to: 9-9, 14-14
112-112: LGTM: Context propagation is properly implemented.
The context parameter is correctly added and propagated to child functions.
Also applies to: 124-124
167-195: Previous review comments are still applicable.
The current implementation still has the issues identified in the previous reviews:
- URL host replacement logic needs improvement
- Error logging should use proper logging instead of fmt.Printf
Let's verify the impact of the URL transformation logic:
#!/bin/bash
# Search for URLs in property values that might be affected
rg -l 'localhost:8080|\.reearth\.dev|\.reearth\.io' --type jsonLine range hint 199-205: LGTM: Context propagation in parsePropertyOptionalValue is correct.
The context is properly passed through to parsePropertyValue.
server/internal/usecase/interactor/nlslayer.go (2)
904-904: LGTM: Context propagation improvement.
Good improvement to pass the context to AddItemFromPropertyJSON. This ensures proper context propagation for cancellation signals and request-scoped values.
Also applies to: 921-921
861-884:
Review URL manipulation logic for security and robustness.
The URL handling code has several areas that could be improved:
- Error handling: The URL parsing error is only logged before returning. Consider adding more context to the error.
- Security: The host validation logic should use a whitelist approach rather than suffix matching.
- Host validation: The current host should be validated before being used in URL manipulation.
Consider applying these improvements:
if nlsLayerJSON.Config != nil {
config := *nlsLayerJSON.Config
if data, ok := config["data"].(map[string]interface{}); ok {
if u, ok := data["url"].(string); ok {
urlVal, err := url.Parse(u)
if err != nil {
- log.Infofc(ctx, "invalid url: %v\n", err.Error())
- return nil, nil, err
+ return nil, nil, fmt.Errorf("invalid URL in layer config: %w", err)
}
+ currentHost := adapter.CurrentHost(ctx)
+ if currentHost == "" {
+ return nil, nil, fmt.Errorf("current host not found in context")
+ }
+
+ // Use a whitelist of allowed domains
+ allowedDomains := map[string]bool{
+ "localhost:8080": true,
+ "reearth.dev": true,
+ "reearth.io": true,
+ }
+
- if urlVal.Host == "localhost:8080" || strings.HasSuffix(urlVal.Host, ".reearth.dev") || strings.HasSuffix(urlVal.Host, ".reearth.io") {
+ hostParts := strings.Split(urlVal.Host, ".")
+ if len(hostParts) >= 2 && (allowedDomains[urlVal.Host] || allowedDomains[strings.Join(hostParts[len(hostParts)-2:], ".")]) {
- currentHost := adapter.CurrentHost(ctx)
currentHost = strings.TrimPrefix(currentHost, "https://")
currentHost = strings.TrimPrefix(currentHost, "http://")
urlVal.Host = currentHostLet's verify the current host handling:
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
server/internal/usecase/interactor/project_test.go (1)
153-153: Consider making the host URL configurable.
While the current implementation works, consider extracting "https://xxxx.reearth.dev" to a test constant or configuration to make it more maintainable.
+const testHost = "https://xxxx.reearth.dev"
+
func TestImportProject(t *testing.T) {
ctx := context.Background()
- ctx = adapter.AttachCurrentHost(ctx, "https://xxxx.reearth.dev")
+ ctx = adapter.AttachCurrentHost(ctx, testHost)server/internal/usecase/interactor/nlslayer_test.go (1)
268-268: Consider using a standard test domain
While the context modification is correct, consider using a standard test domain (e.g., example.com) instead of xxxx.reearth.dev. This follows best practices for test URLs and avoids potential confusion with real domains.
- ctx = adapter.AttachCurrentHost(ctx, "https://xxxx.reearth.dev")
+ ctx = adapter.AttachCurrentHost(ctx, "https://example.com")server/internal/usecase/interactor/nlslayer.go (1)
865-870: Improve URL validation error handling.
The error handling for URL parsing could be more informative. Consider wrapping the error with additional context about which URL failed to parse.
-if err != nil {
- log.Infofc(ctx, "invalid url: %v\n", err.Error())
- return nil, nil, err
-}
+if err != nil {
+ return nil, nil, fmt.Errorf("failed to parse URL %q: %w", u, err)
+}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- server/internal/usecase/interactor/nlslayer.go (5 hunks)
- server/internal/usecase/interactor/nlslayer_test.go (3 hunks)
- server/internal/usecase/interactor/project_test.go (3 hunks)
🔇 Additional comments (3)
server/internal/usecase/interactor/project_test.go (1)
Line range hint 89-117: LGTM: Well-structured test case for CoreSupport feature.
The new test case thoroughly validates the CoreSupport parameter while maintaining existing error handling scenarios.
server/internal/usecase/interactor/nlslayer_test.go (1)
9-9: LGTM: Import addition is appropriate
The new adapter import is correctly placed and necessary for the host context functionality being tested.
server/internal/usecase/interactor/nlslayer.go (1)
906-907: LGTM: Context propagation in property handling.
The addition of context parameter to AddItemFromPropertyJSON calls is consistent and improves context propagation through the application.
Also applies to: 923-924
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/e2e/test.zipis excluded by!**/*.zip
📒 Files selected for processing (2)
- server/e2e/gql_import_export_test.go (3 hunks)
- server/internal/usecase/interactor/nlslayer.go (5 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_import_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (2)
server/e2e/gql_import_export_test.go (1)
60-65: Add validation for exported project ZIP content.
The test verifies the export operation succeeds but doesn't inspect the ZIP contents. A past review already suggested adding validation here to ensure:
- The correct file structure is maintained
- URLs are properly transformed
- Project metadata is accurately preserved
server/internal/usecase/interactor/nlslayer.go (1)
904-904: LGTM: Context propagation is properly implemented.
The updates to include context parameter in AddItemFromPropertyJSON calls are correct and maintain proper context propagation through the call chain.
Also applies to: 921-921
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
server/e2e/gql_import_export_test.go (1)
93-93: Remove debug print statement.
The commented-out debug print statement should be removed as it's not providing any value.
- // fmt.Println(toJSONString(r.Raw()))📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- server/e2e/gql_import_export_test.go (3 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_import_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/e2e/test.zipis excluded by!**/*.zip
📒 Files selected for processing (1)
- server/e2e/gql_import_export_test.go (3 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_import_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (1)
server/e2e/gql_import_export_test.go (1)
81-81:
Fix test file dependency.
The test depends on "test.zip" which isn't created in the test setup. This makes the test unreliable.
Create the test file using TestCallExportProject:
- filePath := "test.zip"
+ // Create and export a project first
+ e2 := StartServer(t, &config.Config{
+ Origins: []string{"https://example.com"},
+ AuthSrv: config.AuthSrvConfig{
+ Disabled: true,
+ },
+ }, true, baseSeeder)
+ pID := createProjectWithExternalImage(e2, "test")
+ filePath := exporProject(t, e2, pID)
+ defer os.Remove(filePath)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
server/e2e/gql_project_export_test.go (1)
14-15: Enhance environment setup documentation.
Consider adding more context to the environment setup comments, such as prerequisites and expected outcomes.
-// export REEARTH_DB=mongodb://localhost
-// go test -v -run TestCallExportProject ./e2e/...
+// Test Prerequisites:
+// 1. MongoDB running locally
+// 2. Set environment variable: export REEARTH_DB=mongodb://localhost
+//
+// Run test:
+// go test -v -run TestCallExportProject ./e2e/...
+//
+// Expected outcome:
+// - Creates a test project with external images
+// - Exports the project to a zip file
+// - Cleans up temporary filesserver/e2e/gql_project_import_test.go (1)
114-553: Add documentation for GraphQL query structure.
The GraphQL query is well-structured but lacks documentation explaining the purpose and relationships between fragments. Consider adding comments to explain the major sections and their relationships.
Add documentation like this at the start of the query:
const GetSceneGuery = `
+# Query to retrieve a complete scene with all its components:
+# - Basic scene information (id, rootLayerId, etc.)
+# - Properties and their schemas
+# - Widgets and their alignment system
+# - Stories and their pages
+# - Layers and their styles
+# - Features and their geometries
query GetScene($sceneId: ID!, $lang: Lang) {
// ... rest of the query
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- server/e2e/gql_project_export_test.go (1 hunks)
- server/e2e/gql_project_import_test.go (1 hunks)
🧰 Additional context used
📓 Learnings (2)
server/e2e/gql_project_export_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
server/e2e/gql_project_import_test.go (4)
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:48-80
Timestamp: 2024-10-08T16:38:59.270Z
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.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-10-08T16:38:59.270Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (3)
server/e2e/gql_project_export_test.go (1)
17-66: Verify if GraphQL assertions are stable.
Previous learnings indicate that GraphQL assertions were disabled in similar test files due to problems. Please verify if these assertions are now stable or if they need to be conditionally disabled.
✅ Verification successful
Based on the search results, I can now generate a final response. The evidence shows that GraphQL assertions are actively used and working across multiple test files, particularly in gql_project_export_test.go and gql_storytelling_test.go. There are no signs of disabled assertions, and the assertions are consistently used for verifying GraphQL responses.
GraphQL assertions are stable and working as expected
The codebase shows consistent and successful use of GraphQL assertions through res.Path().Equal() pattern, particularly for:
- Property value validations
- Response structure verification
- Array content checks
- Complex nested object validations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for disabled GraphQL assertions in test files
rg -l "(?i)disabled.*graphql.*assert" server/e2e/
Length of output: 51
Script:
#!/bin/bash
# Let's search for any GraphQL assertions in test files to understand the testing patterns
rg -B2 -A2 "\.Path.*\$\.data" server/e2e/
Length of output: 42
Script:
#!/bin/bash
# Search for any disabled tests or skipped assertions
rg -B2 -A2 "(?i)(skip|disabled).*test" server/e2e/
Length of output: 225
Script:
#!/bin/bash
# Look for test helper functions that might contain assertion logic
ast-grep --pattern 'func $_(t *testing.T, $$$) {
$$$
}' server/e2e/
Length of output: 71
Script:
#!/bin/bash
# Let's try a different approach to find GraphQL assertions using gjson syntax
rg -B2 -A2 "res\.(Path|Get)" server/e2e/
Length of output: 7994
Script:
#!/bin/bash
# Look for test files that might contain similar patterns
fd "gql.*test.go" server/e2e/ --exec cat {}
Length of output: 119840
Script:
#!/bin/bash
# Let's search for any test files that might have disabled GraphQL assertions
rg -B2 -A2 "(?i)(disable|skip).*assert" server/e2e/
Length of output: 53
Script:
#!/bin/bash
# Look for any test helper functions that handle GraphQL assertions
rg -B2 -A2 "\.Path.*\.Equal" server/e2e/
Length of output: 4340
server/e2e/gql_project_import_test.go (2)
1-15: LGTM! Package structure and imports are well organized.
The package declaration, imports, and test execution instructions are clear and properly structured.
29-35: Verify if previous GraphQL assertion issues are resolved.
Based on previous learnings, GraphQL assertions were disabled in similar test files due to existing problems. We should verify if these issues have been resolved before adding more detailed assertions.
| defer func() { | ||
| err := os.Remove(fileName) | ||
| assert.Nil(t, err) | ||
| }() |
There was a problem hiding this comment.
Improve error handling in deferred cleanup.
The deferred cleanup could fail silently. Consider logging the error or using a test cleanup function.
- defer func() {
- err := os.Remove(fileName)
- assert.Nil(t, err)
- }()
+ t.Cleanup(func() {
+ if err := os.Remove(fileName); err != nil {
+ t.Errorf("Failed to cleanup test file %s: %v", fileName, err)
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer func() { | |
| err := os.Remove(fileName) | |
| assert.Nil(t, err) | |
| }() | |
| t.Cleanup(func() { | |
| if err := os.Remove(fileName); err != nil { | |
| t.Errorf("Failed to cleanup test file %s: %v", fileName, err) | |
| } | |
| }) |
| _, res = updatePropertyValue(e, propID1, "default", "", "src", "http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png", "URL") | ||
| res.Path("$.data.updatePropertyValue.propertyField.value").Equal("http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png") | ||
|
|
||
| _, res = updatePropertyValue(e, propID2, "default", wID.String(), "src", "https://test.com/project.jpg", "URL") | ||
| res.Path("$.data.updatePropertyValue.propertyField.value").Equal("https://test.com/project.jpg") | ||
|
|
||
| _, res = updatePropertyValue(e, propID3, "default", wID.String(), "src", "https://api.visualizer.test.reearth.dev/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png", "URL") | ||
| res.Path("$.data.updatePropertyValue.propertyField.value").Equal("https://api.visualizer.test.reearth.dev/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract test URLs to constants and validate URL patterns.
Hard-coded URLs make tests brittle and harder to maintain. Consider extracting them to constants and adding validation for URL patterns.
+const (
+ testLocalAssetURL = "http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png"
+ testExternalURL = "https://test.com/project.jpg"
+ testAPIAssetURL = "https://api.visualizer.test.reearth.dev/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png"
+)
+
+func isValidAssetURL(url string) bool {
+ // Add URL validation logic here
+ return true
+}
- _, res = updatePropertyValue(e, propID1, "default", "", "src", "http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png", "URL")
+ _, res = updatePropertyValue(e, propID1, "default", "", "src", testLocalAssetURL, "URL")
+ assert.True(t, isValidAssetURL(testLocalAssetURL))
res.Path("$.data.updatePropertyValue.propertyField.value").Equal("http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png")Committable suggestion was skipped due to low confidence.
| func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { | ||
| requestBody := GraphQLRequest{ | ||
| OperationName: "CreateProject", | ||
| Query: `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { | ||
| createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { | ||
| project { | ||
| id | ||
| __typename | ||
| } | ||
| __typename | ||
| } | ||
| }`, | ||
| Variables: map[string]any{ | ||
| "name": name, | ||
| "description": "abc", | ||
| "imageUrl": "https://test.com/project.jpg", | ||
| "teamId": wID.String(), | ||
| "visualizer": "CESIUM", | ||
| "coreSupport": true, | ||
| }, | ||
| } | ||
| res := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithJSON(requestBody). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| return res.Path("$.data.createProject.project.id").Raw().(string) | ||
| } |
There was a problem hiding this comment.
Add validation for project creation response.
The project creation function should validate the response structure and ensure the project was created successfully.
func createProjectWithExternalImage(e *httpexpect.Expect, name string) string {
+ const expectedTypeName = "Project"
+
requestBody := GraphQLRequest{
OperationName: "CreateProject",
@@ -94,5 +98,11 @@
Expect().
Status(http.StatusOK).
JSON()
- return res.Path("$.data.createProject.project.id").Raw().(string)
+
+ projectID := res.Path("$.data.createProject.project.id").Raw().(string)
+ assert.NotEmpty(t, projectID, "Project ID should not be empty")
+
+ typeName := res.Path("$.data.createProject.project.__typename").String().Raw()
+ assert.Equal(t, expectedTypeName, typeName, "Unexpected typename in response")
+
+ return projectID
}Committable suggestion was skipped due to low confidence.
| func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { | ||
| requestBody := GraphQLRequest{ | ||
| OperationName: "ExportProject", | ||
| Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }", | ||
| Variables: map[string]any{ | ||
| "projectId": p, | ||
| }, | ||
| } | ||
| r := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithJSON(requestBody). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
| downloadPath := r. | ||
| Value("data").Object(). | ||
| Value("exportProject").Object(). | ||
| Value("projectDataPath").String().Raw() | ||
| downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| Body().Raw() | ||
| fileName := "project_data.zip" | ||
| err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) | ||
| assert.Nil(t, err) | ||
| return fileName | ||
| } |
There was a problem hiding this comment.
Enhance export functionality with additional validations.
The export function needs improvements in several areas:
- Add timeout handling for HTTP requests
- Validate zip file contents
- Use appropriate file permissions
- Validate response content-type
func exporProject(t *testing.T, e *httpexpect.Expect, p string) string {
+ const timeout = 30 * time.Second
+ ctx, cancel := context.WithTimeout(context.Background(), timeout)
+ defer cancel()
+
requestBody := GraphQLRequest{
OperationName: "ExportProject",
Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }",
@@ -122,9 +126,19 @@
downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)).
Expect().
Status(http.StatusOK).
+ ContentType("application/zip").
Body().Raw()
+
fileName := "project_data.zip"
- err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm)
+ err := os.WriteFile(fileName, []byte(downloadResponse), 0600)
assert.Nil(t, err)
+
+ // Validate zip file contents
+ reader, err := zip.OpenReader(fileName)
+ assert.Nil(t, err)
+ defer reader.Close()
+
+ // Add validation for expected files in the zip
+ assert.Greater(t, len(reader.File), 0, "Zip file should not be empty")
+
return fileName
}Committable suggestion was skipped due to low confidence.
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | ||
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | ||
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) |
There was a problem hiding this comment.
Add assertions for block creation responses.
The block creation responses are currently ignored. Consider adding assertions to verify successful creation of blocks.
- _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
- _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
- _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
+ block1ID, _, res1 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
+ assert.NotEmpty(t, block1ID)
+ res1.Path("$.data.createBlock.block.id").NotNull()
+ block2ID, _, res2 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
+ assert.NotEmpty(t, block2ID)
+ res2.Path("$.data.createBlock.block.id").NotNull()
+ block3ID, _, res3 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil)
+ assert.NotEmpty(t, block3ID)
+ res3.Path("$.data.createBlock.block.id").NotNull()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| _, _, _ = createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| block1ID, _, res1 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| assert.NotEmpty(t, block1ID) | |
| res1.Path("$.data.createBlock.block.id").NotNull() | |
| block2ID, _, res2 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| assert.NotEmpty(t, block2ID) | |
| res2.Path("$.data.createBlock.block.id").NotNull() | |
| block3ID, _, res3 := createBlock(e, sID, storyID, pageID, "reearth", "imageStoryBlock", nil) | |
| assert.NotEmpty(t, block3ID) | |
| res3.Path("$.data.createBlock.block.id").NotNull() |
| func importProject(t *testing.T, e *httpexpect.Expect, filePath string) *httpexpect.Object { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| t.Fatalf("failed to open file: %v", err) | ||
| } | ||
| defer func() { | ||
| if cerr := file.Close(); cerr != nil && err == nil { | ||
| err = cerr | ||
| } | ||
| }() | ||
| requestBody := map[string]interface{}{ | ||
| "operationName": "ImportProject", | ||
| "variables": map[string]interface{}{ | ||
| "teamId": wID.String(), | ||
| "file": nil, | ||
| }, | ||
| "query": `mutation ImportProject($teamId: ID!, $file: Upload!) { | ||
| importProject(input: {teamId: $teamId, file: $file}) { | ||
| projectData | ||
| __typename | ||
| } | ||
| }`, | ||
| } | ||
| r := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithMultipart(). | ||
| WithFormField("operations", toJSONString(requestBody)). | ||
| WithFormField("map", `{"0": ["variables.file"]}`). | ||
| WithFile("0", filePath). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
| projectData := r.Value("data").Object().Value("importProject").Object().Value("projectData") | ||
| projectData.NotNull() | ||
| return projectData.Object() | ||
| } | ||
|
|
||
| func getScene(e *httpexpect.Expect, s string) *httpexpect.Object { | ||
| requestBody := GraphQLRequest{ | ||
| OperationName: "GetScene", | ||
| Query: GetSceneGuery, | ||
| Variables: map[string]any{ | ||
| "sceneId": s, | ||
| }, | ||
| } | ||
| r := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithJSON(requestBody). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
| v := r.Value("data").Object().Value("node") | ||
| v.NotNull() | ||
| return v.Object() | ||
| } | ||
|
|
||
| func toJSONString(v interface{}) string { | ||
| jsonData, _ := json.Marshal(v) | ||
| return string(jsonData) | ||
| } |
There was a problem hiding this comment.
Improve error handling and resource management in helper functions.
Several improvements can be made to the helper functions:
- The
toJSONStringfunction silently ignores JSON marshaling errors - File handling in
importProjectcould be improved - GraphQL variables lack type safety
Consider these improvements:
func importProject(t *testing.T, e *httpexpect.Expect, filePath string) *httpexpect.Object {
file, err := os.Open(filePath)
if err != nil {
t.Fatalf("failed to open file: %v", err)
}
- defer func() {
- if cerr := file.Close(); cerr != nil && err == nil {
- err = cerr
- }
- }()
+ defer file.Close()
+
+ fileInfo, err := file.Stat()
+ if err != nil {
+ t.Fatalf("failed to get file info: %v", err)
+ }
+ if fileInfo.Size() == 0 {
+ t.Fatal("file is empty")
+ }
requestBody := map[string]interface{}{
"operationName": "ImportProject",
"variables": map[string]interface{}{
"teamId": wID.String(),
"file": nil,
},
"query": `mutation ImportProject($teamId: ID!, $file: Upload!) {
importProject(input: {teamId: $teamId, file: $file}) {
projectData
__typename
}
}`,
}
// ... rest of the function
}
func toJSONString(v interface{}) string {
- jsonData, _ := json.Marshal(v)
- return string(jsonData)
+ jsonData, err := json.Marshal(v)
+ if err != nil {
+ panic(fmt.Sprintf("failed to marshal JSON: %v", err))
+ }
+ return string(jsonData)
}Committable suggestion was skipped due to low confidence.
| func TestCallImportProject(t *testing.T) { | ||
|
|
||
| e := StartServer(t, &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| }, true, baseSeeder) | ||
|
|
||
| filePath := "test.zip" | ||
|
|
||
| r := importProject(t, e, filePath) | ||
|
|
||
| r.Value("project").NotNull() | ||
| r.Value("plugins").Array() | ||
| r.Value("schema").Array() | ||
| r.Value("scene").NotNull() | ||
| r.Value("nlsLayer").Array() | ||
| r.Value("style").Array() | ||
| r.Value("story").NotNull() | ||
|
|
||
| sid := r.Value("scene").Object().Value("id").Raw().(string) | ||
|
|
||
| r = getScene(e, sid) | ||
| // fmt.Println(toJSONString(r.Raw())) | ||
|
|
||
| r.Value("id").Equal(sid) | ||
|
|
||
| } |
There was a problem hiding this comment.
Add comprehensive test coverage and proper test file handling.
The test function has several areas that could be improved:
- The test file path is hardcoded without verifying its existence
- Missing test cases for error scenarios
- No cleanup of test resources
- Limited assertions on the imported data
Consider implementing these improvements:
func TestCallImportProject(t *testing.T) {
+ // Setup test file
+ testFile := "test.zip"
+ if _, err := os.Stat(testFile); os.IsNotExist(err) {
+ t.Skipf("Test file %s not found", testFile)
+ }
+
+ // Ensure cleanup
+ t.Cleanup(func() {
+ // Add cleanup logic if needed
+ })
e := StartServer(t, &config.Config{
Origins: []string{"https://example.com"},
AuthSrv: config.AuthSrvConfig{
Disabled: true,
},
}, true, baseSeeder)
- filePath := "test.zip"
-
- r := importProject(t, e, filePath)
+ r := importProject(t, e, testFile)
r.Value("project").NotNull()
r.Value("plugins").Array()
r.Value("schema").Array()
r.Value("scene").NotNull()
r.Value("nlsLayer").Array()
r.Value("style").Array()
r.Value("story").NotNull()
+
+ // Add more specific assertions
+ r.Value("project").Object().Value("id").NotNull()
+ r.Value("project").Object().Value("name").NotNull()
sid := r.Value("scene").Object().Value("id").Raw().(string)
r = getScene(e, sid)
r.Value("id").Equal(sid)
+
+ // Test error cases
+ t.Run("invalid_file", func(t *testing.T) {
+ r := importProject(t, e, "nonexistent.zip")
+ r.Value("errors").Array().NotEmpty()
+ })
}Committable suggestion was skipped due to low confidence.
Overview
Fixes for file import and export
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation