fix(composition): Catch unsatisfiable @shareable mutation fields#3305
Conversation
🦋 Changeset detectedLatest commit: 56d8a13 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 3f9a5196a9740523450d73ef |
|
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. |
17ed572 to
bda02c8
Compare
369029b to
57c8040
Compare
bda02c8 to
31be1e5
Compare
29b4c86 to
85099bf
Compare
a436eaa to
c3a0bc0
Compare
85099bf to
e19b3f9
Compare
e19b3f9 to
98aa699
Compare
composition-js/src/validate.ts
Outdated
| previousVisits: QueryGraphState<VertexVisit[]>, | ||
| ): boolean { | ||
| const vertex = this.supergraphPath.tail; | ||
| if (this.subgraphPathInfos instanceof Array) { |
There was a problem hiding this comment.
The repeated use of this instaceof Array check makes me uneasy. Is there any way we can encode this is a type or make some generic structure which can handle both cases?
There was a problem hiding this comment.
Changed these to wrapper classes in the 3rd PR commit (in JS, classes have a bit more overhead compared to other languages, but if it makes the code clearer it's probably fine here).
| cannot be satisfied by the subgraphs because: | ||
| - from subgraph \"A\": | ||
| - cannot find field \"F.y\". | ||
| - cannot move to subgraph \"B\", which has field \"F.y\", because type \"F\" has no @key defined in subgraph \"B\". |
There was a problem hiding this comment.
I'm curious if we could tailor/tighten-up this message. To me, this reads like I can resolve this problem by slapping @key on F in one of my subgraphs.
There was a problem hiding this comment.
You actually can resolve it that way, the wording is intentional.
There was a problem hiding this comment.
I thought the point of this was that we want to disallow subgraph jumps when performing a mutation. Wouldn't adding a key there result in a jump we don't want? Or, is it fine to jump to F.x in A because we know that field is only resolved from one place?
There was a problem hiding this comment.
The latter, it's really about making sure the top-level mutation field is only executed once, regardless of the sub-query.
89e9eb0 to
5f90280
Compare
5f90280 to
56d8a13
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to next, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `next` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `next`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @apollo/composition@2.12.0-preview.4 ### Minor Changes - Fixes a bug where composition may not generate a satisfiability error for an unsatisfiable `@shareable` mutation field. ([#3305](#3305)) ([#3305](#3305)) ### Patch Changes - Fixed handling `@requires` dependency on fields returned by `@interfaceObject` ([#3318](#3318)) Depending on the merge order of the types, we could fail composition if a type that `@requires` data from an `@interfaceObject` is merged before the interface. Updated merge logic to use explicit merge order of scalars, input objects, interfaces, and finally objects. - Updated dependencies \[[`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499)]: - @apollo/federation-internals@2.12.0-preview.4 - @apollo/query-graphs@2.12.0-preview.4 ## @apollo/gateway@2.12.0-preview.4 ### Patch Changes - Updated dependencies \[[`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499), [`bc07e979b9fd24c9b94740b170f11023fe99ba1e`](bc07e97), [`9cbdcb53f859c877a476e2725faa4cb205506f57`](9cbdcb5)]: - @apollo/federation-internals@2.12.0-preview.4 - @apollo/query-planner@2.12.0-preview.4 - @apollo/composition@2.12.0-preview.4 ## @apollo/federation-internals@2.12.0-preview.4 ### Patch Changes - Fixed demand control validations ([#3314](#3314)) Updated `@cost`/`@listSize` validations to use correct federation spec to look them up in the schema. ## @apollo/query-graphs@2.12.0-preview.4 ### Patch Changes - Fixes a bug where query planning may unexpectedly error due to attempting to generate a plan where a `@shareable` mutation field is called more than once across multiple subgraphs. ([#3304](#3304)) ([#3304](#3304)) - Updated dependencies \[[`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4)]: - @apollo/federation-internals@2.12.0-preview.4 ## @apollo/query-planner@2.12.0-preview.4 ### Patch Changes - Fixes a bug where query planning may unexpectedly error due to attempting to generate a plan where a `@shareable` mutation field is called more than once across multiple subgraphs. ([#3304](#3304)) ([#3304](#3304)) - Updated dependencies \[[`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499)]: - @apollo/federation-internals@2.12.0-preview.4 - @apollo/query-graphs@2.12.0-preview.4 ## @apollo/subgraph@2.12.0-preview.4 ### Patch Changes - Updated dependencies \[[`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4)]: - @apollo/federation-internals@2.12.0-preview.4 ## apollo-federation-integration-testsuite@2.12.0-preview.4 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to next, this PR will be updated. # Releases ## @apollo/composition@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) - Added isSuccess argument to @connect and @source ([#3294](#3294)) - Fixes a bug where composition may not generate a satisfiability error for an unsatisfiable `@shareable` mutation field. ([#3305](#3305)) ([#3305](#3305)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - Automatically propagate authorization requirements from implementing type to interface in the supergraph. ([#3321](#3321)) Authorization requirements now automatically propagate from implementing types to interfaces during composition. Direct auth specifications on interfaces are no longer allowed. Interface access requires satisfying ALL implementing types' requirements (`AND` rule), with these requirements included in the supergraph for backward compatibility with older routers. - Fix transitive auth requirements on `@requires` and `@fromcontext` ([#3321](#3321)) Adds new `postMergeValidation` check to ensure that all fields that depends on data from other parts of the supergraph through `@requires` and/or `@fromContext` directives explicitly specify matching `@authenticated`, `@requiresScopes` and/or `@policy` auth requirements, e.g. ```graphql type T @key(fields: "id") { id: ID! extra: String @external # we need explicit `@authenticated` as it is needed to access extra requiresExtra: String @requires(fields: "extra") @authenticated } type T @key(fields: "id") { id: ID! extra: String @authenticated } ``` - Preparing new preview release 2.12.0-preview.3 (patch). ([#3308](#3308)) - Adding new CompositionOption `maxValidationSubgraphPaths`. This value represents the maximum number of SubgraphPathInfo objects that may exist in a ValidationTraversal when checking for satisfiability. Setting this value can help composition error before running out of memory. Default is 1,000,000. ([#3275](#3275)) - Restrict usage of auth directives on interfaces ([#3321](#3321)) Restricts usage of `@authenticated`, `@policy` and `@requiresScopes` from being applied on interfaces, interface objects and their fields. GraphQL spec currently does not define any interface inheritance rules and developers have to explicitly redefine all interface fields on their implementations. At runtime, GraphQL servers cannot return abstract types and always return concrete output types. Due to the above, applying auth directives on the interfaces may lead to unexpected runtime behavior as they won't have any effect at runtime. - Allow merging external types when using arrays as default arguments. ([#3096](#3096)) - Stricter merge rules for @requiresScopes and @Policy ([#3321](#3321)) Current merge policies for `@authenticated`, `@requiresScopes` and `@policy` were inconsistent. If a shared field uses the same authorization directives across subgraphs, composition merges them using `OR` logic. However, if a shared field uses different authorization directives across subgraphs composition merges them using `AND` logic. This simplified schema evolution, but weakened security requirements. Therefore, the behavior has been changed to always apply `AND` logic to authorization directives applied to the same field across subgraphs. Since `@policy` and `@requiresScopes` values represent boolean conditions in Disjunctive Normal Form, we can merge them conjunctively to get the final auth requirements. For example: ```graphql # subgraph A type T @authenticated { # requires scopes (A1 AND A2) OR A3 secret: String @requiresScopes(scopes: [["A1", "A2"], ["A3"]]) } # subgraph B type T { # requires scopes B1 OR B2 secret: String @requiresScopes(scopes: [["B1"], ["B2"]] } # composed supergraph type T @authenticated { secret: String @requiresScopes( scopes: [ ["A1", "A2", "B1"], ["A1", "A2", "B2"], ["A3", "B1"], ["A3", "B2"] ]) } ``` This algorithm also deduplicates redundant requirements, e.g. ```graphql # subgraph A type T { # requires A1 AND A2 scopes to access secret: String @requiresScopes(scopes: [["A1", "A2"]]) } # subgraph B type T { # requires only A1 scope to access secret: String @requiresScopes(scopes: [["A1"]]) } # composed supergraph type T { # requires only A1 scope to access as A2 is redundant secret: String @requiresScopes(scopes: [["A1"]]) } ``` - Fixed handling `@requires` dependency on fields returned by `@interfaceObject` ([#3318](#3318)) Depending on the merge order of the types, we could fail composition if a type that `@requires` data from an `@interfaceObject` is merged before the interface. Updated merge logic to use explicit merge order of scalars, input objects, interfaces, and finally objects. - Updated dependencies \[[`3e2b0a8569a9fe46726182887ed0b4bfc0b52468`](3e2b0a8), [`bb4614d338ae03bac51a5fc2439590f172c4e54d`](bb4614d), [`99f2da21de88f9ad9a32ee7ed64b2d4a92887b40`](99f2da2), [`468f27842608f4e390cfc88bc7e6b4b0945f95ff`](468f278), [`3fd5157b309f1d3439b2d87c67b0601fb246d04c`](3fd5157), [`b734ea04d118db09cf6077fdd968c8f04a96327a`](b734ea0), [`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`e7e67579908d5cd2fa6fe558228dffe4808cd98d`](e7e6757), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499), [`faea2d1174d80593264f2227cfde9a2ba1a59b96`](faea2d1), [`97b9d2edfcfeed99124f9e115f992cbef3804682`](97b9d2e), [`f6af504f1ba8283fd00af0d6e3c9c1a665d62736`](f6af504), [`a595235d3cf8f67611efd8395332b64d067b5f1f`](a595235)]: - @apollo/query-graphs@2.12.0 - @apollo/federation-internals@2.12.0 ## @apollo/gateway@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - Updated dependencies \[[`3e2b0a8569a9fe46726182887ed0b4bfc0b52468`](3e2b0a8), [`bb4614d338ae03bac51a5fc2439590f172c4e54d`](bb4614d), [`99f2da21de88f9ad9a32ee7ed64b2d4a92887b40`](99f2da2), [`468f27842608f4e390cfc88bc7e6b4b0945f95ff`](468f278), [`3fd5157b309f1d3439b2d87c67b0601fb246d04c`](3fd5157), [`b734ea04d118db09cf6077fdd968c8f04a96327a`](b734ea0), [`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`e7e67579908d5cd2fa6fe558228dffe4808cd98d`](e7e6757), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499), [`faea2d1174d80593264f2227cfde9a2ba1a59b96`](faea2d1), [`0dbc7cc72ffacf324231e9ccb2de4189f6bf3289`](0dbc7cc), [`97b9d2edfcfeed99124f9e115f992cbef3804682`](97b9d2e), [`f6af504f1ba8283fd00af0d6e3c9c1a665d62736`](f6af504), [`bc07e979b9fd24c9b94740b170f11023fe99ba1e`](bc07e97), [`a595235d3cf8f67611efd8395332b64d067b5f1f`](a595235), [`9cbdcb53f859c877a476e2725faa4cb205506f57`](9cbdcb5)]: - @apollo/query-planner@2.12.0 - @apollo/composition@2.12.0 - @apollo/federation-internals@2.12.0 ## @apollo/federation-internals@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) - Added isSuccess argument to @connect and @source ([#3294](#3294)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - Automatically propagate authorization requirements from implementing type to interface in the supergraph. ([#3321](#3321)) Authorization requirements now automatically propagate from implementing types to interfaces during composition. Direct auth specifications on interfaces are no longer allowed. Interface access requires satisfying ALL implementing types' requirements (`AND` rule), with these requirements included in the supergraph for backward compatibility with older routers. - Fix transitive auth requirements on `@requires` and `@fromcontext` ([#3321](#3321)) Adds new `postMergeValidation` check to ensure that all fields that depends on data from other parts of the supergraph through `@requires` and/or `@fromContext` directives explicitly specify matching `@authenticated`, `@requiresScopes` and/or `@policy` auth requirements, e.g. ```graphql type T @key(fields: "id") { id: ID! extra: String @external # we need explicit `@authenticated` as it is needed to access extra requiresExtra: String @requires(fields: "extra") @authenticated } type T @key(fields: "id") { id: ID! extra: String @authenticated } ``` - Preparing new preview release 2.12.0-preview.3 (patch). ([#3308](#3308)) - Adding new CompositionOption `maxValidationSubgraphPaths`. This value represents the maximum number of SubgraphPathInfo objects that may exist in a ValidationTraversal when checking for satisfiability. Setting this value can help composition error before running out of memory. Default is 1,000,000. ([#3275](#3275)) - Fixed demand control validations ([#3314](#3314)) Updated `@cost`/`@listSize` validations to use correct federation spec to look them up in the schema. - Restrict usage of auth directives on interfaces ([#3321](#3321)) Restricts usage of `@authenticated`, `@policy` and `@requiresScopes` from being applied on interfaces, interface objects and their fields. GraphQL spec currently does not define any interface inheritance rules and developers have to explicitly redefine all interface fields on their implementations. At runtime, GraphQL servers cannot return abstract types and always return concrete output types. Due to the above, applying auth directives on the interfaces may lead to unexpected runtime behavior as they won't have any effect at runtime. - Stricter merge rules for @requiresScopes and @Policy ([#3321](#3321)) Current merge policies for `@authenticated`, `@requiresScopes` and `@policy` were inconsistent. If a shared field uses the same authorization directives across subgraphs, composition merges them using `OR` logic. However, if a shared field uses different authorization directives across subgraphs composition merges them using `AND` logic. This simplified schema evolution, but weakened security requirements. Therefore, the behavior has been changed to always apply `AND` logic to authorization directives applied to the same field across subgraphs. Since `@policy` and `@requiresScopes` values represent boolean conditions in Disjunctive Normal Form, we can merge them conjunctively to get the final auth requirements. For example: ```graphql # subgraph A type T @authenticated { # requires scopes (A1 AND A2) OR A3 secret: String @requiresScopes(scopes: [["A1", "A2"], ["A3"]]) } # subgraph B type T { # requires scopes B1 OR B2 secret: String @requiresScopes(scopes: [["B1"], ["B2"]] } # composed supergraph type T @authenticated { secret: String @requiresScopes( scopes: [ ["A1", "A2", "B1"], ["A1", "A2", "B2"], ["A3", "B1"], ["A3", "B2"] ]) } ``` This algorithm also deduplicates redundant requirements, e.g. ```graphql # subgraph A type T { # requires A1 AND A2 scopes to access secret: String @requiresScopes(scopes: [["A1", "A2"]]) } # subgraph B type T { # requires only A1 scope to access secret: String @requiresScopes(scopes: [["A1"]]) } # composed supergraph type T { # requires only A1 scope to access as A2 is redundant secret: String @requiresScopes(scopes: [["A1"]]) } ``` ## @apollo/query-graphs@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - Fixes a bug where query planning may unexpectedly error due to attempting to generate a plan where a `@shareable` mutation field is called more than once across multiple subgraphs. ([#3304](#3304)) ([#3304](#3304)) - Updated dependencies \[[`3e2b0a8569a9fe46726182887ed0b4bfc0b52468`](3e2b0a8), [`bb4614d338ae03bac51a5fc2439590f172c4e54d`](bb4614d), [`99f2da21de88f9ad9a32ee7ed64b2d4a92887b40`](99f2da2), [`468f27842608f4e390cfc88bc7e6b4b0945f95ff`](468f278), [`3fd5157b309f1d3439b2d87c67b0601fb246d04c`](3fd5157), [`b734ea04d118db09cf6077fdd968c8f04a96327a`](b734ea0), [`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`e7e67579908d5cd2fa6fe558228dffe4808cd98d`](e7e6757), [`faea2d1174d80593264f2227cfde9a2ba1a59b96`](faea2d1), [`97b9d2edfcfeed99124f9e115f992cbef3804682`](97b9d2e), [`f6af504f1ba8283fd00af0d6e3c9c1a665d62736`](f6af504), [`a595235d3cf8f67611efd8395332b64d067b5f1f`](a595235)]: - @apollo/federation-internals@2.12.0 ## @apollo/query-planner@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - Fixes a bug where query planning may unexpectedly error due to attempting to generate a plan where a `@shareable` mutation field is called more than once across multiple subgraphs. ([#3304](#3304)) ([#3304](#3304)) - Updated dependencies \[[`3e2b0a8569a9fe46726182887ed0b4bfc0b52468`](3e2b0a8), [`bb4614d338ae03bac51a5fc2439590f172c4e54d`](bb4614d), [`99f2da21de88f9ad9a32ee7ed64b2d4a92887b40`](99f2da2), [`468f27842608f4e390cfc88bc7e6b4b0945f95ff`](468f278), [`3fd5157b309f1d3439b2d87c67b0601fb246d04c`](3fd5157), [`b734ea04d118db09cf6077fdd968c8f04a96327a`](b734ea0), [`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`e7e67579908d5cd2fa6fe558228dffe4808cd98d`](e7e6757), [`f3ab499eaf62b1a1c0f08b838d2cbde5accb303a`](f3ab499), [`faea2d1174d80593264f2227cfde9a2ba1a59b96`](faea2d1), [`97b9d2edfcfeed99124f9e115f992cbef3804682`](97b9d2e), [`f6af504f1ba8283fd00af0d6e3c9c1a665d62736`](f6af504), [`a595235d3cf8f67611efd8395332b64d067b5f1f`](a595235)]: - @apollo/query-graphs@2.12.0 - @apollo/federation-internals@2.12.0 ## @apollo/subgraph@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) - When a `GraphQLScalarType` resolver is provided to `buildSubgraphSchema()`, omitted configuration options in the `GraphQLScalarType` no longer cause the corresponding properties in the GraphQL document/AST to be cleared. To explicitly clear these properties, use `null` for the configuration option instead. ([#3287](#3287)) - Updated dependencies \[[`3e2b0a8569a9fe46726182887ed0b4bfc0b52468`](3e2b0a8), [`bb4614d338ae03bac51a5fc2439590f172c4e54d`](bb4614d), [`99f2da21de88f9ad9a32ee7ed64b2d4a92887b40`](99f2da2), [`468f27842608f4e390cfc88bc7e6b4b0945f95ff`](468f278), [`3fd5157b309f1d3439b2d87c67b0601fb246d04c`](3fd5157), [`b734ea04d118db09cf6077fdd968c8f04a96327a`](b734ea0), [`4bda3a498eba36e187dfd9ae673eca12d3f3502c`](4bda3a4), [`e7e67579908d5cd2fa6fe558228dffe4808cd98d`](e7e6757), [`faea2d1174d80593264f2227cfde9a2ba1a59b96`](faea2d1), [`97b9d2edfcfeed99124f9e115f992cbef3804682`](97b9d2e), [`f6af504f1ba8283fd00af0d6e3c9c1a665d62736`](f6af504), [`a595235d3cf8f67611efd8395332b64d067b5f1f`](a595235)]: - @apollo/federation-internals@2.12.0 ## apollo-federation-integration-testsuite@2.12.0 ### Minor Changes - Federation 2.12 and Connect 0.3 ([#3276](#3276)) - Add connect spec v0.2 ([#3228](#3228)) - Federation v2.12 release ([#3323](#3323)) ### Patch Changes - Preparing preview.2 release ([#3255](#3255)) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com>
This PR fixes a bug where composition may erroneously declare a schema with a
@shareablemutation field satisfiable when it shouldn't be.Specifically, this PR updates composition satisfiability to ensure that all query paths under a
@shareablemutation field can be satisfied starting at the same subgraph. (I.e., that no matter what selection set is on the@shareablemutation field, the query planner will not need to make multiple calls to that mutation field across subgraphs.)This is done by partitioning subgraph paths in
ValidationStateby the initial subgraph when a shared mutation field is encountered, and keeping track of separate error stacks as well. If all these partitions experience satisfiability errors, then there's no way to satisfy the selection set containing the witness operation from each partition using a single@shareablemutation field call, and the user is asked to fix (at least) one of the partition's satisfiability errors.This PR may cause composition errors for a previously successful composition if that composition was mistakenly satisfiable due to this bug. We've determined that this may not be a rare occurrence, and accordingly this fix will land in the next minor release (which is why this PR was split off from #3303, where it was originally filed). This PR is currently stacked on top of #3304 for tests to pass, but once that PR merges and is pulled into
next, this PR's base will change tonext.