Demand control directive validations #3148
Conversation
🦋 Changeset detectedLatest commit: 6a1cbcd 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 |
✅ Deploy Preview for apollo-federation-docs canceled.
|
|
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. |
| // We omit this check to keep the validations to those that will otherwise cause runtime failures. | ||
| // | ||
| // With all that said, assumed size should not be negative. | ||
| if (isDefined(assumedSize) && assumedSize < 0) { |
There was a problem hiding this comment.
isDefined() just checks whether assumedSize is undefined (the docs for isDefined also note, it's mainly for use with .filter(isDefined)).
You'd presumably want to check for null as well, so something like
if (assumedSize !== undefined && assumedSize !== null && assumedSize < 0) {
would be clearer here.
| `Slicing argument "${slicingArgumentName}" is not an argument of "${parent.coordinate}"`, | ||
| { nodes: sourceASTs(application, parent) } | ||
| )); | ||
| } else if (!isIntType(slicingArgument.type) && !(isNonNullType(slicingArgument.type) && isIntType(slicingArgument.type.baseType()))) { |
There was a problem hiding this comment.
Note that .baseType() will unwrap all non-nulls and lists. You probably don't want to support [Int]!, so .ofType would be better here.
| if (!parent.type || !isCompositeType(parent.type)) { | ||
| // The output type must have fields | ||
| errorCollector.push(ERRORS.LIST_SIZE_INVALID_SIZED_FIELD.err( | ||
| `Sized fields cannot be used because "${parent.type}" is not an object type`, |
There was a problem hiding this comment.
Note that isCompositeType() checks whether the type is an object, interface, or union. You probably only want the object or interface case (you should also update the error message here).
There was a problem hiding this comment.
I need to make sure that this covers any type which can have a selection, which I believe should include union. I can change the error message if end users are familiar with the term composite type, but I felt object type was a decent proxy for trying to capture that the output type has some fields.
There was a problem hiding this comment.
@tninesling Unions can have a selection, but their only field is __typename, and __typename doesn't return a list (so a field with @listSize returning a union would always fail validation regardless). Regarding error messages, you're right that people wouldn't know the phrase "composite type", but it would be fine saying e.g. "object or interface type".
| `Sized field "${sizedFieldName}" is not a field on type "${parent.type.coordinate}"`, | ||
| { nodes: sourceASTs(application, parent) } | ||
| )); | ||
| } else if (!sizedField.type || !isListType(sizedField.type)) { |
There was a problem hiding this comment.
Note that this will error if sizedField.type is a non-null list, e.g. [Foo]!.
| )); | ||
| } else { | ||
| for (const sizedFieldName of sizedFields) { | ||
| const sizedField = parent.type.field(sizedFieldName); |
There was a problem hiding this comment.
Note that this validation is currently applied at the subgraph level, which means that if sizedFields references a field in another subgraph, then this validation will fail (that might be fine here, but wanted to make sure y'all were aware).
There was a problem hiding this comment.
(Since the merge logic unions sizedFields together, it shouldn't block anyone/the user can just add a @listSize to the other subgraph.)
There was a problem hiding this comment.
I'm not sure how if would be possible to reference something in another subgraph. Do you mean if the field's output type is @external?
There was a problem hiding this comment.
@tninesling I mean if the field returned an entity type, and some other subgraph resolved a field for that entity.
There was a problem hiding this comment.
I suppose the other subgraph might not have the field that returns that type, so you may not actually be able to apply @listSize in the other subgraph, though arguably this is just a limitation of the IBM cost spec in the context of federation, so it should be fine/could be addressed later. Another way of saying this is, "folks shouldn't use @listSize to indicate the cost characteristics of fields outside their subgraph".
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 main, this PR will be updated. # Releases ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35), [`e1e2605b30efc488b57f62ba43436606a38a3607`](e1e2605), [`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35), [`e1e2605b30efc488b57f62ba43436606a38a3607`](e1e2605), [`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]: - @apollo/[email protected] - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Fixes handling of a `__typename` selection during query planning process. ([#3156](#3156)) When expanding fragments we were keeping references to the same `Field`s regardless where those fragments appeared in our original selection set. This was generally fine as in most cases we would have same inline fragment selection sets across whole operation but was causing problems when we were applying another optimization by collapsing those expanded inline fragments creating a new selection set. As a result, if any single field selection (within that fragment) would perform optimization around the usage of `__typename`, ALL occurrences of that field selection would get that optimization as well. - Add validations for demand control directive applications ([#3148](#3148)) ## @apollo/[email protected] ### Patch Changes - Fixes issue where contextual parameters can have naming collisions if used in multiple subgraphs ([#3155](#3155)) - Updated dependencies \[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35), [`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]: - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Fixes handling of a `__typename` selection during query planning process. ([#3156](#3156)) When expanding fragments we were keeping references to the same `Field`s regardless where those fragments appeared in our original selection set. This was generally fine as in most cases we would have same inline fragment selection sets across whole operation but was causing problems when we were applying another optimization by collapsing those expanded inline fragments creating a new selection set. As a result, if any single field selection (within that fragment) would perform optimization around the usage of `__typename`, ALL occurrences of that field selection would get that optimization as well. - Fixes issue where contextual parameters can have naming collisions if used in multiple subgraphs ([#3155](#3155)) - Updated dependencies \[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35), [`e1e2605b30efc488b57f62ba43436606a38a3607`](e1e2605), [`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35), [`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]: - @apollo/[email protected] ## [email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Adds validations to subgraph directive applications, per the specification in https://ibm.github.io/graphql-specs/cost-spec.html#sec-Validation. These rules primarily assert that the @cost and @listsize directive arguments reference valid fields of the correct type for each application.