Property schema edge case tests+fixes #370
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request includes multiple updates to improve schema update and node property handling. The changes refine query construction in schema updates, enhance error wrapping for constraint violations, and update the handling of the ParentNodeID field by switching from value to pointer types. Additionally, the HTTP serialization logic is modified, and new tests have been introduced to validate property schema updates. These modifications ensure that nil values are correctly handled and that updates to node properties are more robust. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SchemaWriter
participant Database
Client->>SchemaWriter: Request schema update
SchemaWriter->>SchemaWriter: Build dynamic predicate array
SchemaWriter->>Database: Execute update query with predicates
Database-->>SchemaWriter: Return result or error
alt Constraint Error
SchemaWriter->>SchemaWriter: Wrap error with fault tag
end
SchemaWriter-->>Client: Return update response
sequenceDiagram
participant HTTPClient
participant NodesHandler
participant SchemaService
HTTPClient->>NodesHandler: NodeUpdatePropertySchema request
NodesHandler->>SchemaService: Process property schema update
SchemaService-->>NodesHandler: Return updated schema list
NodesHandler-->>HTTPClient: Respond with serialized schema list
Possibly related PRs
Poem
✨ 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
CodeRabbit Configuration File (
|
also fix ent schema not having parent node ID as nullable
…d input (status 400)
Southclaws
force-pushed
the
property-schema-edge-case-tests
branch
from
e79a9f9 to
383b57c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/ent/node.go (2)
300-304: Consider resetting the pointer to nil in the else branch.When
value.Validis false, the code currently leavesParentNodeIDuntouched if it was previously non-nil. Consider explicitly settingn.ParentNodeID = nilto avoid stale values in re-used structs.
474-477: Provide a placeholder or explanatory text when ParentNodeID is nil.Currently, the parent_node_id is simply omitted from the output if it's nil. You could add a lightweight placeholder for clarity when the ID is undefined, though this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/resources/library/node_properties/writer.go(3 hunks)app/transports/http/bindings/nodes.go(1 hunks)internal/ent/mutation.go(1 hunks)internal/ent/node.go(4 hunks)internal/ent/node_create.go(1 hunks)internal/ent/node_query.go(2 hunks)internal/ent/schema/node.go(1 hunks)tests/library/properties/property_schema_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/ent/schema/node.go
- internal/ent/mutation.go
- app/resources/library/node_properties/writer.go
- app/transports/http/bindings/nodes.go
- internal/ent/node_create.go
- internal/ent/node_query.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: backend-test
🔇 Additional comments (9)
internal/ent/node.go (2)
43-43: Ensure pointer usage remains consistent throughout the codebase.Converting
ParentNodeIDto a pointer allows representing a nil parent node, which appears correct. However, please verify all internal logic and related references to confirm they're updated accordingly to handle a pointer-based ID.
216-216: Appropriate use of sql.NullScanner for nullable fields.The scanning logic correctly accounts for fields that may be null in the database. This change aligns well with moving to a pointer-based field for
parent_node_id.tests/library/properties/property_schema_test.go (7)
5-5: Good addition of net/http import for status code checksAdding the net/http import allows for clear HTTP status code validation in the new tests, making the assertions more readable with explicit status code constants.
123-175: Well-structured test for root-level node property schema updatesThis test effectively validates that property schema updates correctly propagate between sibling nodes at the root level. The test creates two independent root nodes and verifies that updating one node's schema properly affects its sibling.
Good use of assertion helpers and HTTP status verification to ensure the expected behavior.
153-172: Thorough validation of schema propagation between siblingsThe assertion logic properly verifies that:
- The schema update returns the expected response
- The updated node has the correct schema properties
- The sibling node also receives the same property schema
This effectively tests the sibling update functionality mentioned in the PR summary.
177-272: Comprehensive testing of invalid property schema update scenariosThis test function thoroughly covers edge cases and error handling for property schema updates with three well-designed test cases:
- Duplicate property definitions in a single request
- Conflicting property types in a single request
- Conflicting property type updates in separate requests
Each test properly verifies both the error response (400 Bad Request) and that the transaction is rolled back, preserving data integrity.
191-214: Good validation of duplicate property handlingThis test correctly verifies that when the same property is defined multiple times in a request, the API returns a 400 Bad Request and the transaction is rolled back, leaving the node's properties unchanged.
216-239: Effective test for type conflict handling in single requestThis test case properly verifies the API's response when properties have the same name but different types in a single request. The assertion of the node's unchanged state confirms proper transaction rollback.
241-269: Good testing of sequential conflicting updatesThis test effectively validates that when a property schema is already defined with one type, attempting to update it with a conflicting type fails appropriately with a 400 Bad Request, while preserving the original property configuration.
No description provided.