Fix merging of arguments #1567
Fix merging of arguments #1567
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
composition-js/src/merging/merge.ts
Outdated
| const arg = dest.addArgument(argName); | ||
| // If all the sources that have the field have the argument, we do merge it | ||
| // and we're good, but otherwise ... | ||
| if (!sources.every((s) => !s || s.argument(argName))) { |
There was a problem hiding this comment.
nitpicky question Could this be changed to
| if (!sources.every((s) => !s || s.argument(argName))) { | |
| if (sources.some((s) => !s || s.argument(argName))) { |
I think that's the same logic, but it's a little easier for my brain to understand that version.
There was a problem hiding this comment.
Right. I initially had the if without the negation and then a else, but realised I didn't need the first part. Agreed avoiding the negation is probably better now. That said, to have the same logic I believe it needs to be:
if (sources.some((s) => s && !s.argument(argName))) {
Essentially, Not(For all x, P(x)) is It exists x, Not(P(x)).
There was a problem hiding this comment.
Right, good catch on the logic. Makes sense to me if you wanna make that tweak.
| // hint. But if it is mandatory, then we have to fail composition, otherwise | ||
| // the query planner would have no choice but to generate invalidate queries. | ||
| const nonOptionalSources = sources.map((s, i) => s && s.argument(argName)?.isRequired() ? this.names[i] : undefined).filter((s) => !!s) as string[]; | ||
| if (nonOptionalSources.length > 0) { |
There was a problem hiding this comment.
question/nitpick Do we normally compare against > 0?
| if (nonOptionalSources.length > 0) { | |
| if (nonOptionalSources.length) { |
There was a problem hiding this comment.
I do :). Overall, I don't have a strong opinion but I'm not a fan of relying on implicit conversions too much (and to me this just reads better that way) and some googling seemed to imply there wasn't a huge consensus either way. Don't mind adapting though if there is a preference for the shorter pattern.
There was a problem hiding this comment.
This seems fine to me, I don't feel strongly. I just wanna make sure I stay consistent when I do this sort of thing.
(Full disclosure: I'm normally in the "don't compare against 0" camp but 🤷 )
internals-js/src/error.ts
Outdated
| @@ -301,6 +301,11 @@ const INVALID_LINK_DIRECTIVE_USAGE = makeCodeDefinition( | |||
| 'An application of the @link directive is invalid/does not respect the specification.' | |||
| ); | |||
|
|
|||
| const REQUIRED_ARGUMENT_MISSING_IN_SOME_SUBGRAPH = makeCodeDefinition( | |||
| 'REQUIRED_ARGUMENT_MISSING_IN_SOME_SUBGRAPH', | |||
| 'An argument (of a field or directive definition) is not defined in all the subgraphs defining the field and is mandatory in at least some of them.' | |||
There was a problem hiding this comment.
nitpick I think this error message could be more clear (but that might be due to my newness)
| 'An argument (of a field or directive definition) is not defined in all the subgraphs defining the field and is mandatory in at least some of them.' | |
| 'An argument of a field or directive definition is mandatory in some subgraphs, but the argument is not defined in all subgraphs that define the field or directive definition.' |
We shouldn't merge arguments unless they appear in all subgraphs, or this lead to unclear semnantic if those argument are passed by the end user.
The patch from #1567 mistakenly broke then merging of graphQL built-in directives (typically `@deprecated`). And unfortunately, we were missing tests for it. This commit fixes that issue and adds proper testing. While adding tests, I also noticed that when merging repeatable directives that have arguments, if the argument values didn't match between subgraphs, we were failing composition (on account that we don't know which of the arguments should "win"), but the error returned was not useful to users. Or more precisely, merging wasn't error out but instead was applying the non-repeatable directive multiple times to the supergraph, and so upon validating the supergraph at the end of merging, the user would get a "@x cannot appears multiple times at this location", but that's not as actionnable as it should be. The patch adds a better error for this (and a test for it).
We shouldn't merge arguments unless they appear in all subgraphs, orthis lead to unclear semantic if those argument are passed by the end user.
I'll also note that the pre-existing behaviour, which was adding arguments to the supergraph if it appeared in any subgraph was potentially leading to errors when decoding the supergraph because we didn't kept information of where the argument was defined and as a result we sometimes inferred that an argument was in a subgraph that didn't have the argument type. Long story short, if we only include argument that are in all subgraphs (that have the field/directive definition), we solve that problem as well.