composition result differences for compose directives #298
composition result differences for compose directives #298n1ru4l wants to merge 1 commit intodirectives-fixfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds test cases to verify the behavior of directive definition conflicts during service composition, specifically focusing on whether the order of subgraphs affects the outcome. The review feedback points out that the tests currently assert a successful composition despite a type mismatch in directive arguments (String! vs ID!). The reviewer suggests that this behavior should likely be treated as a validation error rather than being codified as a success, as it may represent a bug or a missing validation rule.
| expect(result.errors).toEqual(undefined); | ||
| assertCompositionSuccess(result); |
There was a problem hiding this comment.
These assertions validate that a conflict in directive argument types (String! vs ID!) results in a successful composition where one type is silently chosen based on subgraph sorting. However, the PR description suggests that this behavior is inconsistent with expectations ('The Guild composition raises an exception') and notes that Apollo's similar 'silent winner' approach is problematic (😓).
By using assertCompositionSuccess, these tests codify what appears to be a bug or a missing validation rule. If the intention is to ensure that such conflicts are caught (similar to the FIELD_ARGUMENT_TYPE_MISMATCH mentioned in the description), these tests should instead expect a validation error. Codifying the current inconsistent behavior as a success may make it harder to fix later.
The current apollo federation and guild federation libraries behave different on the non-happy path.
If the compose directives definition is different in all supgraph definitions:
The Guild composition raises an exception:
The apollo federation composition chooses one definition that will win by sorting by service name descending (you can switch out the name of the services and see that the tests start passing on apollo federation) 😓