feat(server): sync sketch layer properties #1348
Conversation
WalkthroughThis pull request introduces comprehensive changes to custom property management in the GraphQL layer. The modifications focus on enhancing the functionality for updating, changing titles, and removing custom properties across multiple server components. The changes span GraphQL schema definitions, resolver implementations, and internal layer interaction methods, providing a more flexible approach to managing custom properties in layers. Additionally, improvements to JSON handling functions and the introduction of end-to-end tests for custom properties are included. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
server/e2e/gql_storytelling_test.go (1)
Line range hint
985-994: Improve test maintainability.The JSON comparison could be simplified by:
- Moving the expected JSON to a test fixture file
- Using constants for repeated values like plugin IDs
- Breaking down the large JSON into smaller, focused test cases
Example refactor:
+const ( + testPluginID = "reearth" + testExtensionID = "textStoryBlock" +) + +func loadTestFixture(t *testing.T, name string) string { + data, err := os.ReadFile(filepath.Join("testdata", name)) + assert.NoError(t, err) + return string(data) +} + func TestStoryPublishing(t *testing.T) { // ... test setup ... - expected := fmt.Sprintf(`{...}`, sID, storyID, extensionId, blockID, pageID, pluginId) + expected := fmt.Sprintf(loadTestFixture(t, "story_publishing.json"), + sID, storyID, testExtensionID, blockID, pageID, testPluginID) RegexpJSONEReadCloser(t, rc, expected) // ... rest of test ... }server/e2e/gql_custom_property_test.go (2)
11-99: Consider adding more test cases.While the happy path is well tested, consider adding tests for:
- Edge cases (empty properties, non-existent titles)
- Error cases (invalid inputs, transaction failures)
266-279: Consider enhancing test data setup.The
setupTestDatafunction could be more robust by:
- Adding validation for created resources
- Returning error instead of using multiple return values
- Adding cleanup in case of partial setup failure
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
server/e2e/common.go(2 hunks)server/e2e/gql_custom_property_test.go(1 hunks)server/e2e/gql_project_import_test.go(0 hunks)server/e2e/gql_storytelling_test.go(25 hunks)server/gql/newlayer.graphql(2 hunks)server/internal/adapter/gql/generated.go(23 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_nlslayer.go(2 hunks)server/internal/usecase/interactor/nlslayer.go(2 hunks)server/internal/usecase/interfaces/nlslayer.go(1 hunks)
💤 Files with no reviewable changes (1)
- server/e2e/gql_project_import_test.go
🧰 Additional context used
🪛 GitHub Check: ci-server / ci-server-lint
server/internal/usecase/interactor/nlslayer.go
[failure] 752-752:
File is not properly formatted (gofmt)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / Deploy app to test env
🔇 Additional comments (18)
server/e2e/gql_storytelling_test.go (1)
708-709: LGTM! Good error case coverage.Testing with an invalid page ID is a good practice for ensuring proper error handling.
server/internal/adapter/gql/generated.go (4)
1580-1581: LGTM! Consistent interface design.The new mutation resolvers follow the established pattern and maintain consistency by returning
UpdateNLSLayerPayload.
Line range hint
31541-31711: LGTM! Generated code follows gqlgen patterns.The field context and argument handling code is properly generated and includes appropriate error handling.
Line range hint
60168-61984: Verify field order matches GraphQL schema.The
fieldsInOrderarrays define a specific processing order for input fields. Ensure this matches the order in the GraphQL schema to maintain consistency.Run this script to compare field order:
✅ Verification successful
Field order verification completed successfully
The field order in the generated code matches exactly with the GraphQL schema definitions in
server/gql/newlayer.graphqlfor both input types:
ChangeCustomPropertyTitleInput: layerId, schema, oldTitle, newTitleRemoveCustomPropertyInput: layerId, schema, removedTitle🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Extract field order from GraphQL schema echo "Schema field order:" rg -A 5 'input (Change|Remove)CustomProperty.*Input \{' | rg '^\s+\w+:' # Compare with fieldsInOrder in generated code echo "Generated code field order:" rg 'fieldsInOrder.*layerId.*schema.*(oldTitle|removedTitle)'Length of output: 474
Script:
#!/bin/bash # Search for input type definitions using ast-grep echo "GraphQL Schema Input Types:" ast-grep --pattern 'input ChangeCustomPropertyTitleInput { $$$ }' echo -e "\nInput Type 2:" ast-grep --pattern 'input RemoveCustomPropertyInput { $$$ }' # Show more context from generated code echo -e "\nGenerated Code Context:" rg -B2 -A2 'fieldsInOrder.*layerId.*schema.*(oldTitle|removedTitle)'Length of output: 1049
Script:
#!/bin/bash # Find GraphQL schema files echo "Looking for GraphQL schema files:" fd -e graphql -e gql # Search for input types in all files echo -e "\nSearching for input type definitions:" rg -A 6 'input (ChangeCustomPropertyTitleInput|RemoveCustomPropertyInput)' --type graphqlLength of output: 772
Script:
#!/bin/bash # Search for input types in relevant schema files echo "Searching in property.graphql:" cat server/gql/property.graphql | grep -A 6 'input.*CustomProperty.*Input' echo -e "\nSearching in layer.graphql:" cat server/gql/layer.graphql | grep -A 6 'input.*CustomProperty.*Input' echo -e "\nSearching in _shared.graphql:" cat server/gql/_shared.graphql | grep -A 6 'input.*CustomProperty.*Input'Length of output: 443
Script:
#!/bin/bash # Broad search across all schema files echo "Searching all schema files for input types:" rg -i -A 10 'input.*custompropertyinput' server/gql/ # Alternative search pattern echo -e "\nAlternative search:" rg -i -A 10 'input.*\{.*title.*\}' server/gql/Length of output: 880
Script:
#!/bin/bash echo "Searching for ChangeCustomPropertyTitleInput:" rg -i -A 10 'input.*ChangecustompropertytitleInput' server/gql/ # Alternative search pattern echo -e "\nAlternative search:" rg -i -A 10 'input.*\{.*oldTitle.*newTitle' server/gql/Length of output: 1121
9450-9465: Consider adding schema validation for the JSON field.The
schemafield of typeJSONin all input types could benefit from runtime validation to prevent invalid schemas from causing issues.Run this script to check if schema validation is implemented elsewhere:
server/internal/usecase/interfaces/nlslayer.go (1)
88-89: LGTM! Well-structured interface additions.The new methods
ChangeCustomPropertyTitleandRemoveCustomPropertyare well-defined with clear parameters and consistent error handling patterns.server/internal/adapter/gql/resolver_mutation_nlslayer.go (3)
Line range hint
221-239: LGTM! Clean implementation of UpdateCustomProperties.The resolver correctly maps the GraphQL input to the usecase layer and handles errors appropriately.
240-257: LGTM! Well-structured ChangeCustomPropertyTitle implementation.The resolver properly handles the title change operation with appropriate error handling.
Line range hint
259-275: LGTM! Clean implementation of RemoveCustomProperty.The resolver follows the established pattern and properly handles the removal operation.
server/internal/usecase/interactor/nlslayer.go (2)
261-266: LGTM! Proper config update.The code correctly updates the name in both the layer and its config properties.
686-730: LGTM! Well-implemented title change functionality.The implementation properly handles:
- Transaction management
- Feature property updates
- Schema updates
- Error cases
server/e2e/gql_custom_property_test.go (1)
101-181: LGTM! Comprehensive test coverage for new mutations.The tests thoroughly verify the behavior of
ChangeCustomPropertyTitleandRemoveCustomPropertymutations.Also applies to: 183-262
server/gql/newlayer.graphql (4)
131-134: LGTM! Appropriate rename from AddCustomPropertySchemaInputThe rename better reflects the operation's purpose since it's updating the schema rather than just adding properties.
136-141: LGTM! Well-structured input type for title changesThe input type properly captures all necessary fields for renaming a custom property, with appropriate nullability.
143-146: LGTM! Well-structured input type for property removalThe input type correctly captures all necessary fields for removing a custom property, with appropriate nullability.
213-218: LGTM! Consistent mutation definitionsThe new mutations follow the established pattern of layer operations, using appropriate input types and return values.
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
273-278: LGTM! Correctly generated structThe generated struct matches the GraphQL schema definition with appropriate Go types and JSON tags.
1393-1397: LGTM! Correctly generated structThe generated struct matches the GraphQL schema definition with appropriate Go types and JSON tags.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/internal/usecase/interactor/nlslayer.go (1)
261-266: Add error handling for type assertions.The type assertions for accessing the config properties could fail silently. Consider adding explicit type checks to handle potential errors.
if config := layer.Config(); config != nil { c := *config - if properties, ok := c["properties"].(map[string]interface{}); ok { - properties["name"] = *inp.Name + if props, ok := c["properties"]; ok { + if properties, ok := props.(map[string]interface{}); ok { + properties["name"] = *inp.Name + layer.UpdateConfig(&c) + } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/go.mod(0 hunks)server/internal/usecase/interactor/nlslayer.go(2 hunks)
💤 Files with no reviewable changes (1)
- server/go.mod
🔇 Additional comments (2)
server/internal/usecase/interactor/nlslayer.go (2)
752-752: Fix the formatting issue in the for loop.The code has a formatting issue in the range clause.
- for k := range *props { + for k := range *props {
686-774: Verify the impact on direct callers.Let's check for any direct callers of these methods that might need updates:
✅ Verification successful
Implementation and integration verified successfully
The methods are properly integrated into the codebase with complete GraphQL resolver implementation and E2E test coverage. All callers are correctly using the new methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find direct callers of the new methods # Search for direct calls to ChangeCustomPropertyTitle echo "Searching for ChangeCustomPropertyTitle callers..." rg "ChangeCustomPropertyTitle\(" -A 3 # Search for direct calls to RemoveCustomProperty echo "Searching for RemoveCustomProperty callers..." rg "RemoveCustomProperty\(" -A 3Length of output: 8292
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/e2e/gql_nlslayer_test.go (2)
Line range hint
737-762: Add response validation in the helper function.The function should validate the GraphQL response to ensure the mutation was successful. This helps catch issues early in the tests.
func updateCustomProperties( e *httpexpect.Expect, layerId string, schema map[string]any, ) (GraphQLRequest, *httpexpect.Value) { requestBody := GraphQLRequest{ OperationName: "UpdateCustomProperties", Query: `mutation UpdateCustomProperties($input: UpdateCustomPropertySchemaInput!) { updateCustomProperties(input: $input) { layer { id sketch { customPropertySchema } } } }`, Variables: map[string]interface{}{ "input": map[string]interface{}{ "layerId": layerId, "schema": schema, }, }, } res := Request(e, uID.String(), requestBody) + // Validate response + res.Object(). + Path("$.data.updateCustomProperties.layer.id").Equal(layerId) + res.Object(). + Path("$.data.updateCustomProperties.layer.sketch.customPropertySchema").NotNull() return requestBody, res }
810-810: Enhance test coverage with edge cases.The test only covers successful updates. Consider adding test cases for:
- Invalid schema formats
- Schema type changes
- Empty or null schema values
- Concurrent updates to the same layer
Also applies to: 833-833
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/e2e/gql_nlslayer_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / Deploy app to test env
🔇 Additional comments (1)
server/e2e/gql_nlslayer_test.go (1)
Line range hint
737-847: Add tests for new custom property mutations.Based on the PR objectives, tests are missing for:
changeCustomPropertyTitlemutationremoveCustomPropertymutationThese new operations should be tested to ensure complete coverage of the custom property synchronization feature.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
server/internal/usecase/interactor/nlslayer.go (1)
753-813:⚠️ Potential issueAdd scene write permission check.
The method should verify that the operator has write permissions for the scene before proceeding with the changes.
Add the permission check after retrieving the layer:
layer, err := i.nlslayerRepo.FindByID(ctx, inp.LayerID) if err != nil { return nil, err } +if err := i.CanWriteScene(layer.Scene(), operator); err != nil { + return nil, interfaces.ErrOperationDenied +} if layer.Sketch() == nil || layer.Sketch().FeatureCollection() == nil {
🧹 Nitpick comments (2)
server/internal/usecase/interactor/nlslayer.go (2)
262-267: Add nil check for properties map.The code should check if the "properties" field exists and is not nil before attempting to update it.
if config := layer.Config(); config != nil { c := *config if properties, ok := c["properties"].(map[string]interface{}); ok { + if properties != nil { properties["name"] = *inp.Name + } } }
789-797: Optimize property removal loop.The inner loop can be broken after finding and removing the property since we only need to remove one occurrence.
for _, feature := range layer.Sketch().FeatureCollection().Features() { if props := feature.Properties(); props != nil { for k := range *props { if k == removedTitle { delete(*props, k) + break } } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/e2e/common.go(2 hunks)server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_project_import_test.go(2 hunks)server/internal/usecase/interactor/nlslayer.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/common.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1348
File: server/e2e/common.go:280-282
Timestamp: 2025-01-14T04:44:52.731Z
Learning: In test utilities that handle JSON marshaling/unmarshaling, always include proper error handling to help debug test failures. This applies to functions like `toJSONString` in the e2e test package.
🪛 GitHub Check: ci-server / ci-server-lint
server/e2e/common.go
[failure] 259-259:
Error return value of actual.Close is not checked (errcheck)
🪛 golangci-lint (1.62.2)
server/e2e/common.go
259-259: Error return value of actual.Close is not checked
(errcheck)
🪛 GitHub Actions: ci
server/e2e/common.go
[error] 259-259: Error return value of actual.Close is not checked (errcheck)
🔇 Additional comments (4)
server/e2e/common.go (4)
250-256: LGTM! Robust JSON comparison implementation.The function correctly normalizes JSON strings before comparison and properly escapes regexp special characters.
266-270: LGTM! Clean implementation with proper error handling.The function correctly marshals the interface and handles potential errors.
272-280: LGTM! Flexible JSON normalization.The function correctly handles both JSON objects and arrays, with proper error handling.
282-287: LGTM! Well-implemented error handling.The function properly wraps JSON marshaling errors with context.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/e2e/common.go (1)
250-256: Enhance regex pattern escaping.The function only escapes square brackets (
[), but other regex special characters might need escaping too. Consider usingregexp.QuoteMetafor comprehensive escaping.- regexp.MustCompile(strings.ReplaceAll(aligningJSON(t, expected), "[", "\\[")), + regexp.MustCompile(regexp.QuoteMeta(aligningJSON(t, expected))),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/e2e/common.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
server/e2e/common.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1348
File: server/e2e/common.go:280-282
Timestamp: 2025-01-14T04:44:52.731Z
Learning: In test utilities that handle JSON marshaling/unmarshaling, always include proper error handling to help debug test failures. This applies to functions like `toJSONString` in the e2e test package.
🔇 Additional comments (3)
server/e2e/common.go (3)
270-274: LGTM!The function correctly handles JSON marshaling and comparison with proper error checking.
276-284: LGTM!The function correctly normalizes JSON format and handles both objects and arrays with proper error checking.
286-291: LGTM!The function implements proper error handling with good error context, aligning with the best practices for JSON utilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server/e2e/common.go (4)
250-256: LGTM! Consider adding documentation.The implementation correctly handles JSON comparison with regexp support. Consider adding a docstring to explain the purpose and behavior of the function.
+// JSONEqRegexp compares two JSON strings for equality using regular expressions. +// It normalizes both strings using aligningJSON before comparison. func JSONEqRegexp(t *testing.T, actual string, expected string) bool {
258-268: LGTM! Consider optimizing buffer allocation.The implementation correctly handles resource cleanup and error handling. Consider pre-allocating the buffer with a reasonable size to optimize memory usage.
- actualBuf := new(bytes.Buffer) + actualBuf := bytes.NewBuffer(make([]byte, 0, 1024)) // Pre-allocate 1KB
270-274: LGTM! Consider improving error messages.The implementation correctly handles JSON comparison for interface types. Consider adding more descriptive error messages to help with debugging.
- assert.Nil(t, err) + assert.Nil(t, err, "failed to marshal actual value to JSON")
276-284: LGTM! Consider improving error messages.The implementation correctly handles JSON normalization. Consider adding more descriptive error messages to help with debugging.
var obj interface{} err := json.Unmarshal([]byte(str), &obj) - assert.Nil(t, err) + assert.Nil(t, err, "failed to unmarshal JSON string: %s", str) strBytes, err := json.Marshal(obj) - assert.Nil(t, err) + assert.Nil(t, err, "failed to marshal normalized JSON")server/internal/usecase/interactor/nlslayer.go (2)
690-758: Consider extracting validation logic and standardizing error messages.The implementation is solid with proper transaction handling, permission checks, and value preservation during title changes. Consider these improvements:
- Extract the title validation logic into a separate function as it's also used in
RemoveCustomProperty.- Use predefined error constants instead of fmt.Errorf for consistency with the codebase.
+func (i *NLSLayer) validateCustomPropertyTitle(features []nlslayer.Feature, oldTitle, newTitle string) error { + titleExists := false + for _, feature := range features { + if props := feature.Properties(); props != nil { + if _, ok := (*props)[oldTitle]; ok { + titleExists = true + } + if newTitle != "" { + if _, ok := (*props)[newTitle]; ok { + return interfaces.ErrCustomPropertyTitleExists + } + } + } + } + if !titleExists { + return interfaces.ErrCustomPropertyNotFound + } + return nil +}
760-820: Reuse validation logic from the suggested helper function.The implementation is solid but has duplicate validation logic. Use the suggested
validateCustomPropertyTitlefunction to reduce code duplication:- titleExists := false - for _, feature := range layer.Sketch().FeatureCollection().Features() { - if props := feature.Properties(); props != nil { - if _, ok := (*props)[removedTitle]; ok { - titleExists = true - break - } - } - } - - if !titleExists { - return nil, fmt.Errorf("property with title %s not found", removedTitle) - } + if err := i.validateCustomPropertyTitle(layer.Sketch().FeatureCollection().Features(), removedTitle, ""); err != nil { + return nil, err + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/e2e/common.go(2 hunks)server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_project_import_test.go(2 hunks)server/internal/usecase/interactor/nlslayer.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/e2e/gql_asset_test.go
- server/e2e/gql_project_import_test.go
🧰 Additional context used
📓 Learnings (1)
server/e2e/common.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1348
File: server/e2e/common.go:280-282
Timestamp: 2025-01-14T04:44:52.731Z
Learning: In test utilities that handle JSON marshaling/unmarshaling, always include proper error handling to help debug test failures. This applies to functions like `toJSONString` in the e2e test package.
🔇 Additional comments (3)
server/e2e/common.go (1)
286-291: LGTM! Error handling is well implemented.The implementation correctly handles JSON conversion and error wrapping.
server/internal/usecase/interactor/nlslayer.go (2)
262-267: LGTM! Good synchronization of layer name with config.The implementation correctly updates the "name" property in the config when the layer name changes, with proper nil checks and type assertions.
656-658: LGTM! Good security practice with early permission check.The permission check is correctly placed before any modifications and uses the appropriate error constant.
Co-authored-by: Tomokazu Tantaka <t.tantaka@eukarya.io>
Overview
Synchronize data updates in the sketch layer.
What I've done
To synchronize the data, the server needs to know about title renames and deleted titles.
Therefore, the API has been updated.
Updated GraphQL:
Note: Since addCustomProperties is identical to updateCustomProperties, please call updateCustomProperties instead.
What I haven't done
The GeoJSON format check will be implemented in a separate pull request.
How I tested
I added a method to use assert.Regexp for checking with regular expressions in tests:
func JSONEqRegexp(t *testing.T, actual string, expected string) bool
Using this, I created the following end-to-end (e2e) tests:
https://github.com/reearth/reearth-visualizer/pull/1348/files#diff-b1a67b8e570b786682445adb6c015cc17d86ce6af88589b785e110089b289e1b
Which point I want you to review particularly
When changing the type of a custom property, the value remains unchanged.
Therefore, along with calling UpdateCustomProperties, you also need to update the value using UpdateGeoJSONFeature.
Memo
The implementation details for the web have been documented in Notion.
https://www.notion.so/eukarya/BE-Support-synchronization-of-properties-after-schema-changes-of-sketch-layer-17716e0fb165806eb2d1eef2fb6244e3
Summary by CodeRabbit
Release Notes
New Features
Improvements
Testing
Chores