fix(server): revert sort logic [VIZ-1423] #1528
Conversation
WalkthroughThis pull request updates multiple modules to incorporate and leverage a property schema during conversion and mutation operations. The changes add schema-based sorting logic for property items in conversion functions and adapt method signatures to accept a new schema parameter. Several test cases have been updated to reflect different field orders, and obsolete tests have been removed. Additionally, sorting logic in group processing has been removed, and a new GraphQL field ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant Builtin
participant Converter
Client->>Resolver: Send AddPropertyItem mutation
Resolver->>Builtin: GetPropertySchema()
Builtin-->>Resolver: Return property schema object
Resolver->>Converter: ToPropertyItem(item, property, groupList, schema)
Converter-->>Resolver: Return PropertyItem
Resolver-->>Client: Return PropertyItemPayload
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/adapter/gql/loader_property.go(2 hunks)
🔇 Additional comments (3)
server/internal/adapter/gql/loader_property.go (3)
9-9: No issues with the new import.
Importingbuiltinis appropriate given its usage below; no concerns here.
11-11: No issues with the new import.
Importing thepropertypackage is necessary for the subsequent logic; no concerns here.
37-42: Validate fallback logic if no built-in schema is found.
Whenbuiltin.GetPropertySchema(prop.Schema())returnsnil, the property is appended as-is. This fallback is fine, but ensure no edge cases (e.g. partial or unknown schemas) lead to lost data or duplication. Consider verifying all properties are always appended once.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/pkg/builtin/main.go (1)
74-79: Global state modification needs a reset mechanismThe
E2ETestChangefunction modifies the globalpluginManifestvariable without providing a way to restore the original state after testing. This could lead to test interdependencies if multiple tests use this function sequentially.Consider adding a companion function to restore the original state:
+var originalPluginManifest = pluginManifest +// RestoreOriginalPluginManifest restores the original plugin manifest after testing +func RestoreOriginalPluginManifest() { + pluginManifest = originalPluginManifest +} func E2ETestChange(e2ePluginManifestJSON []byte) { var pluginTranslationList = manifest.TranslationMap{ "ja": manifest.MustParseTranslationFromBytes(pluginManifestJSON_ja), } pluginManifest = manifest.MustParseSystemFromBytes(e2ePluginManifestJSON, nil, pluginTranslationList.TranslatedRef()) }server/e2e/gql_property_schema_test.go (1)
41-123: Consider extracting test schema to a separate fileThe large YAML schema definition embedded in the test code makes the test less readable and harder to maintain.
Consider moving this schema definition to a separate file in a
testdatadirectory and loading it in the test:- data := ` -id: reearth -system: true ... -` + data, err := os.ReadFile("testdata/test_schema.yml") + if err != nil { + t.Fatalf("Failed to read test schema: %v", err) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/e2e/gql_property_schema_test.go(1 hunks)server/pkg/builtin/main.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
server/e2e/gql_property_schema_test.go (1)
server/pkg/builtin/main.go (1)
E2ETestChange(74-79)
🔇 Additional comments (2)
server/e2e/gql_property_schema_test.go (2)
13-137: Test validates field ordering matches schema definitionThis test confirms that the property field ordering follows the schema definition rather than being automatically sorted, which aligns with the PR objective to revert sorting logic affecting display order.
26-26:❓ Verification inconclusive
Investigate potential bug in array property handling
The comment
// bug?suggests there might be an issue with how array values are being processed or returned. This should be investigated to determine if it's a real bug or expected behavior.
🏁 Script executed:
#!/bin/bash # Search for other similar array property handling in tests or implementation rg -A 10 -B 5 "updatePropertyValue.*ARRAY" --glob "*.go" | grep -v "test_"Length of output: 2975
Double-check integer array conversion in updatePropertyValue
It appears that when handling integer array updates (for example, in the "tile_zoomLevel" property), the test currently expects the result to be[]*int{nil, nil}, whereas other array types (such as string arrays) return correctly populated values. This discrepancy suggests that the conversion logic for integer arrays might not be working as intended.
- In
server/e2e/gql_property_schema_test.go, the call:returns nil pointers instead of the expected integer values._, r = updatePropertyValue(e, propID, "tiles", groupID, "tile_zoomLevel", []int{10, 20}, "ARRAY") r.Path("$.data.updatePropertyValue.propertyField.value").IsEqual([]*int{nil, nil}) // bug?- Compare this with other cases (e.g., string arrays) where the returned values match the input.
Please investigate whether this behavior is an intentional side effect or a bug in the conversion process for integer arrays.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/pkg/property/schema_group.go (1)
122-146: Enhance error handling and handle no-op moves.Currently, out-of-range indices or a
nilreceiver cause a silent return, which might obscure logic errors. Also, skipping the operation whenfrom == tocan be beneficial.func (s *SchemaGroup) Move(from int, to int) { + // Consider a quick check for no-op moves + if s == nil || from == to { + return + } if from < 0 || from >= len(s.fields) { - return + // Optionally log or return an error return } if to < 0 || to >= len(s.fields) { - return + // Optionally log or return an error return } // existing logic remains unchanged }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/e2e/gql_property_schema_test.go(1 hunks)server/pkg/builtin/main.go(1 hunks)server/pkg/property/schema_group.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/pkg/builtin/main.go
- server/e2e/gql_property_schema_test.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (2)
server/pkg/property/schema_group.go (2)
39-39: Reverted sorting logic with a shallow copy.This line reverts the sorting by returning a shallow copy of
s.fields. It looks aligned with the stated goal of removing sorting. Keep in mind that the fields themselves remain mutable references, so any in-place modification to a field will still affect the underlying objects.
121-121: No functional change.This line is merely whitespace or a blank line; no impact on code logic is observed.
Overview
Sort the properties according to the order defined in manifest.yml.
Since property sorting was added, the display order has changed.
The sort that was previously added for debugging was affecting the display order.
What I've done
Revert previous changes
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores