fix(typegraph): implement additional_props
#980
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new field ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User
participant AC as ArgumentCollector.collectObjectArg
participant Validator as generateObjectValidator
Client->>AC: Submit object for validation
AC->>AC: Check for unexpected properties
alt additionalProps is disallowed
AC->>Client: Throw UnexpectedPropertiesError
else additionalProps is allowed
AC->>AC: Skip error for unexpected properties
end
AC->>Validator: Forward object for further validation
Validator->>Validator: Iterate over object keys
alt Key marked with additionalProps
Validator->>Validator: Skip further checks for key
else
Validator->>Validator: Perform standard key validation
end
Validator-->>AC: Return validation result
AC-->>Client: Return final validation result
sequenceDiagram
participant Caller as Function Caller
participant SS as stringify_struct
Caller->>SS: Call stringify_struct with structured params
SS->>SS: Convert params using JSON.stringify
SS-->>Caller: Return JSON string
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/typegate/src/engine/typecheck/inline_validators/object.ts (1)
1-62: 💡 Verification agent🧩 Analysis chain
Add tests to verify object validation behavior with additional properties
The changes look solid, but I recommend adding tests that specifically verify:
- Objects with additional properties are rejected when
additionalPropsisfalse(default)- Objects with additional properties are accepted when
additionalPropsistrueThese tests would ensure the feature works as expected across different scenarios.
Would you like me to suggest some test cases for validating this new functionality?
🏁 Script executed:
#!/bin/bash # Find existing tests for object validation echo "Checking for existing object validation tests..." rg -A 5 -B 5 "additionalProps|additional_props" --type=ts --type=js test/Length of output: 228
Action Required: Add Tests for Object Validation with Additional Properties
The implementation changes look solid. However, I couldn't find any tests covering the behavior for additional properties. Please add tests to ensure that:
- Rejection Scenario: When
additionalPropsis false (the default), objects with extra keys are rejected with the expected error.- Acceptance Scenario: When
additionalPropsis true, objects with extra keys are accepted.Also, if tests already exist in a different location than the non-existent
test/directory, please verify that they adequately cover these scenarios.Would you like suggestions for specific test cases?
🧹 Nitpick comments (2)
src/typegate/src/engine/planner/args.ts (1)
678-678: Key implementation ofadditional_propsbehaviorThis is the critical change that implements the core behavior for the
additional_propsfeature. The code now only throws anUnexpectedPropertiesErrorwhen unexpected properties are found ANDadditionalPropsis set to false, which properly implements the feature described in the PR objectives.Consider adding a brief comment explaining the purpose of this conditional check for future developers:
- if (!typ.additionalProps && unexpectedProps.length > 0) { + // Only throw an error for unexpected properties if additional properties are not allowed + if (!typ.additionalProps && unexpectedProps.length > 0) {src/typegate/src/engine/typecheck/inline_validators/object.ts (1)
33-33: Implementation of additional properties validation looks good.The added condition
if (${typeNode.additionalProps}) { continue; }correctly allows objects to contain properties not defined in the schema whenadditionalPropsis set totrue. This works well with the existing validation logic and properly integrates with the changes in the type definitions.I would suggest adding a brief comment explaining this behavior for future developers:
+ // Skip validation for unknown keys if additional properties are allowed if (${typeNode.additionalProps}) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/metagen/src/fdk_rs/stubs.rs(1 hunks)src/metagen/src/fdk_rs/types.rs(8 hunks)src/metagen/src/tests/fixtures.rs(1 hunks)src/typegate/src/engine/planner/args.ts(1 hunks)src/typegate/src/engine/typecheck/inline_validators/object.ts(2 hunks)src/typegate/src/typegraph/types.ts(1 hunks)src/typegraph/core/src/typedef/struct_.rs(1 hunks)src/typegraph/core/src/typegraph.rs(1 hunks)src/typegraph/schema/src/types.rs(2 hunks)tests/typecheck/input_validator_test.ts(1 hunks)tests/typecheck/typecheck.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (13)
src/metagen/src/tests/fixtures.rs (1)
61-61: Newadditional_propsfield implementation looks goodThe new field
additional_props: falsehas been properly added to theObjectTypeDatastruct. This default value maintains the current behavior of not allowing additional properties in objects.src/metagen/src/fdk_rs/stubs.rs (1)
119-119: Consistent implementation ofadditional_propsfieldThe
additional_props: falsefield has been correctly added to theObjectTypeDatastruct in the test, consistent with the implementation in other files.tests/typecheck/input_validator_test.ts (1)
243-254: Good test coverage for thestringifyStructfunctionThe test case effectively verifies that the
stringifyStructfunction handles both simple and nested structured inputs correctly. It ensures the function doesn't produce errors when processing valid inputs.The test covers:
- Simple structure with primitive types (string and integer)
- Nested structure with an object property
This provides good basic coverage for the new functionality.
tests/typecheck/typecheck.py (2)
109-113: Well-implemented feature for dynamic object handling.The
stringify_structfunction effectively demonstrates the newadditional_props=Truefeature by allowing arbitrary properties in the input structure. This creates a flexible interface that accepts any JSON-compatible object through theparamsproperty. The implementation is straightforward and aligned with the PR objective.
124-124: Good export naming using camelCase for the public API.The function is correctly exposed with a camelCase name (
stringifyStruct) which maintains consistency with the API naming convention throughout the codebase.src/typegraph/core/src/typedef/struct_.rs (1)
84-84: Properly propagates the additional_props field to the type definition.The implementation correctly adds the
additional_propsfield to theObjectTypeDatastructure in theconvertmethod. This ensures that the struct's configuration for allowing additional properties is properly preserved in the generated type representation.src/metagen/src/fdk_rs/types.rs (2)
445-445: Default behavior correctly established for object type.The
additional_propsfield is appropriately set tofalsefor themy_objobject type, establishing the default behavior that objects don't allow additional properties unless explicitly configured. This conservative default promotes schema validation and type safety.
621-621: Consistent implementation across all test object definitions.The changes consistently add
additional_props: falseto all test object definitions, ensuring that the behavior is well-tested across various scenarios including cyclic references, unions, and complex object structures. This thorough approach provides good test coverage for the new feature.Also applies to: 634-634, 647-647, 679-679, 692-692, 732-732, 745-745
src/typegate/src/typegraph/types.ts (1)
70-70:✅ Verification successful
TypeScript interface properly updated with optional additionalProps.
The
ObjectNodeinterface is correctly enhanced with an optionaladditionalPropsboolean property, maintaining language-specific naming conventions (camelCase for TypeScript). Making this property optional is appropriate since it should have a default behavior when not specified.Let's verify that the corresponding validation logic has been updated to handle this property:
🏁 Script executed:
#!/bin/bash # Look for validation logic that might use the additionalProps field rg -A 5 "additionalProps" --glob "*.ts" | grep -v "types.ts"Length of output: 2840
Interface Update Verified:
additionalPropsIntegration ConfirmedThe update to the
ObjectNodeinterface insrc/typegate/src/typegraph/types.tsadding the optionaladditionalProps?: boolean;field is correctly propagated. Our verification shows:
- Planner Validation: In
src/typegate/src/engine/planner/args.ts, the code correctly checksif (!typ.additionalProps && unexpectedProps.length > 0)and raises an error when unexpected properties exist.- GraphQL Field Handling: In
src/typegate/src/runtimes/utils/graphql_forward_vars.ts, the property is used to conditionally include fields likeselectionSetandarguments.- Inline Validators: In
src/typegate/src/engine/typecheck/inline_validators/object.ts, the conditionif (${typeNode.additionalProps}) { continue; }confirms that the property controls the flow as intended.The changes ensure that when
additionalPropsis not specified, appropriate default behavior is maintained. No further modifications are required.src/typegraph/core/src/typegraph.rs (1)
140-140: LGTM: Default value foradditional_propsis correctly set to false.The implementation correctly initializes
additional_propstofalse, which is a sensible default value. This ensures backward compatibility as objects won't accept unknown properties unless explicitly configured to do so.src/typegraph/schema/src/types.rs (2)
155-155: Adding camelCase serialization for ObjectTypeData is properly applied.The
#[serde(rename_all = "camelCase")]attribute ensures consistent field naming convention in serialized JSON output, which is good for API consistency.
164-166: Proper implementation ofadditional_propsfield with appropriate Serde attributes.The implementation includes:
#[serde(skip_serializing_if = "std::ops::Not::not")]- Efficiently omits the field whenfalseto reduce payload size#[serde(default)]- Properly handles deserialization when the field is missingThis is a well-designed implementation that maintains backward compatibility while adding new functionality.
src/typegate/src/engine/typecheck/inline_validators/object.ts (1)
12-12: Parameter renamed to reflect its usage.Removing the underscore prefix from
_typeNodetotypeNodeimproves code clarity by indicating that the parameter is now being used within the function.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
=======================================
Coverage 81.13% 81.13%
=======================================
Files 142 142
Lines 17967 17968 +1
Branches 1961 1960 -1
=======================================
+ Hits 14578 14579 +1
Misses 3371 3371
Partials 18 18 ☔ View full report in Codecov by Sentry. |
|
additionalProps on output types are filtered for now, but think about it. It doesn't make sense to have variable output shape. It should always be determined in advance. |
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Fixup for #980 <!-- 3. Explain HOW users should update their code --> #### Migration notes --- - [x] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced object validation now accepts additional properties when allowed, offering more flexible data processing. - Introduced a new REST endpoint that returns structured, stringified data based on provided input. - **Tests** - Added a test case to verify the functionality of the new REST endpoint and ensure robust input validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Migration notes
Summary by CodeRabbit
New Features
Tests