fix: interface object implementing entity interface#2117
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughThe changes introduce new type aliases for key domain concepts and systematically replace plain string types with these aliases across the codebase to enhance type safety and clarity. Several functions and data structures are updated to use these types. Additional logic ensures correct handling of interface objects and shareability in federation, and new tests are added to validate interface object behaviors and error scenarios. Minor fixes and enhancements are applied to error messages and test schemas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
composition/src/errors/errors.ts(4 hunks)composition/src/schema-building/types.ts(4 hunks)composition/src/schema-building/utils.ts(2 hunks)composition/src/types/types.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(8 hunks)composition/src/v1/normalization/normalization-factory.ts(2 hunks)composition/tests/utils/utils.ts(2 hunks)composition/tests/v1/directives/interface-object.test.ts(3 hunks)composition/tests/v1/entity-interface.test.ts(1 hunks)composition/tests/v1/resolvability.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
composition/src/types/types.ts (1)
3-11: LGTM! Excellent type safety enhancement.The introduction of semantic type aliases (
FieldName,FieldCoords,SubgraphName,TypeName,NodeType) is a great improvement over generic string types. This enhances:
- Code readability and self-documentation
- Type safety by preventing accidental mixing of different string contexts
- Future maintainability and refactoring capabilities
The aliases are appropriately simple and will provide clear semantic meaning throughout the codebase.
composition/tests/v1/entity-interface.test.ts (1)
366-367: LGTM! Test schema correctly updated for shareability validation.The addition of
@shareabledirectives to thenameandagefields in thesubgraphDinterface object aligns with the broader enhancements to interface object shareability handling. This test schema change supports proper validation of field propagation rules in interface types.composition/tests/v1/resolvability.test.ts (1)
2179-2179: LGTM! Consistent test schema update for shareability validation.The addition of
@shareableto theagefield insubgraphAWis consistent with similar updates across test files to support enhanced interface object shareability validation.composition/tests/utils/utils.ts (1)
65-69: Excellent debugging enhancement!Adding detailed error logging before the success assertion will greatly help with troubleshooting when federation unexpectedly fails in tests. Using
console.dirwithdepth: nullensures full object inspection, providing comprehensive debugging information.composition/src/schema-building/utils.ts (1)
576-576: LGTM! Consistent type safety improvement.The refinement from
Array<string>toArray<SubgraphName>aligns with the broader effort to replace generic string types with semantic type aliases. This enhances type safety and code clarity while maintaining the same functionality.composition/src/schema-building/types.ts (4)
28-28: LGTM! Enhanced type safety with semantic aliases.Adding imports for
FieldNameandSubgraphNametype aliases improves type clarity and safety over generic strings.
105-105: LGTM! Improved type safety for subgraph name keys.Replacing generic
stringwithSubgraphNamefor map keys provides better semantic meaning and type safety.
114-114: LGTM! Consistent typing for subgraph names.Using
Set<SubgraphName>instead ofSet<string>maintains consistency with the enhanced typing approach.
176-176: LGTM! Enhanced field name typing.Replacing generic
stringwithFieldNamefor map keys provides better semantic clarity and type safety for field references.composition/src/errors/errors.ts (4)
33-33: LGTM! Enhanced type safety for error handling.Adding imports for
NodeTypeandTypeNamesemantic types improves type clarity in error message generation.
428-430: LGTM! Improved function signature with semantic types.Replacing generic
stringparameters withTypeNameandNodeTypeprovides better type safety and semantic meaning for error generation.
490-490: LGTM! Consistent usage of semantic type parameter.Using the
parentNodeTypeparameter directly in the error message maintains consistency with the enhanced typing approach.
860-861: LGTM! Fixed pluralization in error message.The correction from "entit" + conditional "ies"/"y" to proper pluralization improves error message readability.
composition/src/v1/normalization/normalization-factory.ts (2)
292-292: LGTM!The import addition is necessary for the conditional checks implemented in the method below.
1271-1273: Correctly implements interface object handling logic.The conditional checks appropriately prevent treating interface objects as concrete implementations of interfaces. This is crucial for proper federation behavior since interface objects with the
@interfaceObjectdirective represent interfaces in subgraphs that don't actually implement them as concrete types.The logic is consistently applied in both the existing parent data update path and the new parent data creation path, ensuring uniform behavior.
Also applies to: 1277-1279
composition/tests/v1/directives/interface-object.test.ts (6)
4-10: Imports look appropriate for the new test scenarios.The new type imports and error handling functions align well with the interface object implementation and shareability tests being added.
Also applies to: 15-15
534-571: Test correctly validates interface object implementing entity interface.The test appropriately verifies that interface objects can implement entity interfaces and the resulting federated schema correctly reflects the interface hierarchy.
573-591: Test properly validates field propagation constraints.The test correctly verifies that interface objects cannot propagate fields to entity interfaces, producing the expected invalid interface implementation error.
594-636: Test assertions correctly validate shareability errors.The test properly checks for shareability errors on both EntityB and EntityC when non-shareable fields are propagated through multiple interface objects.
638-677: Test correctly validates @Shareable field propagation.The test appropriately verifies that fields marked with @Shareable can be successfully propagated through multiple interface objects, with the federated schema correctly reflecting this propagation.
809-900: Test data subgraphs are well-structured for interface object scenarios.The new subgraph definitions (fca through fcf) appropriately model various interface inheritance, extension, and field propagation scenarios needed for the test cases.
composition/src/v1/federation/federation-factory.ts (7)
233-233: LGTM! Good use of semantic type aliases.The introduction of
ContractName,FieldCoords, andTypeNametype aliases improves type safety and code clarity throughout the file.
247-247: Type safety improvement.Good refactoring to use
TypeNameandFieldCoordstypes instead of generic strings, making the Map's purpose clearer.
427-431: Good defensive programming practice.The change from
getOrThrowErrorto.get()with a guard clause properly handles the case where configuration data might be absent when all fields are overridden. The explanatory comment is helpful.
1576-1581: Clear variable naming improvement.Renaming
entityInterfacetoentityInterfaceFederationDatabetter reflects the variable's content and improves code readability.
1606-1608: Proper type conversion for entity interface concrete type names.Creating a new
Set<TypeName>ensures type safety when assigning concrete type names to the configuration.
1651-1686: Excellent implementation of interface object field handling.The changes properly handle:
- Direct iteration over subgraph parent definition field data instead of set operations
- Type-safe field coordinates mapping using
FieldCoordstype- Proper propagation of shareability flags from subgraph field data (replacing the previous TODO)
- Correct handling of field data copying with inaccessibility checks
This implementation is cleaner and more maintainable than the previous approach.
3063-3063: Minor style improvement.Simplified array instantiation without explicit type annotation - TypeScript correctly infers the type.
…ting-entity-interface
|
LGTM |
Aenimus
deleted the
david/eng-7749-fix-interface-object-implementing-entity-interface
branch
3 participants
Summary by CodeRabbit
New Features
@interfaceObjectdirectives, including scenarios involving interface inheritance, field propagation, and shareability constraints.Bug Fixes
Refactor
Tests
@shareabledirectives where appropriate.Checklist