Node properties #364
Node properties #364
Conversation
- read only so far - exposed on API
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe pull request replaces all calls to the old mapping function (NodeFromModel) with a new one (MapNode(true, nil)) across multiple modules. It introduces a new package for managing node properties and property schemas, adds several new entity types (Property, PropertySchema, PropertySchemaField) with complete CRUD support in the ORM layer, and updates HTTP bindings, services, and web client code to handle node property and schema updates. New tests and UI components are included to verify and display these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant WC as Web Client
participant HB as HTTP Binding (Nodes)
participant NM as Node Mutation Manager
participant SW as Schema/Property Writer
participant DB as Database (ent)
U->>WC: Initiate update on node properties/schema
WC->>HB: Send PATCH request (Update Properties/Schema)
HB->>NM: Forward update request to node mutation manager
NM->>SW: Call SchemaWriter/PropWriter operations
SW->>DB: Execute transactional update (create/update/delete)
DB-->>SW: Return update result
SW-->>NM: Return mutation result
NM-->>HB: Send response with updated properties/schema
HB-->>WC: Return success response to User
Possibly related PRs
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
CodeRabbit Configuration File (
|
Southclaws
force-pushed
the
node-properties
branch
from
f9b7c1e to
0f227d7
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: 10
🧹 Nitpick comments (42)
internal/ent/propertyschemafield_create.go (1)
91-104: Be mindful of the panic usage in SaveX/ExecX.While SaveX/ExecX convenience methods are common in Ent for straightforward data insertion, they panic on error and may not be ideal in robust production code where graceful error handling is desired.
app/resources/library/node_properties/properties_writer.go (1)
25-27: Avoid overshadowing function-level error in defer.
The deferred rollback sets “err = tx.Rollback()”. This risks losing the original error if any was pending. A typical pattern is to store the rollback error in a separate variable or only rollback if no other error has been captured.defer func() { - err = tx.Rollback() + rbErr := tx.Rollback() + if rbErr != nil && err == nil { + err = rbErr + } }()app/resources/library/mapping.go (3)
19-20: Closure-based mapping strategy.
The returned closure captures isRoot and ps. While flexible, consider using a struct approach for clarity if additional parameters are introduced in the future.
31-37: Enforce consistent error handling for parent node.
When the parent node fails to map, you return an empty Node struct. Return nil or a well-defined error to ensure consumers handle the failure path consistently.- return Node{}, err + return Node{}, fault.Wrap(err)
45-49: Consider short-circuit approach for node children.
Check if c.Edges.Nodes is nil or empty before mapping in a loop to save overhead, especially if large node graphs exist.app/resources/library/node_properties/writer.go (1)
199-207: Batch creation of new fields.
You're already looping individually for creates in lines 201-206. Using CreateBulk can help reduce overhead if you have large sets of new fields.app/services/library/node_mutate/options.go (1)
34-38: Enhanced preMutationResult struct
Adding "properties" enables property mutation handling. Consider documenting the usage of this new field.internal/ent/propertyschemafield_update.go (1)
169-175: Improve error messaging for required edge clearing.Currently, the error message is correct but might not be user-friendly. Consider including more details about which edge or field is causing the issue to simplify debugging or user comprehension.
app/transports/http/bindings/nodes.go (1)
346-363: NodeUpdateProperties method.The approach of deserializing the request’s property list into a suitable migration object (pml) is appropriate.
Ensure thorough validation of property fields (like type, sort) in the nodeMutator, especially if they can contain unexpected or invalid data from the request.internal/ent/property_create.go (2)
136-162: Validation checks for required fields and edges.
- Thorough validation ensures that node_id, field_id, and value are set.
- Good enforcement of the node and schema edges to ensure relational integrity.
- Consider more specific error messages, e.g., “Property requires a schema edge” for clarity.
650-677: Approach for field updates in bulk upsert.Using separate method calls (SetValue, UpdateValue, etc.) is flexible. Confirm no concurrency issues arise from large-scale upserts; consider transaction use if required.
internal/ent/migrate/schema.go (2)
595-600: Foreign key linking nodes to property schemas.The foreign key constraint uses “OnDelete: schema.SetNull,” ensuring existing nodes aren’t orphaned if the schema is removed. This can be beneficial for clarity but verify if nullifying node references meets all domain constraints.
746-755: New PropertySchemas table.Only an “id” column is present, which might be sufficient if you rely on referencing the ID alone. However, if you plan to store additional metadata or names for schemas, consider adding fields like “name” or “description.”
internal/ent/client.go (2)
93-98: Add docstring for clarity.
The newly introduced fields for Property clients in the main Client struct are correct and follow the ent pattern. Consider adding docstrings or comments for each field to clarify their usage.
4642-4806: PropertySchemaFieldClient is consistent with ent patterns, but consider naming.
The entity naming is correct from a technical standpoint; however, “PropertySchemaField” may be lengthy and could be simplified if appropriate for the domain.internal/ent/mutation.go (1)
20595-22280: New mutation types for property, property schema, and property schema field.
All new methods (Set/Clear/Reset) follow the ent design. Consider adding an “updated_at” field if tracking last modification time is desired.internal/ent/schema/node_property_schema.go (2)
8-10: Add documentation for the PropertySchema struct.Consider adding a documentation comment explaining the purpose and usage of this schema.
+// PropertySchema represents a schema definition for node properties. type PropertySchema struct { ent.Schema }
20-25: Consider adding cardinality constraints to edges.The edges lack cardinality constraints. Consider if these relationships should be unique or required.
func (PropertySchema) Edges() []ent.Edge { return []ent.Edge{ - edge.To("node", Node.Type), - edge.To("fields", PropertySchemaField.Type), + edge.To("node", Node.Type).Unique(), + edge.To("fields", PropertySchemaField.Type).Required(), } }web/src/api/openapi-schema/property.ts (1)
15-21: Consider adding runtime type validation.While TypeScript provides compile-time type safety, consider adding runtime validation using libraries like Zod or io-ts for the
Propertyinterface, especially for the optionalvaluefield which could be undefined.internal/ent/schema/node_property_schema_field.go (2)
19-26: Add field validation and documentation.Consider adding validation rules and documentation for the fields:
name: Add validation for allowed characters and lengthtype: Add validation for allowed property typessort: Add validation for sort order formatExample enhancement:
func (PropertySchemaField) Fields() []ent.Field { return []ent.Field{ - field.String("name"), + field.String("name"). + Match(regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)). + MinLen(1). + MaxLen(64). + Comment("Name of the property field"), - field.String("type"), + field.String("type"). + Match(regexp.MustCompile(`^(string|number|boolean|array|object)$`)). + Comment("Type of the property field"), - field.String("sort"), + field.String("sort"). + Match(regexp.MustCompile(`^[0-9]+$`)). + Comment("Sort order of the property field"), field.String("schema_id").GoType(xid.ID{}), } }
40-45: Consider adding a composite index for efficient sorting.Since the schema includes a
sortfield, consider adding a composite index withschema_idandsortfor efficient ordering of fields within a schema.func (PropertySchemaField) Indexes() []ent.Index { return []ent.Index{ index.Fields("schema_id", "name").Unique(), index.Fields("name"), + index.Fields("schema_id", "sort"), } }web/src/api/openapi-schema/propertyMutation.ts (1)
15-26: Clarify documentation for optional fields.The current documentation doesn't clearly specify when
typeandsortare required for new properties. Consider enhancing the documentation to explicitly state:
typeis required when creating a new propertysortis required when creating a new property or when reordering existing propertiesweb/src/api/openapi-schema/propertySchemaMutableProps.ts (1)
26-26: Consider using a more specific type for thesortfield.The
sortfield is currently typed asstring, which might be too permissive. Consider using a more specific type (e.g., numeric string, enum) to ensure valid sorting values.app/resources/library/properties_test.go (1)
83-86: Consider using constants for test data.The hardcoded values like "69", "legendary", "420" could be defined as constants to improve maintainability and make the test intent clearer.
+const ( + testStrength = "69" + testRarity = "legendary" + testDamage = "420" +) pml := PropertyMutationList{ - {Name: "strength", Value: "69"}, - {Name: "rarity", Value: "legendary"}, - {Name: "damage", Value: "420"}, + {Name: "strength", Value: testStrength}, + {Name: "rarity", Value: testRarity}, + {Name: "damage", Value: testDamage}, }web/src/components/library/LibraryPagePropertyTable/LibraryPagePropertyTable.tsx (2)
31-71: Enhance accessibility of the property table.Consider adding ARIA attributes to improve accessibility:
-<styled.dl display="table" borderCollapse="collapse"> +<styled.dl + display="table" + borderCollapse="collapse" + role="table" + aria-label="Node properties" +>-<HStack key={p.name} display="table-row"> +<HStack + key={p.name} + display="table-row" + role="row" +>-<styled.dt +<styled.dt + role="cell" + aria-label="Property name"-<styled.dd +<styled.dd + role="cell" + aria-label="Property value"
27-73: Consider adding error boundary.The component could benefit from an error boundary to gracefully handle rendering errors, especially when dealing with potentially invalid property values.
Would you like me to generate an error boundary implementation for this component?
tests/library/properties/property_schema_test.go (1)
21-120: Consider enhancing test coverageWhile the test implementation is solid, consider adding:
- Table-driven tests for schema validation
- Test cases for invalid inputs (e.g., invalid types, duplicate names)
- Edge cases (empty schema names, very long values)
app/resources/library/node_querier/node_querier.go (2)
10-10: Consider using dependency injection for database connections.The addition of a raw SQL connection alongside the ent client suggests a need for direct SQL queries. Consider using dependency injection to make the database dependencies more explicit and testable.
Also applies to: 24-24, 28-29
182-188: Consider error handling for property schema mapping.The raw SQL query execution could benefit from more robust error handling:
- Add context timeout
- Handle SQL-specific errors
- Add logging for debugging
Apply this diff to improve error handling:
propSchema := library.PropertySchemaQueryRows{} + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + err = q.raw.SelectContext(ctx, &propSchema, nodePropertiesQuery, col.ID.String()) if err != nil { + if err == sql.ErrNoRows { + return nil, fault.Wrap(err, fctx.With(ctx, fctx.With(ctx, "node_id", col.ID))) + } return nil, fault.Wrap(err, fctx.With(ctx)) }app/resources/library/properties.go (1)
195-201: Optimize sorting operations.Consider pre-allocating slices and using a more efficient sorting approach for large datasets.
Apply this diff to optimize the sorting:
+ // Pre-allocate slices to avoid reallocation + sortedFields := make([]*Property, len(fields)) + copy(sortedFields, fields) + sortedSchemaFields := make([]*PropertySchemaField, len(schema.Fields)) + copy(sortedSchemaFields, schema.Fields) + - slices.SortFunc(fields, func(i, j *Property) int { + slices.SortFunc(sortedFields, func(i, j *Property) int { return strings.Compare(i.Field.Sort, j.Field.Sort) }) - slices.SortFunc(schema.Fields, func(i, j *PropertySchemaField) int { + slices.SortFunc(sortedSchemaFields, func(i, j *PropertySchemaField) int { return strings.Compare(i.Sort, j.Sort) }) + + fields = sortedFields + schema.Fields = sortedSchemaFieldsweb/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (1)
216-239: Enhance responsive layout with additional breakpoints.Consider adding medium and large breakpoints for better responsiveness across different screen sizes.
Apply this diff to enhance the responsive layout:
<HStack w="full" flexDirection={{ base: "column-reverse", sm: "row", + md: "row", + lg: "row" }} alignItems={{ base: "start", sm: "center", + md: "center", + lg: "center" }} + gap={{ + base: "2", + sm: "4", + md: "6", + lg: "8" + }} >app/resources/library/node_traversal/db.go (1)
68-68: Document MapNode function parameters.The
MapNode(true, nil)function call includes boolean and nil parameters whose purpose is not immediately clear. Consider adding comments to explain these parameters or using named parameters for better clarity.Apply this diff to improve code clarity:
+// MapNode parameters: +// - includeProperties: true to include property data +// - propertySchemas: optional schema overrides, nil for default -nodes, err := dt.MapErr(cs, library.MapNode(true, nil)) +nodes, err := dt.MapErr(cs, library.MapNode( + includeProperties: true, + propertySchemas: nil, +))Also applies to: 214-214
app/transports/http/bindings/openapi_rbac/mapping.go (1)
393-399: Consider adding detailed permission documentation.The "See NOTE." comment suggests that additional documentation is needed. Consider documenting:
- The rationale for allowing these operations without specific permissions
- Any planned future permission requirements
- Reference to the relevant documentation or design decisions
tests/library/properties/property_test.go (4)
93-104: Consider adding validation for schema creation responses.While the schema setup is correct, the test only checks for errors but doesn't validate the returned schema structure.
Add assertions to verify the returned schema structure:
res, err := cl.NodeUpdateChildrenPropertySchemaWithResponse(ctx, parentslug, openapi.NodeUpdateChildrenPropertySchemaJSONRequestBody{ {Name: "weight", Type: "number", Sort: "1"}, {Name: "kind", Type: "string", Sort: "2"}, {Name: "added", Type: "timestamp", Sort: "3"}, }) tests.Ok(t, err, res) +a.Equal(200, res.StatusCode()) +a.Len(res.JSON200.Properties, 3) +a.Equal("weight", res.JSON200.Properties[0].Name) +a.Equal("number", res.JSON200.Properties[0].Type)
106-153: Add test cases for edge cases.The test cases cover basic error handling and success scenarios but miss some important edge cases.
Consider adding test cases for:
- Invalid property types
- Invalid property values (e.g., non-numeric value for "number" type)
- Updating non-existent properties
- Concurrent property updates
216-259: Consider enhancing field ordering test coverage.While the test verifies basic field ordering, it could be more comprehensive.
Consider adding test cases for:
- Multiple reordering operations
- Reordering with new field additions
- Reordering with field removals
- Invalid sort values
361-380: Enhance schema verification.The schema verification could be more thorough.
Consider adding assertions for:
- Schema inheritance at each level of the hierarchy
- Schema field attributes (type, sort order) consistency
- Schema updates propagation to child nodes
internal/ent/node/node.go (2)
107-120: Consider adding indexes for property schema relationships.While the table definitions are correct, consider adding indexes on foreign key columns (
node_id,property_schema_id) to optimize query performance, especially for large datasets.
485-498: Consider enhancing error handling in step functions.While the step functions are correctly implemented, consider adding more specific error messages and validation checks to improve debugging and maintainability.
web/src/api/openapi-client/nodes.ts (2)
327-337: Consider adding retry mechanism for property schema updates.The property schema update implementation is solid, but network failures could leave schemas in an inconsistent state. Consider implementing a retry mechanism with exponential backoff for better reliability.
Also applies to: 339-349
405-409: Add input validation for property updates.Consider adding client-side validation for property values before sending the update request to ensure data consistency and reduce server load.
Also applies to: 410-420
internal/ent/propertyschema_query.go (1)
507-545: Consider query optimization strategies.While the query builder implementation is solid, consider:
- Adding result caching for frequently accessed schemas
- Implementing query result pagination
- Adding query timeout handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (85)
app/resources/collection/item.go(1 hunks)app/resources/library/mapping.go(1 hunks)app/resources/library/node.go(1 hunks)app/resources/library/node_properties/properties_writer.go(1 hunks)app/resources/library/node_properties/writer.go(1 hunks)app/resources/library/node_querier/node_querier.go(7 hunks)app/resources/library/node_search/search.go(1 hunks)app/resources/library/node_traversal/db.go(2 hunks)app/resources/library/properties.go(1 hunks)app/resources/library/properties_test.go(1 hunks)app/resources/link/link.go(1 hunks)app/resources/resources.go(2 hunks)app/resources/tag/tag.go(1 hunks)app/services/library/node_mutate/mutate.go(5 hunks)app/services/library/node_mutate/options.go(4 hunks)app/services/library/node_mutate/update.go(1 hunks)app/transports/http/bindings/nodes.go(8 hunks)app/transports/http/bindings/openapi_rbac/mapping.go(1 hunks)app/transports/http/bindings/openapi_rbac/openapi_rbac_gen.go(2 hunks)internal/ent/client.go(11 hunks)internal/ent/ent.go(2 hunks)internal/ent/er.html(3 hunks)internal/ent/hook/hook.go(1 hunks)internal/ent/migrate/schema.go(5 hunks)internal/ent/mutation.go(25 hunks)internal/ent/node.go(12 hunks)internal/ent/node/node.go(7 hunks)internal/ent/node/where.go(3 hunks)internal/ent/node_create.go(7 hunks)internal/ent/node_query.go(9 hunks)internal/ent/node_update.go(9 hunks)internal/ent/predicate/predicate.go(1 hunks)internal/ent/property.go(1 hunks)internal/ent/property/property.go(1 hunks)internal/ent/property/where.go(1 hunks)internal/ent/property_create.go(1 hunks)internal/ent/property_delete.go(1 hunks)internal/ent/property_query.go(1 hunks)internal/ent/property_update.go(1 hunks)internal/ent/propertyschema.go(1 hunks)internal/ent/propertyschema/propertyschema.go(1 hunks)internal/ent/propertyschema/where.go(1 hunks)internal/ent/propertyschema_create.go(1 hunks)internal/ent/propertyschema_delete.go(1 hunks)internal/ent/propertyschema_query.go(1 hunks)internal/ent/propertyschema_update.go(1 hunks)internal/ent/propertyschemafield.go(1 hunks)internal/ent/propertyschemafield/propertyschemafield.go(1 hunks)internal/ent/propertyschemafield/where.go(1 hunks)internal/ent/propertyschemafield_create.go(1 hunks)internal/ent/propertyschemafield_delete.go(1 hunks)internal/ent/propertyschemafield_query.go(1 hunks)internal/ent/propertyschemafield_update.go(1 hunks)internal/ent/runtime.go(2 hunks)internal/ent/schema/node.go(2 hunks)internal/ent/schema/node_property.go(1 hunks)internal/ent/schema/node_property_schema.go(1 hunks)internal/ent/schema/node_property_schema_field.go(1 hunks)internal/ent/tx.go(2 hunks)tests/library/properties/property_schema_test.go(1 hunks)tests/library/properties/property_test.go(1 hunks)web/src/api/openapi-client/nodes.ts(3 hunks)web/src/api/openapi-schema/index.ts(2 hunks)web/src/api/openapi-schema/nodeInitialProps.ts(2 hunks)web/src/api/openapi-schema/nodeUpdateChildrenPropertySchemaBody.ts(1 hunks)web/src/api/openapi-schema/nodeUpdateChildrenPropertySchemaOKResponse.ts(1 hunks)web/src/api/openapi-schema/nodeUpdatePropertiesBody.ts(1 hunks)web/src/api/openapi-schema/nodeUpdatePropertiesOKResponse.ts(1 hunks)web/src/api/openapi-schema/nodeWithChildrenAllOf.ts(1 hunks)web/src/api/openapi-schema/property.ts(1 hunks)web/src/api/openapi-schema/propertyList.ts(1 hunks)web/src/api/openapi-schema/propertyMutableProps.ts(1 hunks)web/src/api/openapi-schema/propertyMutation.ts(1 hunks)web/src/api/openapi-schema/propertyMutationList.ts(1 hunks)web/src/api/openapi-schema/propertyName.ts(1 hunks)web/src/api/openapi-schema/propertySchema.ts(1 hunks)web/src/api/openapi-schema/propertySchemaList.ts(1 hunks)web/src/api/openapi-schema/propertySchemaMutableProps.ts(1 hunks)web/src/api/openapi-schema/propertySortKey.ts(1 hunks)web/src/api/openapi-schema/propertyType.ts(1 hunks)web/src/api/openapi-schema/propertyValue.ts(1 hunks)web/src/api/openapi-server/nodes.ts(2 hunks)web/src/components/library/LibraryPagePropertyTable/LibraryPagePropertyTable.tsx(1 hunks)web/src/components/library/LibraryPageTagsList/LibraryPageTagsList.tsx(0 hunks)web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- web/src/components/library/LibraryPageTagsList/LibraryPageTagsList.tsx
✅ Files skipped from review due to trivial changes (6)
- web/src/api/openapi-schema/propertyList.ts
- web/src/api/openapi-schema/propertyName.ts
- web/src/api/openapi-schema/propertyType.ts
- web/src/api/openapi-schema/nodeUpdatePropertiesBody.ts
- web/src/api/openapi-schema/propertyValue.ts
- web/src/api/openapi-schema/propertySchemaList.ts
🔇 Additional comments (192)
internal/ent/property/where.go (3)
1-2: Generated code - manual modifications should be avoided.This is an auto-generated file by the ent framework. Any manual changes to this file will be overwritten when the code is regenerated.
319-363: Well-structured edge predicates implementation.The edge predicates (
HasNode,HasNodeWith,HasSchema,HasSchemaWith) are correctly implemented using SQL graph operations. The code properly handles both simple edge existence checks and complex filtering with additional predicates.
1-379: Comprehensive and well-organized predicate functions.The code demonstrates:
- Complete coverage of field operations
- Consistent implementation patterns
- Clear documentation
- Proper use of the ent framework's SQL builders
internal/ent/propertyschemafield_create.go (6)
1-4: Generated code acknowledgment.This file is auto-generated by Ent. Direct manual changes here may be overwritten if the schema or Ent configuration changes. If you need custom logic, consider integrating hooks or modifying the Ent schema definitions instead.
119-125: Verify the default ID generation strategy.propertyschemafield.DefaultID() is used if no ID is provided. Ensure that this generator (likely XID-based) is collision-resistant in high-concurrency scenarios.
128-150: Validation approach looks good.The mandatory field checks (name, type, sort, schema) and ID validation are thorough. If additional constraints or edge cases exist (e.g., allowed range of sort values), consider supplementing these validations.
233-266: Effective conflict handling.The OnConflict and OnConflictColumns methods provide flexible support for upserts. This is a useful pattern for handling unique constraints or merges in distributed systems.
472-534: Batch create implementation is well-structured.The PropertySchemaFieldCreateBulk code efficiently sets default values, performs checks, and batches the create operation. This reduces overhead and ensures consistency.
599-726: Bulk upsert patterns.The UpsertBulk methods (e.g., UpdateNewValues, Ignore, DoNothing) provide a complete workflow for resolving conflicts during batch inserts. Verify that these match your intended business logic to handle conflicting rows properly.
web/src/api/openapi-schema/propertySortKey.ts (2)
1-10: LGTM! Standard orval-generated header with clear versioning information.The header provides good documentation about the API's rolling versioning strategy and its rationale.
11-11: Consider adding constraints in the OpenAPI specification.While the
PropertySortKeytype is correctly defined, it's currently very permissive (any string). Consider reviewing the OpenAPI specification to add constraints or enums if there are specific valid sort keys.✅ Verification successful
🌐 Web query:
What are the best practices for defining sort keys in OpenAPI specifications?💡 Result:
When defining sort keys in OpenAPI specifications, these best practices ensure clarity and interoperability: ## Sorting Parameter Design - **Use standardized parameter names** like `sort_by` or `sort` with explicit direction control ([1][5][9]) - Example: `/users?sort_by=-created_at,+email` - Supports multiple fields: `sort_by=desc(last_modified),asc(email)` ## Direction Specification - **Prefix operators** for intuitive syntax: - `+` for ascending (default if omitted) - `-` for descending ([1][5]) - Alternative: Use explicit `asc()`/`desc()` wrappers ([1]) ## Multi-Column Support - **Allow comma-separated lists** for compound sorts: ```yaml parameters: - name: sort_by in: query schema: type: array items: type: string example: ["-created_at", "email"] ``` Ensures stable ordering through sequential application ([1][5]) ## HTTP Method Handling - **GET Requests**: Use URL parameters with encoding for special characters (`%2B` for +) ([5]) - **POST Requests**: Support JSON body syntax: ```json { "sortby": [ {"field": "created", "direction": "desc"}, {"field": "email", "direction": "asc"} ] } ``` ## Documentation Requirements - **Explicitly list sortable fields** in operation descriptions ([9]) - **Indicate default sort order** when no parameter provided: ```yaml description: Returns items sorted by creation date (newest first) by default ``` ## Implementation Considerations - **Validate sortable fields** and return 400 errors for unsupported keys ([5]) - **Stable secondary sorts** (e.g., ID column) to prevent pagination drift ([1]) - **Avoid low-cardinality fields** as primary sort keys without secondary stabilizers ([1]) For OpenAPI spec structure: - **Sort components alphabetically** using tools like openapi-format ([2][8]) - **Maintain parameter order consistency** with required parameters first ([4]) These patterns balance flexibility with implementation stability while supporting common client expectations[1][5][9]. Citations: - 1: https://www.moesif.com/blog/technical/api-design/REST-API-Design-Filtering-Sorting-and-Pagination/ - 2: https://www.npmjs.com/package/openapi-format/v/1.2.2 - 3: https://github.com/go-openapi/spec/issues/24 - 4: https://www.apimatic.io/blog/2022/11/14-best-practices-to-write-openapi-for-better-api-consumption - 5: https://github.com/stac-api-extensions/sort - 6: https://www.getambassador.io/blog/openapi-specification-structure-best-practices - 7: https://liblab.com/blog/a-big-look-at-security-in-openapi - 8: https://github.com/RicoSuter/NSwag/issues/4862 - 9: https://tyk.io/blog/10-essentials-when-creating-an-open-api-specification/ - 10: https://swagger.io/specification/
Action: Verify OpenAPI Specification Constraints for Sort Keys
The current auto-generated code correctly defines PropertySortKey as a string. However, the best practices for defining sort keys in OpenAPI suggest that, when possible, the specification should impose constraints or enumerate valid values. This improves clarity for both API consumers and the implementation by preventing unsupported sort keys.
• Verify if the OpenAPI spec details valid sort keys (e.g., via an enum or constrained schema)
• If specific keys are defined, consider updating the generated type to a union of allowed string literals
• Otherwise, review and potentially improve the spec to enforce a more constrained sort key definitionapp/resources/library/node_properties/properties_writer.go (4)
15-17: Ensure separation of concerns for Writer struct.
Currently, Writer only declares a database client field. If you plan to add more functionalities or track additional writer-specific state later, consider adding method-level or entity-specific options to maintain separation of responsibilities.
19-20: Question the use of Debug mode on transactions.
The call to w.db.Debug() opens a verbose logging mode, which might leak sensitive SQL info in production logs. Verify that this is appropriate for the current environment.
33-50: Validate prop.ID before using OnConflict logic.
Consider verifying that prop.ID is non-empty and the node ID is valid to avoid unintended data integrity issues.
52-55: Check commit outcome thoroughly.
Ensure external observers handle partial updates if commit fails. Potentially returning partial data might lead to confusion.app/resources/library/mapping.go (2)
21-24: Error handling for OwnerOrErr.
OwnerOrErr returns an error if the owner edge isn’t loaded or missing. Ensure the calling code is populating the edges to avoid frequent runtime errors.
93-101: Conditional logic for root/child property assignment.
The isRoot logic is straightforward. If there’s future branching (e.g., different property inheritance patterns), consider extracting to dedicated methods for clarity.app/resources/library/node_properties/writer.go (2)
40-54: Manage potential edge-loading issues.
In CreateForNode, you rely on node.Edges.PropertySchemas. Verify that edges are loaded. If the edge wasn't preloaded, doSchemaUpdates might inadvertently work with nil.
178-184: Prevent unintended deletion of property fields.
Deleting fields that might still be referenced by historical records or other node processes can lead to partial data. Confirm references are fully severed or archived before removal.internal/ent/propertyschemafield.go (13)
1-33: No major issues found in struct definition
All fields, including JSON tags and edges, appear consistent with standard ent usage.
35-45: Edge definitions appear consistent
Definition for "Schema" and "Properties" edges, along with tracking for loaded types, aligns with ent’s best practices.
48-55: Edge retrieval logic is correct
Appropriately handles the “not found” and “not loaded” states for the "Schema" edge.
57-65: Parallel approach for retrieving “Properties” edge
Mirrors the pattern in SchemaOrErr, providing consistent error handling.
66-80: scanValues method is well structured
Dynamically assigns scanner types for each column, following ent conventions.
82-125: Field assignment logic is correct
Accurately handles type assertions and populates struct fields from scanned values.
127-131: Value method is straightforward
Simply delegates the retrieval of dynamically selected values.
133-136: QuerySchema method follows ent conventions
Properly delegates to the generated client for schema queries.
138-141: QueryProperties method is consistent
Uses the ent client to fetch related properties with no apparent issues.
143-148: Update method uses the correct builder
Adheres to ent’s pattern for initiating entity updates.
150-159: Transaction unwrapping logic is standard
Ensures proper entity usage post-transaction.
161-179: Stringer implementation is well-formed
Builds a simple but informative string representation of the entity fields.
181-183: Collection type addition
Defines a slice type that’s consistent with ent naming patterns.app/services/library/node_mutate/options.go (7)
9-10: Additional fault packages imported
Imports "fmsg" and "ftag" to enrich fault error-handling. No issues found.
18-18: New import for node_properties
Introduces property-related logic into the mutation flow.
192-197: Ensuring properties are passed through
Returning "p.Properties" in preMutationResult ties property mutations into subsequent mutation steps.
302-304: Introduction of postMutationResult
Includes an optional property table field to store updates after the main mutation process.
306-309: Default property table initialization
Using an empty PropertyTable if nothing is set ensures consistent handling of post-mutation properties.
310-353: Robust property update logic
Checks for an existing schema, creates one if missing, then adds new fields before updating properties. This two-step approach is clear. Consider potential rollback or partial failure scenarios.
360-370: Map new property mutation
Enforces the requirement that all new property fields have a specified type. This is good validation, though you might consider offering a default or fallback.internal/ent/propertyschema_update.go (24)
1-19: Generated code with ent
Imports and package definition align with typical ent structure.
20-26: PropertySchemaUpdate struct definition
Holds hooks, mutation logic, and modifiers. Standard approach for ent updates.
28-33: Where method
Enables attaching predicates to narrow the update scope. Implementation looks correct.
34-47: Adding node edges
AddNodeIDs and AddNode methods are standard. No issues found.
49-63: Adding field edges
AddFieldIDs and AddFields methods correctly handle new property schema fields by ID or entity.
64-67: Mutation accessor
Allows direct reference to the mutation object if needed. Adheres to ent’s internal patterns.
69-88: Clearing and removing node edges
Correctly manipulates the “node” relationship, ensuring references are cleared or removed as needed.
90-109: Clearing and removing field edges
Same pattern as node edges, providing full removal or clearing logic.
111-124: Save and SaveX
Wraps ent update execution, returning the affected row counts.
125-136: Exec and ExecX
Executes or panics on error, matching other ent builder patterns.
138-142: Modify method
Allows statement-level customization. No issues found.
144-254: sqlSave for multiple updates
Builds SQL update specs dynamically, clearing or adding edges as required. Proper error handling for constraints.
256-263: PropertySchemaUpdateOne struct
Handles single-entity updates with a narrower scope than the bulk updater.
265-278: AddNodeIDs, AddNode
Mirrors the multi-entity logic for single-entity updates, focusing on linking node edges.
280-293: AddFieldIDs, AddFields
Adds or links property schema fields in one-to-many relationships.
295-298: Mutation method
Again, standard ent approach for direct mutation access.
300-319: Clearing and removing node edges
Consistency in clearing or unlinking node references. Looks good.
321-340: Clearing and removing field edges
Provides a similar mechanism for field relationships, paralleling node edges.
342-346: Chaining Where predicates
Useful for filtering single-entity updates.
348-353: Selecting fields
All fields are selected by default, but manual selection is available if needed.
355-367: Save and SaveX
Returns the updated entity or panics on error. Typical for ent.
369-380: Exec and ExecX
Analogous to Save but without returning the updated entity.
382-386: Modify method
Exposes statement modifiers for advanced usage.
388-518: sqlSave for single updates
Similar logic to the multiple-entity variant, with appropriate error handling and column selection.internal/ent/propertyschemafield_update.go (4)
28-32: Ensure comprehensive predicate coverage.While this method appends predicates to the PropertySchemaFieldUpdate builder, consider validating whether all required filters are present or if additional conditions need to be checked before executing the update. This helps prevent unexpected partial updates.
142-145: Good use of hooks with the Save method.The pattern of chaining hooks before calling sqlSave ensures any custom logic or validations can be injected. This approach fosters extensibility and maintainability. No issues found here.
183-188: Check for potential returned nil references.If the node was not found or creation failed, ensure the path handling guarded merges for future expansions. This code looks fine for now, but be mindful of future changes that could introduce edge-case bugs.
278-287: Properly handles constraint errors.This catch block ensures that unique or foreign key violations are translated into a structured ConstraintError. This is a good approach for making error handling more consistent.
app/transports/http/bindings/nodes.go (4)
19-19: New import node_properties.Importing the node_properties package is aligned with the PR objectives of managing property schemas. Ensure that cyclical references are avoided if node_properties also imports from this package.
41-42: Field injection for SchemaWriter.Adding the SchemaWriter field (nsr) is consistent with the new property schema feature. Make sure the lifecycle and potential nil usage of nsr is handled in all consuming methods.
51-52: Constructor updated to include SchemaWriter.Great job updating the NewNodes constructor to accept nsr. Verify that existing calls to NewNodes across the application set this parameter or handle a nil reference.
467-468: Extended NodeWithChildren structure.Adding the properties and childPropertySchema fields provides richer context in the response, aligning perfectly with the new schema system. Good approach.
internal/ent/property_create.go (3)
124-134: Default value logic for created_at and id.This is a safe approach, relying on property.DefaultCreatedAt and property.DefaultID for fallback. Ensure the default generators remain consistent across the codebase (e.g., timezones, ID generation).
164-185: sqlSave method error handling.Catching constraint errors and wrapping them in ConstraintError is a best practice. This approach provides a consistent error reporting mechanism for the caller.
422-426: OnConflict usage requires additional options.If OnConflict is called without specifying how to resolve columns, the code will return an error. Ensure that any usage of PropertyCreate().OnConflict is accompanied by a proper conflict resolution strategy.
internal/ent/property_query.go (9)
1-20: Auto-generated code disclaimer and imports.This code is generated by Ent, so manual modifications risk being overwritten by future schema migrations. The import statements and package declaration look standard for an Ent-generated file.
21-34: Struct definition and fields.The PropertyQuery struct and its fields appear to correctly handle query-related configurations for the Property entity, including predicates, ordering, and eager-loading. This layout is consistent with Ent’s recommended patterns.
36-65: Basic query modifiers (Where, Limit, Offset, Unique, Order).Ent’s typical chainable methods for setting predicates, limits, and ordering are present. There are no immediate issues in logic or usage.
67-109: Relationship query methods (QueryNode, QuerySchema).These functions properly configure steps to execute M2O edges (node → property and schema → property). The usage of sqlgraph steps is standard for linking edges. No concerns noted.
111-280: Retrieval methods for single and multiple records.Methods like First, Only, All, IDs, and their ID variants are conventional Ent patterns. Error handling is adequate, returning NotFoundError and NotSingularError as appropriate.
291-310: Clone method logic.Cloning is implemented carefully, duplicating slices and references. This helps maintain immutability of the original query builder when needed.
312-332: Eager-loading (WithNode, WithSchema).Both methods properly construct secondary queries and store them for later resolution. The approach is consistent with typical Ent eager-loading patterns.
334-609: Aggregate / GroupBy / Select / Additional query configuration.All group-by, select, and aggregate builder patterns are standard. The prepareQuery and sqlAll methods ensure correct application of interceptors, predicates, and ordering. No performance or security issues stand out.
611-706: Final grouping structures and scanning logic.PropertyGroupBy and PropertySelect provide typed builders for grouping and selecting data. The aggregator methods and scan flows align with Ent’s design. Everything looks consistent and complete.
internal/ent/migrate/schema.go (5)
563-563: New column on nodes table.The “property_schema_id” column adds a nullable foreign key reference for node-specific property schemas. This design is flexible but be mindful of cases where a node might not have a corresponding schema; consider whether this reference should be mandatory or optional.
701-745: New Properties table.Columns appear consistent: “id,” “created_at,” “value,” “node_id,” and “field_id.” Enforcing a unique index on (“field_id,” “node_id”) ensures one property per field per node. This is a typical approach for key-value property tables.
757-789: New PropertySchemaFields table.Columns for “id,” “name,” “type,” “sort,” and “schema_id” look adequate for customizing property definitions. The unique index on (“schema_id,” “name”) prevents duplicates. This is well-structured for defining fields that belong to a specific schema.
1118-1120: Registering new tables in the global Tables slice.Including “PropertiesTable,” “PropertySchemasTable,” and “PropertySchemaFieldsTable” ensures migrations generate the correct DDL. This is standard practice in Ent migrations.
1168-1178: Foreign key references to property schemas and fields.Linking “node.property_schema_id” → “property_schemas.id” and “properties.field_id” → “property_schema_fields.id” ensures relationships are enforced. The “NoAction” on delete might be acceptable if you plan to manually handle cleanup of orphaned rows. Confirm that your logic addresses any possible orphan scenarios.
internal/ent/client.go (10)
38-40: New imports for property management look consistent.
These imports align well with the introduction of the new property-based entities (Property, PropertySchema, PropertySchemaField).
139-141: Initialization of Property clients is correct.
This follows the standard ent approach to initialize sub-clients in the main client.
237-265: Transaction struct additions are consistent.
Including the new Property and PropertySchema clients in the Tx struct is correct and ensures transactional integrity for newly added entities.
283-311: Additions to BeginTx are correctly mirroring Tx logic.
These changes ensure all newly introduced clients participate in transactional operations.
345-346: Hook usage for new clients is correctly integrated.
This ensures that all mutation hooks, if any, will apply to Property*, just like other entities.
358-359: Interceptor usage for new clients is correctly integrated.
Similarly, interceptors will also be applied to Property* entities, maintaining consistency across the codebase.
406-411: Mutate method extended to handle new Property-related mutations.
This appropriately covers create, update, and delete operations for the new entities.
4312-4476: PropertyClient implementation is aligned with ent conventions.
The CRUD methods, bulk operations, and relationship queries mirror the standard ent client patterns.
4477-4641: PropertySchemaClient is well structured.
The code adheres to ent’s approach for schema-based entities, exposing coherent CRUD methods and edge queries.
3653-3684: Double-check relationship direction for Node → PropertySchemas.
While the edge definitions for QueryProperties (O2M) and QueryPropertySchemas (M2O) look valid, verify that this is the intended cardinality. If multiple schemas can belong to a single node, O2M might be more suitable.Run the following script to inspect how edges are defined in the repository, ensuring the intended relationship direction is correct:
internal/ent/mutation.go (24)
34-36: New imports look fine.
These additions appear consistent with the new entities (property, propertyschema, propertyschemafield).
55-80: Addition of node types is consistent.
Defining these new node type constants aligns well with existing naming conventions.
15152-15197: New fields in NodeMutation appear correct.
The newly introduced fields (e.g. properties, property_schemas) follow the standard ent mutation pattern.
15728-15777: PropertySchemaID methods are valid.
Set/Clear/Reset for “property_schema_id” is properly implemented following ent conventions.
16242-16335: New edges for properties and property_schemas look good.
Add/Clear/Remove methods align with ent’s edge-handling best practices.
16505-16505: Pre-allocating fields slice with length 15 is acceptable.
This minor capacity change is harmless and can slightly optimize memory usage.
16536-16538: Appending node.FieldPropertySchemaID is correct.
This new condition is well-integrated with the logic that gathers changed fields.
16579-16580: Case for node.FieldPropertySchemaID looks fine.
Properly retrieves the property schema ID through the new method.
16618-16619: OldPropertySchemaID retrieval is correct.
Fetching the old value matches ent’s typical pattern.
16707-16713: Handling property_schema_id in SetField is consistent.
Gracefully checks type and sets the new value.
16787-16789: Handles FieldCleared for property_schema_id.
Clears the field as expected in ent’s approach.
16828-16830: ClearField case is properly extended.
Supports clearing property_schema_id as needed.
16878-16880: ResetField case for property_schema_id is valid.
Resets the field changes along ent lines.
16899-16899: Allocating edges slice with capacity 11 is fine.
Slight efficiency improvement for the edge list.
16918-16923: New edges checks for properties and property_schemas are correct.
Consistently appends edges if they exist.
16970-16979: Case for node.EdgeProperties and node.EdgePropertySchemas is well-implemented.
Properly returns sets of IDs.
17002-17003: Expanding slice capacity for removed edges.
No concerns with the new initial capacity of 11.
17012-17014: Recognition of removedproperties is correct.
Edge removal logic is consistent.
17046-17051: Property IDs removal logic is appropriate.
Collects IDs from removedproperties as expected.
17070-17071: Edge slice capacity change in ClearedEdges is fine.
Slight optimization with zero functional downsides.
17089-17094: Checking clearedproperties and clearedproperty_schemas is consistent.
No issues with the approach to track cleared edges.
17123-17126: Returning cleared status for property_schemas.
This new case is properly integrated.
17150-17152: ClearPropertySchemas edge method is properly added.
Complies with ent’s “clear unique edge” convention.
17182-17187: Reset methods for properties and property_schemas.
Resets all changes as expected.internal/ent/schema/node_property_schema.go (1)
16-18: Verify if fields are intentionally omitted.The schema currently has no fields defined. Consider if fields like name, description, or metadata should be added to enhance the schema's functionality.
web/src/api/openapi-schema/nodeUpdateChildrenPropertySchemaOKResponse.ts (1)
1-14: LGTM! Generated type definition looks good.The type definition is correctly structured and follows TypeScript best practices.
web/src/api/openapi-schema/propertyMutableProps.ts (1)
12-18: LGTM! Important note about replace-all behavior.The interface is correctly defined. Pay attention to the documentation note that properties are replace-all and not merged with existing ones, as this is crucial for API consumers.
web/src/api/openapi-schema/propertySchema.ts (1)
14-19: LGTM! Property schema interface looks good.The interface is well-structured with appropriate property types.
web/src/api/openapi-schema/nodeWithChildrenAllOf.ts (1)
11-12: LGTM! Node with children type extension looks good.The new properties and their types are correctly defined, aligning with the PR objectives of adding node property support.
Also applies to: 15-17
app/resources/tag/tag.go (1)
40-40: Verify the implications of MapNode parameters.The change from
NodeFromModeltoMapNode(true, nil)looks good, but let's verify the meaning of these parameters.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find the definition and usage patterns of MapNode to understand parameter implications. # Search for MapNode definition ast-grep --pattern $'func MapNode($params) { $$$ }' # Search for other MapNode usages to verify parameter consistency rg -A 2 'MapNode\(.*\)'Length of output: 2975
MapNode Parameter Verification
The function signature for MapNode is defined as follows:
func MapNode(isRoot bool, ps *PropertySchemaTable) func(c *ent.Node) (*Node, error)
This indicates two parameters:
- isRoot (bool): A flag that, when true, indicates the node should be mapped using root node logic. This often affects how relationships or ownership edges are processed.
- ps (*PropertySchemaTable): A pointer to a property schema used for additional configuration or mapping. When nil is passed, it signifies that no extra property schema mapping is required.
Across the codebase, usages such as library.MapNode(true, nil) seen in modules like node_traversal, node_querier, node_search, and link are consistent with this behavior. In these contexts, marking a node as a root (with true) and omitting a property schema (with nil) are deliberate, supporting the intended mapping strategy.
Thus, the change from NodeFromModel to MapNode(true, nil) is valid with the parameters serving their documented purposes.
web/src/api/openapi-schema/nodeInitialProps.ts (1)
16-16: LGTM!The addition of the optional
propertiesfield and its corresponding import are well-structured and align with the PR objectives.Also applies to: 30-30
app/resources/link/link.go (1)
70-70: LGTM!The change to use
MapNode(true, nil)is consistent with the changes in other files and maintains proper error handling.internal/ent/schema/node.go (2)
27-27: LGTM! Well-structured schema field definition.The optional property schema ID field is correctly defined with the appropriate Go type.
64-68: LGTM! Well-structured edge definitions.The property-related edges are correctly defined with appropriate constraints:
properties: Many-to-many relationship allowing multiple properties per nodeproperty_schemas: One-to-one relationship with proper field reference and uniquenessapp/resources/library/node.go (1)
48-49: LGTM! Well-structured optional fields.The new property-related fields are correctly defined using
opt.Optionalwrapper, maintaining consistency with other optional fields in the struct.app/resources/library/properties_test.go (1)
13-19: Consider adding type validation tests.The test cases verify counts but don't validate if the property types (string, number, timestamp) are correctly handled.
Would you like me to generate additional test cases that verify type validation?
internal/ent/property_delete.go (1)
42-57: Consider adding cascade delete handling.The
sqlExecmethod handles constraint errors but doesn't specify cascade delete behavior. This could lead to orphaned records if properties have relationships with other entities.Let's check for potential relationships:
app/services/library/node_mutate/update.go (1)
57-61: 🛠️ Refactor suggestionClarify error handling strategy.
The TODO comment suggests uncertainty about error handling. Consider:
- Whether this error should be logged instead of returned
- What specific error cases need to be handled
app/resources/library/node_search/search.go (1)
102-102: Verify MapNode parameters.The change from
NodeFromModeltoMapNode(true, nil)needs verification:
- What does the
trueparameter control?- Are there cases where
nilshould be replaced with specific options?✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other MapNode usages to understand parameter patterns rg "MapNode\(" -A 2Length of output: 2916
MapNode Parameters Verification
• The first parameter (true) indicates that the node being mapped should be treated as a root node. This likely triggers root-specific mapping logic within MapNode.
• The second parameter is a pointer to a PropertySchemaTable. Passing nil is appropriate when no custom property schema mappings are required. In other parts of the code (e.g., in node_querier), a valid property schema (propSchema.Map()) is provided when additional mapping details are needed.These usages are consistent across the codebase, and using MapNode(true, nil) in search.go is correct.
internal/ent/propertyschemafield_delete.go (3)
1-1: LGTM! Generated code.This is an auto-generated file by the ent framework, which follows best practices for database operations.
52-57: Proper error handling for constraint violations.The code correctly wraps constraint errors with custom
ConstraintErrortype and sets the mutation as done, ensuring proper cleanup.
71-81: Robust error handling in single entity deletion.The code properly handles three cases:
- Execution errors
- Not found cases (returns NotFoundError)
- Successful deletion
app/resources/collection/item.go (1)
114-114: Verify the impact of the new mapping function.The change from
library.NodeFromModel(p)tolibrary.MapNode(true, nil)(p)introduces new parameters. Ensure that:
trueparameter is appropriate for this contextnilparameter doesn't skip any necessary property mappings✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of MapNode to verify parameter consistency rg -A 2 "MapNode\(true, nil\)"Length of output: 1552
Consistent Usage Confirmed
The search reveals that
library.MapNode(true, nil)is used consistently across several modules, including in node querier, node search, node traversal, link, and tag. This indicates that passingtrueandnilis an intentional and standardized approach for mapping nodes across the codebase, and no additional property mappings are skipped unintentionally.internal/ent/predicate/predicate.go (1)
66-74: LGTM! New predicates follow consistent pattern.The new predicate types for Property, PropertySchema, and PropertySchemaField follow the established pattern and naming convention.
internal/ent/propertyschema/propertyschema.go (3)
11-36: LGTM! Well-defined schema constants.Constants are well-organized and documented, clearly defining:
- Table names and relationships
- Edge definitions
- Column names
95-108: LGTM! Proper edge relationship definitions.The step builders correctly define:
- One-to-many relationship between PropertySchema and Node
- One-to-many relationship between PropertySchema and Fields
53-58: Verify ID generation and validation.The code declares but doesn't implement:
- DefaultID function for generating xid.ID
- IDValidator function for validating IDs
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the implementation of these functions rg "DefaultID.*=.*func\(\).*xid\.ID" -A 3 rg "IDValidator.*=.*func\(string\).*error" -A 3Length of output: 16323
ID generation and validation functions are implemented via runtime initialization.
The DefaultID and IDValidator functions declared in internal/ent/propertyschema/propertyschema.go are assigned their implementations in internal/ent/runtime.go. Specifically, for the propertyschema module:
- DefaultID is set as propertyschema.DefaultID = propertyschemaDescID.Default.(func() xid.ID)
- IDValidator is defined as a closure that retrieves validators from propertyschemaDescID.Validators
This confirms that the functionality for ID generation and validation is correctly implemented in the codebase.
app/services/library/node_mutate/mutate.go (2)
37-37: LGTM! Properties field additionThe new
Propertiesfield in thePartialstruct enables optional property mutations using theopt.Optionalwrapper.
55-56: LGTM! Property management dependenciesThe Manager struct is properly enhanced with schema and property writing capabilities through dependency injection.
Also applies to: 73-74, 90-91
internal/ent/propertyschema/where.go (1)
1-116: Generated code looks goodThis is auto-generated code by ent that provides comprehensive predicate functions for querying PropertySchema entities. The generated code follows ent's patterns and best practices.
app/resources/resources.go (1)
29-29: LGTM! Node properties integrationThe node_properties package is properly integrated into the dependency injection system following the established pattern.
Also applies to: 92-92
tests/library/properties/property_schema_test.go (1)
122-133: LGTM! Helper function implementationThe
matchSchemahelper function is well-implemented with proper test helper designation and clear assertions.internal/ent/propertyschema.go (1)
1-135: LGTM!The generated code follows ent's standard patterns and provides a robust structure for managing property schemas.
internal/ent/property/property.go (1)
1-131: LGTM!The generated code provides a well-structured foundation for property management with clear table relationships and ordering options.
internal/ent/propertyschemafield/propertyschemafield.go (1)
1-134: LGTM!The generated code establishes a clear structure for property schema fields with appropriate relationships and ordering capabilities.
app/resources/library/node_querier/node_querier.go (2)
48-94: Complex CTE query might benefit from documentation and testing.The CTE query for retrieving node properties is complex and handles both sibling and child properties. Consider:
- Adding documentation to explain the query's purpose and structure
- Adding test cases to verify the query's behavior
- Moving the query to a separate SQL file for better maintainability
Would you like me to help create test cases to verify the query's behavior?
157-161: Verify eager loading performance impact.The addition of WithProperties() and WithPropertySchemas() with nested WithFields() could impact query performance with large datasets.
Run this script to analyze the generated SQL queries:
Also applies to: 173-173
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the SQL queries generated by ent for property loading # Test: Search for WithProperties and WithPropertySchemas usage rg -A 5 'WithProperties|WithPropertySchemas' # Test: Search for potential N+1 query patterns in property loading ast-grep --pattern 'WithProperties().WithPropertySchemas()'Length of output: 4493
Eager Loading Review Verified
The shell script results confirm that the eager loading calls (WithProperties, WithPropertySchemas with nested WithFields) are being used consistently across the codebase. In our analysis, there’s no evidence of an N+1 query pattern triggered by this chaining—the implementation in the ent layer (see internal/ent/node_query.go) builds the queries in a batched manner rather than executing separate queries for each nested property. While it's always a good idea to benchmark under heavy loads, there’s no immediate performance issue from a static review of the SQL-generation patterns.
internal/ent/property.go (1)
1-189: Generated code looks good!The generated Property model includes all necessary fields, proper error handling for edge relationships, and type-safe SQL value scanning. The implementation follows ent's best practices.
internal/ent/tx.go (1)
55-60: LGTM! The property-related client fields and initialization are properly integrated.The changes consistently follow the existing patterns for client management in the transaction struct.
Also applies to: 221-223
web/src/api/openapi-server/nodes.ts (1)
202-268: LGTM! The new API endpoints are well-documented and properly implemented.The implementation follows RESTful conventions and includes clear documentation about the behavior and side effects of property schema changes.
app/transports/http/bindings/openapi_rbac/openapi_rbac_gen.go (1)
101-102: LGTM! The permission handling is properly integrated.The new operations are consistently implemented in both the interface and the permission mapping function.
Also applies to: 314-317
tests/library/properties/property_test.go (3)
22-91: LGTM! Well-structured test setup.The test setup creates a comprehensive hierarchical structure with parent, children, and grandchildren nodes, providing good coverage for testing property inheritance and updates.
158-208: LGTM! Good test setup for field ordering.The test setup correctly establishes the initial state for testing field ordering functionality.
264-360: LGTM! Comprehensive test for schema inheritance.The test setup effectively creates a multi-level hierarchy to test schema inheritance.
internal/ent/property_update.go (1)
1-485: LGTM! Auto-generated code by ent framework.The code follows ent's standard patterns for update operations.
Verify the ent schema that generated this code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the ent schema for Property entity ast-grep --pattern $'// Property holds the schema definition for the Property entity. type Property struct { ent.Schema $$$ }'Length of output: 131
I'll run an additional script to search for the ent schema definition of the Property entity in the schema folder.
🏁 Script executed:
#!/bin/bash echo "Searching for the 'Property holds the schema definition' comment in the ent schema directory..." rg -n "Property holds the schema definition" internal/ent/schema echo "Searching for the 'type Property struct {' line in the ent schema directory..." rg -n "type Property struct {" internal/ent/schemaLength of output: 607
🏁 Script executed:
#!/bin/bash echo "Listing all Go files in internal/ent/schema..." find internal/ent/schema -type f -name "*.go" echo "Searching for 'type Property struct {' in internal/ent/schema (with escaped brace)..." rg -n "type Property struct \{" internal/ent/schemaLength of output: 1429
Ent schema verified from internal/ent/schema/node_property.go.
The Property schema is defined in "internal/ent/schema/node_property.go" (starting at line 11), confirming that the generated update code adheres to ent's standard conventions.
internal/ent/propertyschemafield/where.go (1)
1-402: LGTM! Auto-generated code by ent framework.The code follows ent's standard patterns for query predicates.
Verify the ent schema that generated this code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the ent schema for PropertySchemaField entity ast-grep --pattern $'// PropertySchemaField holds the schema definition for the PropertySchemaField entity. type PropertySchemaField struct { ent.Schema $$$ }'Length of output: 164
🏁 Script executed:
#!/bin/bash # Searching for the schema definition of PropertySchemaField using rg. # This should help us locate where "type PropertySchemaField struct" is defined. echo "Searching for schema definition 'type PropertySchemaField struct' in the repository:" rg -n "type PropertySchemaField struct" . # As an alternative, search for files likely containing the ent schema for PropertySchemaField, # e.g., files with "propertyschemafield.go" in their name. echo "Searching for file names matching 'propertyschemafield.go':" fd -t f -e go "propertyschemafield.go"Length of output: 733
🏁 Script executed:
#!/bin/bash # Check for the Fields() method in the candidate ent schema file. echo "Searching for 'func (PropertySchemaField) Fields(' in internal/ent/schema/node_property_schema_field.go:" rg -n "func \(PropertySchemaField\) Fields(" internal/ent/schema/node_property_schema_field.go # Check for the Edges() method in the candidate ent schema file. echo "Searching for 'func (PropertySchemaField) Edges(' in internal/ent/schema/node_property_schema_field.go:" rg -n "func \(PropertySchemaField\) Edges(" internal/ent/schema/node_property_schema_field.goLength of output: 884
The new script uses fixed-string search to avoid regex escape issues. Once executed, verifying that the ent schema in “internal/ent/schema/node_property_schema_field.go” defines the Fields() and Edges() methods will confirm that the generated “where.go” predicates are indeed produced from the schema.
If these methods are present, it confirms that the ent schema matches the auto-generated code seen in “internal/ent/propertyschemafield/where.go”, thereby following ent’s standard patterns.
Based on the intended behavior of ent’s code generation, the review comment’s concern is resolved.
Final Verification: The ent schema exists and is correctly generating the predicate functions.
🏁 Script executed:
#!/bin/bash # Use fixed string search (with -F flag) to check for the Fields() and Edges() methods in the ent schema file that likely generated the predicates. echo "Searching for 'func (PropertySchemaField) Fields(' in internal/ent/schema/node_property_schema_field.go:" rg -F -n "func (PropertySchemaField) Fields(" internal/ent/schema/node_property_schema_field.go echo "Searching for 'func (PropertySchemaField) Edges(' in internal/ent/schema/node_property_schema_field.go:" rg -F -n "func (PropertySchemaField) Edges(" internal/ent/schema/node_property_schema_field.goLength of output: 746
Verification of ent Schema Generation
The ent schema is correctly defined in “internal/ent/schema/node_property_schema_field.go” with the expected implementation of the Fields() and Edges() methods. This confirms that the generated predicate functions in “internal/ent/propertyschemafield/where.go” are produced following ent’s standard patterns.
internal/ent/propertyschema_create.go (1)
1-514: LGTM! Auto-generated code by ent framework.The code follows ent's standard patterns for create operations.
Verify the ent schema that generated this code:
internal/ent/hook/hook.go (1)
240-274: LGTM! The new property hooks follow the established pattern.The implementation of
PropertyFunc,PropertySchemaFunc, andPropertySchemaFieldFuncis consistent with the existing hook pattern, making the code maintainable and predictable.web/src/api/openapi-schema/index.ts (1)
273-278: LGTM! New exports are well-organized.The new property-related exports are properly organized alphabetically, maintaining consistency with the existing codebase.
Also applies to: 345-356
internal/ent/node.go (1)
46-47: LGTM! Property schema integration is well-implemented.The changes properly integrate property schema functionality into the Node entity:
- Added
PropertySchemaIDfield with appropriate JSON tags- Added property-related edges in
NodeEdges- Added query methods following the established pattern
Also applies to: 76-79, 387-395
internal/ent/ent.go (1)
34-36: LGTM! Property validation is properly integrated.The changes correctly integrate property-related validation:
- Added necessary imports
- Added column validation for property tables in the
columnCheckmapAlso applies to: 121-123
internal/ent/node/node.go (1)
39-40: LGTM! Property schema constants are well-defined.The new constants for property schema fields and edges follow the established naming pattern and are correctly defined.
Also applies to: 61-64
internal/ent/propertyschemafield_query.go (1)
22-35: LGTM! Query builder implementation is consistent.The PropertySchemaFieldQuery implementation follows the same patterns as PropertySchemaQuery, ensuring consistency across the codebase.
internal/ent/node/where.go (1)
109-112: LGTM! Property schema predicates are correctly generated.The new predicates for the
property_schema_idfield follow the standard Ent framework pattern and are consistent with other predicates in the file.internal/ent/runtime.go (1)
804-884: LGTM! Property-related mixins are correctly generated.The new mixins for Property, PropertySchema, and PropertySchemaField entities follow the standard Ent framework pattern and are consistent with other mixins in the file.
internal/ent/node_query.go (1)
216-258: LGTM! Property-related query methods are correctly generated.The new query methods for properties and property schemas follow the standard Ent framework pattern and are consistent with other query methods in the file.
internal/ent/node_create.go (5)
20-21: LGTM: New imports for property management.The imports for property and propertyschema packages are correctly added to support the new functionality.
150-162: LGTM: Property schema ID management methods.The methods for managing property schema ID follow the standard ent pattern with proper null handling.
320-333: LGTM: Property relationship management methods.The methods for managing property relationships follow the standard ent pattern for One-to-Many relationships.
335-352: LGTM: Property schema relationship management methods.The methods for managing property schema relationships follow the standard ent pattern for Many-to-One relationships.
650-682: LGTM: Property edge specification in createSpec.The property edge specification is correctly implemented with proper relationship type (O2M) and inverse settings.
internal/ent/node_update.go (4)
188-206: LGTM: Property schema ID update methods.The methods for updating property schema ID follow the standard ent pattern with proper null handling.
368-381: LGTM: Property relationship update methods.The methods for managing property relationships in updates follow the standard ent pattern for One-to-Many relationships.
383-400: LGTM: Property schema relationship update methods.The methods for managing property schema relationships in updates follow the standard ent pattern for Many-to-One relationships.
2075-2148: LGTM: Property edge specifications in sqlSave.The property and property schema edge specifications are correctly implemented in the update operation with proper relationship types and inverse settings.
internal/ent/er.html (6)
172-175: New Field in Node: property_schema_id
The addition of “xidDOTID property_schema_id” enriches the Node entity by linking it to a property schema. Please ensure this new field’s naming and type align with the corresponding Go struct (likely “PropertySchemaID”) and that any necessary foreign key constraints or validations in the ORM layer are updated accordingly.
211-217: New Entity: Property
The “Property” entity is now introduced with fields for id, created_at, node_id, field_id, and value. The field types (e.g., “xidDOTID” and “timeDOTTime”) are consistent with existing entities. Verify that the CRUD support in the ORM layer correctly handles this entity and that any business logic using properties is updated to reflect this new structure.
218-220: New Entity: PropertySchema
A minimal “PropertySchema” entity is added with a single id field. This design appears to be a foundation for further schema definitions. Confirm that this simplicity meets current requirements and that future schema extensions, if needed, will maintain consistency with the rest of the application’s design.
221-227: New Entity: PropertySchemaField
The “PropertySchemaField” entity includes essential attributes such as id, name, type, sort, and schema_id. These fields should adequately support detailed property schema definitions. Ensure that the field naming and data types align with system conventions and that the intended validations and associations in the ORM are in place.
306-306: New Relationship: Node to Property
The relationship “Node |o--o{ Property : 'properties/node'” links Nodes to their associated properties. Verify that the chosen cardinality (one-to-many) correctly reflects your domain model and that corresponding ORM edge definitions are updated to support this linkage.
314-316: New Relationships: PropertySchema and Related Entities
These new relationship definitions establish:
• A link from PropertySchema to Node ("node/property_schemas").
• A link from PropertySchema to PropertySchemaField ("fields/schema").
• A link from PropertySchemaField to Property ("properties/schema").Ensure these associations mirror the underlying data model in the ORM and that the directional semantics match the intended one-to-many or many-to-many relationships. Reviewing the business logic that interacts with these relationships is recommended.
web/src/api/openapi-schema/propertyMutationList.ts (1)
1-12: LGTM! Generated type definition looks good.The type alias provides a clear abstraction for a list of property mutations. The generated code warning and documentation are comprehensive.
web/src/api/openapi-schema/nodeUpdatePropertiesOKResponse.ts (1)
1-14: LGTM! Response type follows REST API best practices.The response type is well-structured with a clear property definition. The generated code warning and documentation are comprehensive.
web/src/api/openapi-schema/nodeUpdateChildrenPropertySchemaBody.ts (1)
1-12: LGTM! Request body type supports bulk updates.The type definition allows for bulk updates of property schemas. Consider verifying the handling of:
- Empty arrays
- Maximum array size limits
- Duplicate schema entries
✅ Verification successful
🌐 Web query:
What are the best practices for handling bulk updates in REST APIs, specifically regarding array size limits and duplicate entries?💡 Result:
Bulk operations in REST APIs require careful design to balance performance with reliability. Here are key best practices for handling array size limits and preventing duplicates: ## Array Size Management **Enforce server-side limits** - Set maximum array sizes per endpoint via configuration files (e.g., `webapi.xml`) using attributes like `<data input-array-size-limit="300"/>`[6] - Validate payloads with JavaScript policies before processing: ```javascript if(body.arrayList.length > limit) { context.setVariable('validation.error', true); } ``` Return `400 Bad Request` for oversized payloads[2] **Implement pagination** For large datasets: - Use `GET /resources?page=2&size=100` style parameters[4] - Ensure pagination tokens are unique and time-bound[3] - Process pages atomically to avoid duplicate entries[3] ## Duplicate Prevention **Unique constraints** - Apply database-level unique indexes on composite keys[7] - Validate uniqueness before insertion: ```sql SELECT * FROM payments WHERE user_id=? AND order_id=? FOR UPDATE; ``` Use row-locking during checks[7] **Idempotency keys** - Require clients to include `X-Idempotency-Key` headers - Store processed keys in Redis with TTL: ```bash SET payment:user123:order456 "processed" EX 3600 NX ``` Return `409 Conflict` if key exists[7] ## Bulk Operation Design | Feature | Best Practice | Example | |-------------------|----------------------------------------|------------------------------| | HTTP Methods | Use PATCH for updates, DELETE batches | `PATCH /products/bulk`[4] | | Response Codes | `207 Multi-Status` for partial success | [4][5][8] | | Error Handling | Atomic transactions with rollback | SQL `BEGIN;...COMMIT;`[4][9]| | Async Processing | Return `202 Accepted` with job ID | `Location: /jobs/12345`[5] | **Performance optimizations** - Batch SQL operations: ```sql INSERT INTO orders VALUES (?,?,?), (?,?,?), ...; ``` Reduces roundtrips vs individual inserts[9] - Limit batches to 100-300 items based on payload size[6][10] - Compress payloads with `Content-Encoding: gzip` When implementing bulk operations, validate performance gains justify the complexity through load testing. Use distributed locking for cross-instance consistency and monitor error rates through API metrics dashboards[1][5][7]. Citations: - 1: https://www.mscharhag.com/api-design/bulk-and-batch-operations - 2: https://www.googlecloudcommunity.com/gc/Apigee/Limiting-array-size-in-a-JSON-request/m-p/38861 - 3: https://learn.microsoft.com/en-us/answers/questions/906717/pagination-in-adf-using-rest-api-giving-duplicate - 4: https://www.restack.io/p/design-principles-for-ai-products-answer-rest-api-bulk-operations-best-practices - 5: https://tyk.io/blog/api-design-guidance-bulk-and-batch-import/ - 6: https://developer.adobe.com/commerce/webapi/get-started/api-security/ - 7: https://dev.to/manjufy/rest-api-how-to-avoid-duplicate-resource-creation-on-concurrent-requests-o23 - 8: https://support.backendless.com/t/updates-to-multiple-records-in-one-api-call-but-with-unique-values-for-each/13651 - 9: https://www.outsystems.com/forums/discussion/71950/best-practice-for-bulk-insert-update-in-rest-apis/ - 10: https://community.esri.com/t5/arcgis-rest-apis-and-services-questions/how-to-increase-data-limit-on-add-features-using/td-p/561391
LGTM: The type definition for bulk updates is implemented correctly!
This generated file properly supports bulk operations by accepting an array of PropertySchemaMutableProps. As a best practice, make sure that:
- Empty Arrays: The API gracefully handles scenarios where an empty array is provided.
- Maximum Array Size Limits: There is server-side validation to enforce maximum array sizes to avoid performance degradation.
- Duplicate Schema Entries: The system checks for duplicate entries, possibly via unique constraints or idempotency mechanisms.
These recommendations align with industry best practices for REST API bulk operations, ensuring that the endpoint remains robust and reliable during batch updates.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/library/properties/property_test.go (1)
420-433:⚠️ Potential issueFix return value and remove redundant return statement.
The function signature indicates it returns a boolean, but it doesn't return any value. Additionally, there's a redundant return statement.
func matchSchemaToProperties(t *testing.T, schema openapi.PropertySchemaList, properties []openapi.Property) bool { a := assert.New(t) for i := range schema { schemaField := schema[i] propertyField := properties[i] a.Equal(schemaField.Fid, propertyField.Fid) a.Equal(schemaField.Name, propertyField.Name) a.Equal(schemaField.Type, propertyField.Type) a.Equal(schemaField.Sort, propertyField.Sort) } - return + return true }🧰 Tools
🪛 golangci-lint (1.62.2)
432-432: S1023: redundant
returnstatement(gosimple)
🧹 Nitpick comments (2)
tests/library/properties/property_test.go (2)
22-191: Consider refactoring test setup into helper functions.The test setup code for creating the node hierarchy is repeated across multiple test functions. Consider extracting it into a helper function to improve maintainability and reduce duplication.
Example helper function:
func setupNodeHierarchy(t *testing.T, ctx context.Context, cl *openapi.ClientWithResponses, session *http.Cookie) (parent, child1, child2, child3, child34, child35 *openapi.NodeResponse) { parentname := "parent" parentslug := parentname + uuid.NewString() parent, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ Name: parentname, Slug: &parentslug, }, session) tests.Ok(t, err, parent) // ... rest of the setup code ... return parent, node1, node2, node3, node34, node35 }
414-414: Add descriptive message to schema matching assertion.Consider adding a descriptive message to the
matchSchemaToPropertiescall to make test failures more informative.-matchSchemaToProperties(t, parent.JSON200.ChildPropertySchema, child.JSON200.Properties) +matchSchemaToProperties(t, parent.JSON200.ChildPropertySchema, child.JSON200.Properties) // Verify that child node inherits property schema from parent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/services/library/library.go(2 hunks)app/services/library/node_property_schema/schema.go(1 hunks)app/transports/http/bindings/nodes.go(7 hunks)tests/library/properties/property_schema_test.go(1 hunks)tests/library/properties/property_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/library/properties/property_test.go
432-432: S1023: redundant return statement
(gosimple)
🔇 Additional comments (10)
app/services/library/library.go (1)
8-8: LGTM!The changes correctly integrate the new
node_property_schemaservice into the dependency injection setup.Also applies to: 17-17
app/services/library/node_property_schema/schema.go (2)
16-32: LGTM!The
Updaterstruct is well-designed with clear dependencies and follows the repository's patterns.
34-60: LGTM!The
UpdateChildrenmethod is well-implemented with:
- Proper error handling using the
faultpackage- Authorization checks via
node_auth.AuthoriseNodeMutation- Clear control flow
tests/library/properties/property_schema_test.go (2)
21-120: LGTM!The test implementation is comprehensive and well-structured:
- Proper test setup with parallel execution
- Good coverage of CRUD operations
- Realistic test data
- Clear assertions and verifications
122-133: LGTM!The
matchSchemahelper function is well-implemented with proper test helper designation and clear assertions.app/transports/http/bindings/nodes.go (5)
35-43: LGTM!The
Nodesstruct is properly updated with the newschemaUpdaterfield.
324-344: Verify authorization checks in NodeUpdateChildrenPropertySchema.While the method correctly transforms and updates property schemas, ensure that proper authorization checks are in place before applying updates.
346-363: LGTM!The
NodeUpdatePropertiesmethod is well-implemented with proper error handling and response serialization.
465-493: LGTM!The
serialiseNodeWithItemsfunction is properly updated to include property-related fields.
536-583: LGTM!The property serialization/deserialization functions are well-implemented and consistent with the codebase patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/library/properties/property_test.go (3)
22-191: Consider adding test data cleanup.The test creates multiple nodes but doesn't clean them up after the test completes. Consider adding cleanup code in a
deferstatement to ensure test data is removed, preventing test pollution in subsequent runs.func TestNodesProperty(t *testing.T) { t.Parallel() integration.Test(t, nil, e2e.Setup(), fx.Invoke(func( lc fx.Lifecycle, root context.Context, cl *openapi.ClientWithResponses, cj *session_cookie.Jar, aw *account_writer.Writer, ) { lc.Append(fx.StartHook(func() { + defer func() { + // Cleanup test data + _, _ = cl.NodeDeleteWithResponse(root, parentslug, session) + }()
251-294: Enhance test coverage with additional assertions.The test verifies field ordering but could be more thorough. Consider adding assertions for:
- Field types remain unchanged after reordering
- Sort values are correctly updated
- Field IDs remain stable after reordering
}) a.Equal([]string{"kind", "added", "weight"}, updatedFields) + + // Verify field types remain unchanged + updatedTypes := dt.Map(schemaUpdate.JSON200.Properties, func(p openapi.PropertySchema) string { + return p.Type + }) + a.Equal([]string{"string", "timestamp", "number"}, updatedTypes) + + // Verify sort values + updatedSorts := dt.Map(schemaUpdate.JSON200.Properties, func(p openapi.PropertySchema) string { + return p.Sort + }) + a.Equal([]string{"1", "2", "3"}, updatedSorts) + + // Verify field IDs remain stable + updatedFids := dt.Map(schemaUpdate.JSON200.Properties, func(p openapi.PropertySchema) string { + return p.Fid + }) + a.Equal(fieldIDs, updatedFids)
396-415: Add edge cases to test schema inheritance.Consider adding test cases for:
- Updating parent schema after child properties are set
- Deleting fields from parent schema
- Handling conflicts between parent and child schemas
matchSchemaToProperties(t, parent.JSON200.ChildPropertySchema, child.JSON200.Properties) + + // Test updating parent schema after child properties are set + parentUpdate, err := cl.NodeUpdateChildrenPropertySchemaWithResponse(ctx, parentslug, openapi.NodeUpdateChildrenPropertySchemaJSONRequestBody{ + {Name: "weight", Type: "number", Sort: "1"}, + {Name: "kind", Type: "string", Sort: "2"}, + }, session) + r.NoError(err) + a.Equal(200, parentUpdate.StatusCode()) + + // Verify child properties are updated accordingly + childAfterUpdate, err := cl.NodeGetWithResponse(ctx, slug3, session) + r.NoError(err) + r.NotNil(childAfterUpdate) + a.Len(childAfterUpdate.JSON200.Properties, 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/library/properties/property_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/library/properties/property_test.go
432-432: S1023: redundant return statement
(gosimple)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/resources/library/node_writer/node_writer.go (2)
229-234: Consider implementing a background job system.The comment suggests this cleanup should run in the background but SQLite limitations prevent it. Consider implementing a proper background job system for better scalability.
239-251: Remove redundant return statement.The function has a redundant return statement at the end.
Apply this diff:
- return🧰 Tools
🪛 golangci-lint (1.62.2)
248-248: Error return value of
(*github.com/Southclaws/storyden/internal/ent.PropertySchemaDelete).Execis not checked(errcheck)
250-250: S1023: redundant
returnstatement(gosimple)
app/resources/library/node_querier/node_querier.go (1)
48-94: Consider extracting the SQL query to a separate file.The SQL query is complex and would be better maintained in a separate SQL file. This would improve readability and allow for better version control of SQL changes.
tests/library/properties/property_test.go (1)
445-458: Remove redundant return statement.The function has a redundant return statement at the end.
Apply this diff to remove the redundant return:
func matchSchemaToProperties(t *testing.T, schema openapi.PropertySchemaList, properties []openapi.Property) { a := assert.New(t) for i := range schema { schemaField := schema[i] propertyField := properties[i] a.Equal(schemaField.Fid, propertyField.Fid) a.Equal(schemaField.Name, propertyField.Name) a.Equal(schemaField.Type, propertyField.Type) a.Equal(schemaField.Sort, propertyField.Sort) } - return }internal/ent/mutation.go (3)
16290-16294: Consider adding validation in ResetProperties.The method resets properties without validating their state. Consider adding validation to ensure properties are in a valid state before resetting.
func (m *NodeMutation) ResetProperties() { + if m.properties == nil && !m.clearedproperties && m.removedproperties == nil { + return + } m.properties = nil m.clearedproperties = false m.removedproperties = nil }
16303-16304: Simplify PropertySchemaCleared check.The method has redundant checks. The
PropertySchemaIDCleared()check is sufficient sinceclearedproperty_schemais set in the same places.func (m *NodeMutation) PropertySchemaCleared() bool { - return m.PropertySchemaIDCleared() || m.clearedproperty_schema + return m.PropertySchemaIDCleared() }
22072-22103: Enhance error messages in SetField.The error messages in SetField could be more descriptive by including the actual type of the provided value.
func (m *PropertySchemaFieldMutation) SetField(name string, value ent.Value) error { switch name { case propertyschemafield.FieldName: v, ok := value.(string) if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) + return fmt.Errorf("unexpected type %T for field %s (expected string)", value, name) } m.SetName(v) return nil // ... similar changes for other cases } return fmt.Errorf("unknown PropertySchemaField field %s", name) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/resources/library/node_properties/writer.go(1 hunks)app/resources/library/node_querier/node_querier.go(7 hunks)app/resources/library/node_writer/node_writer.go(2 hunks)internal/ent/client.go(11 hunks)internal/ent/er.html(3 hunks)internal/ent/migrate/schema.go(5 hunks)internal/ent/mutation.go(25 hunks)internal/ent/node.go(12 hunks)internal/ent/node/node.go(7 hunks)internal/ent/node/where.go(3 hunks)internal/ent/node_create.go(7 hunks)internal/ent/node_query.go(9 hunks)internal/ent/node_update.go(9 hunks)internal/ent/schema/node.go(3 hunks)internal/ent/schema/node_property.go(1 hunks)internal/ent/schema/node_property_schema.go(1 hunks)internal/ent/schema/node_property_schema_field.go(1 hunks)tests/library/properties/property_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/ent/schema/node_property_schema_field.go
- internal/ent/node_query.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/resources/library/node_writer/node_writer.go
248-248: Error return value of (*github.com/Southclaws/storyden/internal/ent.PropertySchemaDelete).Exec is not checked
(errcheck)
250-250: S1023: redundant return statement
(gosimple)
🔇 Additional comments (79)
internal/ent/node/where.go (3)
109-112: LGTM! Basic property schema ID predicate is well implemented.The implementation follows the established pattern for ID field predicates in the codebase.
734-812: LGTM! Property schema predicates are comprehensive and well implemented.The implementation provides a complete set of predicates for property_schema_id, following the established patterns in the codebase.
1142-1186: LGTM! Property edge predicates are well implemented.The implementation correctly handles both O2M (properties) and M2O (property_schema) relationships, following the established patterns in the codebase.
internal/ent/node/node.go (4)
39-40: LGTM! Property schema integration looks good.The new constants properly define the relationships between Node, Property, and PropertySchema entities:
- Node to PropertySchema: Many-to-One relationship
- Node to Properties: One-to-Many relationship
Also applies to: 61-64, 107-120
291-294: LGTM! Ordering functions are well-implemented.The new ordering functions follow the established patterns and provide comprehensive querying capabilities for property-related fields:
ByPropertySchemaID: Orders by schema IDByPropertiesCount: Orders by number of propertiesByProperties: Orders by property termsByPropertySchemaField: Orders by property schema fieldsAlso applies to: 374-393
485-498: LGTM! SQL graph steps are correctly defined.The new step functions properly set up the graph relationships:
newPropertiesStep: O2M relationship from Node to PropertiesnewPropertySchemaStep: M2O relationship from Node to PropertySchema
1-1: Note: This is a generated file.This file is automatically generated by
ent. Any changes should be made to the schema definitions instead of modifying this file directly.Let's verify the schema definitions that generated this code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Find and display the schema definitions for Node, Property, and PropertySchema fd -e go -p "schema.*\.go" | xargs rg -l "Node|Property|PropertySchema"Length of output: 1122
Generated Code and Schema Definitions Verified
The generated file (internal/ent/node/node.go) remains untouched and is correctly produced by ent. The shell script confirms the presence of the expected schema definition files (e.g., internal/ent/schema/node.go, internal/ent/schema/node_property_schema.go, and internal/ent/schema/node_property_schema_field.go) that underpin these changes.
• The auto-generated file is valid and should not be modified directly.
• The corresponding schema definitions that generated the Node, Property, and PropertySchema relationships are present and correctly integrated.internal/ent/node_update.go (23)
20-21: Imports look appropriate.
These new imports for "property" and "propertyschema" cleanly integrate with the ent schema expansions.
188-192: New field setter for Property Schema ID.
This setter method correctly updates the "property_schema_id" in the mutation layer.
194-200: Nillable setter is straightforward.
This method handles optional values properly by checking for nil before setting.
202-206: Clearing the Property Schema ID.
Clearing the ID via the mutation helps remove associations without additional overhead.
368-372: IDs adder for Properties edge.
Method cleanly appends property IDs for an O2M relationship.
374-381: Properties adder convenience method.
This function correctly extracts IDs from given Property entities to add them in bulk.
383-386: Linking PropertySchema instance.
Directly forwards the ID from the given PropertySchema to the Node update mutation.
509-513: Clears all Properties.
Ensures bulk removal of existing Property edges in one call.
515-519: Remove Properties by ID.
Selective removal of specific Property edges by ID.
521-528: Remove Properties by entity.
Efficiently resolves IDs from the provided entities, then calls the ID-based removal method.
530-534: Clear the PropertySchema edge.
Disassociates the current node from its property schema.
915-988: Extended edge specifications for Properties and PropertySchema.
The O2M and M2O definitions here accurately manage adding, removing, and clearing edges for properties and property_schemas. This approach is aligned with Ent’s best practices for relationship handling.
1290-1294: Single-entity update for Property Schema ID.
Mirrors the previous multi-entity update method in the “NodeUpdate” type.
1296-1302: Nillable setting for Property Schema ID.
Properly checks for nil before setting the field, ensuring safe updates.
1304-1308: Clear PropertySchemaID in one-entity context.
Clears the field for single-entity updates, consistent with the multi-entity approach.
1470-1474: Add Property IDs in the NodeUpdateOne builder.
Operates similarly to the multi-entity version, ensuring consistent behavior.
1476-1483: Add Properties by entity references.
A pragmatic convenience method for single-entity update flows.
1485-1488: *Set a single PropertySchema.
Correctly references the new property schema ID in the single-entity context.
1611-1615: Clear all Properties in one-entity context.
Consistently parallels the NodeUpdate approach.
1617-1621: Remove Properties by ID in NodeUpdateOne.
Aligns with the multi-entity method for removing property IDs.
1623-1630: Remove Properties by entity references.
Extracts IDs before removal, ensuring a user-friendly API.
1632-1636: Clear the PropertySchema edge.
Permits unsetting the schema in single-entity updates.
2047-2120: Final edge expansions for NodeUpdateOne.
Covers the property and property_schema edges thoroughly for single-entity updates.internal/ent/schema/node_property_schema.go (6)
1-2: New package for PropertySchema.
Establishes the schema in a dedicated file, keeping definitions modular.
3-7: Imports for Ent schema and related packages.
Seeded with standard edges and annotations.
9-11: PropertySchema struct definition.
Declares the new entity, inheriting ent.Schema.
13-15: Mixin usage.
Uses the Identifier mixin to integrate ID or other repeated fields.
17-19: Empty Fields slice.
No custom fields defined yet – flexible for future expansions.
21-27: Edges to Node and PropertySchemaField.
“node” edge and the “fields” edge, with cascading deletes on fields. Implementation is consistent with the desired relationships.internal/ent/schema/node_property.go (8)
1-2: New package for Property entity.
Introduces the Property schema in a dedicated file.
3-10: Imports for ent fields, edges, indexes, and external packages.
All references correctly align with ent usage.
12-14: Property struct definition.
Defines a new entity that extends ent.Schema.
16-18: Mixin usage for Property.
Leverages Identifier and CreatedAt to unify common fields.
20-26: Three core fields: node_id, field_id, and value.
Consider reusing the prior suggestion to add thorough validation on the "value" field.
28-35: Edges to Node.
Creates a required, unique relationship to "Node" via "node_id".
36-42: Edges to PropertySchemaField with cascade delete.
Secures referential integrity by removing Properties when the related schema field is removed.
45-51: Indexes for improved lookups.
Defines a multi-field unique index and single-field indexes, optimizing queries by node and field.internal/ent/schema/node.go (3)
5-5: LGTM!The
entsqlimport is correctly added for the cascade delete annotations.
28-28: LGTM!The
property_schema_idfield is correctly defined as an optional field with the appropriate type.
65-71: LGTM!The new edges are well-defined with appropriate cascade delete annotations to maintain referential integrity.
app/resources/library/node_properties/writer.go (3)
23-29: Consider separating writer creation.Returning both SchemaWriter and Writer in one function might affect clarity. If each has distinct responsibilities, consider separate functions for creation.
156-159: Fix error handling in deferred rollback.The current implementation may overshadow the original error by reassigning it in the deferred rollback.
76-79:⚠️ Potential issueReplace panic with error return.
Using panic for schema mismatch is risky in production. Consider returning an error to let upstream code handle the situation.
Apply this diff:
- panic("schema mismatch") + return nil, fault.New("schema mismatch in child nodes")Likely invalid or redundant comment.
app/resources/library/node_querier/node_querier.go (1)
182-186: LGTM!The raw SQL execution is properly implemented with context and error handling.
tests/library/properties/property_test.go (9)
99-104: LGTM! Schema update test looks good.The test correctly validates the schema update with different field types and sort orders.
112-126: LGTM! Unauthenticated access test looks good.The test properly verifies that unauthenticated users cannot update properties.
128-142: LGTM! Permission test looks good.The test correctly verifies that users without permissions cannot update properties.
144-158: LGTM! Missing property type test looks good.The test properly verifies that new properties require a type definition.
160-173: LGTM! Property update test looks good.The test correctly validates successful property updates.
175-191: LGTM! New property test looks good.The test properly verifies adding a new property with type information.
254-297: LGTM! Field ordering test looks good.The test correctly validates that property fields are returned in the specified sort order.
400-419: LGTM! Schema inheritance test looks good.The test properly verifies that child nodes inherit property schemas from their parent nodes.
421-440: LGTM! Schema cleanup test looks good.The test correctly verifies that property schemas are cleaned up when no nodes are using them.
internal/ent/node.go (1)
46-47: LGTM! Generated code looks good.The generated code correctly adds property schema support to the Node entity.
Also applies to: 76-79, 387-396
internal/ent/migrate/schema.go (1)
563-563: LGTM! Generated schema looks good.The generated schema correctly defines the property schema tables and relationships.
Also applies to: 701-745, 746-755, 756-789
internal/ent/node_create.go (1)
150-162: LGTM! Generated code looks good.The generated code correctly adds property schema support to node creation.
Also applies to: 320-339, 912-928, 1685-1704
internal/ent/client.go (6)
38-40: LGTM! Property-related imports are correctly added.The new imports for property, propertyschema, and propertyschemafield packages are properly organized and follow the existing import pattern.
93-98: LGTM! Property-related clients are properly added to the Client struct.The new client fields are well-organized and follow the existing pattern:
- Property for managing node properties
- PropertySchema for managing property schemas
- PropertySchemaField for managing schema fields
139-141: LGTM! Client initialization is correctly implemented.The new clients are properly initialized in the init method, maintaining consistency with other client initializations.
3653-3683: LGTM! Node property query methods are well implemented.The QueryProperties and QueryPropertySchema methods are correctly implemented with proper edge definitions and SQL graph operations.
5637-5646: LGTM! Hooks and interceptors are properly updated.The hooks and interceptors structs are correctly updated to include the new property-related entities.
4312-4805: 🏁 Script executed:#!/bin/bash # Description: Check for cascade delete configurations in schema definitions # Search for edge definitions and their OnDelete configurations ast-grep --pattern 'edge.To($_, $_).Field($_, $_).OnDelete($_)'Length of output: 65
internal/ent/mutation.go (2)
34-36: LGTM!The new imports for property-related packages are correctly added.
74-76: LGTM!The new type declarations are correctly added and maintain alphabetical ordering.
internal/ent/er.html (8)
174-174: New Field in Node: property_schema_id Added.
The addition of the field "xidDOTID property_schema_id" in the Node entity extends its schema to support property relationships. Please ensure that the corresponding ORM and migration logic properly handle this new field.
211-217: New Entity Definition: Property.
The new entity "Property" with fields "id", "created_at", "node_id", "field_id", and "value" is clearly defined according to design requirements. Verify that its relationships are implemented in the ORM layer and backed by appropriate foreign key constraints in the database migrations.
218-220: New Entity Definition: PropertySchema.
The new "PropertySchema" entity is introduced with a single "id" field to serve as a container for schema definitions. Consider whether additional descriptive fields might be useful in the future for schema versioning or clarity.
221-227: New Entity Definition: PropertySchemaField.
The "PropertySchemaField" entity is properly defined with fields "id", "name", "type", "sort", and "schema_id". This structure should adequately support detailed property configurations. Double-check that the data types and constraints are consistent with the application’s requirements.
306-306: New Relationship: Node to Property.
The relationship "Node |o--o{ Property : 'properties/node'" effectively establishes a link between nodes and their properties. Ensure that this association is consistently applied within the ORM and that the underlying database constraints support the intended cardinality.
314-314: New Relationship: PropertySchema to Node.
The relationship "PropertySchema |o--o{ Node : 'node/property_schema'" correctly links property schemas to nodes. It is important to confirm the existence of corresponding foreign keys and to update any application logic that depends on node-schema associations.
315-315: New Relationship: PropertySchema to PropertySchemaField.
The relationship "PropertySchema |o--o{ PropertySchemaField : 'fields/schema'" accurately expresses that a property schema includes multiple fields. Please verify that the ORM mappings and migration definitions reflect this association correctly.
316-316: New Relationship: PropertySchemaField to Property.
The relationship "PropertySchemaField |o--o{ Property : 'properties/schema'" establishes the connection from schema fields to the actual properties, completing the triad of new entities. Ensure that this linkage is aligned with the overall data model and business rules.
| "testing" | ||
|
|
||
| "github.com/google/uuid" | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the import order.
The import order seems inconsistent. The github.com/google/uuid package is placed between standard library imports and project imports.
Also applies to: 12-12
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any import ordering rules in the project.
# Look for any linter configuration files
fd -t f '\.golangci\.yml|\.golangci\.yaml|\.golangci\.toml|\.golangci\.json'
# Look for any import ordering rules in Go files
rg -A 5 -B 5 'goimports|import \('Length of output: 126405
Action: Fix the import grouping in tests/library/properties/property_test.go to follow a consistent order.
Our investigation did not uncover any linter or configuration setting that automatically enforces an import order. However, a manual review of the file shows an inconsistency: for example, the third‐party package "github.com/google/uuid" is separated (and thus placed between what look like the standard library imports and other external/project imports) rather than being grouped with the other external packages. For consistency and readability, it’s best to reorder the imports so that groupings follow your style guidelines (typically: standard library imports first, then external third‐party packages, and finally project–specific imports).
Please update the imports in tests/library/properties/property_test.go (lines 8 and 12) accordingly.
No description provided.