New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Demand control directive validations #3148

Merged

tninesling merged 8 commits into main from tninesling/listsize-validations

Sep 24, 2024

Contributor

tninesling commented Sep 19, 2024

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.

tninesling added 5 commits

September 18, 2024 15:09


          Initial test for listSize validation

57b56cd


          Initial validations for slicing arguments and sized fields

2fa6cbc


          Remaining validations from spec

7d26a46


          Make error codes more specific

29d3a0d


          Fix lint errors

eacb711

tninesling requested a review from a team as a code owner

September 19, 2024 19:39

changeset-bot bot commented Sep 19, 2024 •

edited

Loading

🦋 Changeset detected

Latest commit: 6a1cbcd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/federation-internals	Patch
@apollo/gateway	Patch
@apollo/composition	Patch
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

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

netlify bot commented Sep 19, 2024 •

edited

Loading

✅ Deploy Preview for apollo-federation-docs canceled.

Name	Link
🔨 Latest commit	`6a1cbcd`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/66f1d11654b22f0008c01785

codesandbox-ci bot commented Sep 19, 2024 •

edited

Loading

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.

tninesling added 2 commits

September 19, 2024 14:41


          Add changeset

3fdd3c4


          Backfill federation changelog with minimum router version for demand …

6b50e05

…control directives

tninesling requested a review from a team as a code owner

September 19, 2024 21:01

clenfest approved these changes

View reviewed changes

internals-js/src/federation.ts Show resolved Hide resolved

internals-js/src/federation.ts Outdated Show resolved Hide resolved

internals-js/src/federation.ts Outdated Show resolved Hide resolved


          Refactor demand control validations into separate functions

6a1cbcd

tninesling merged commit 5ac01b5 into main

16 of 17 checks passed

tninesling deleted the tninesling/listsize-validations branch

September 24, 2024 16:56

github-actions bot mentioned this pull request

release: on branch main #3153

Merged

sachindshinde reviewed

View reviewed changes

internals-js/src/federation.ts

+                // 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) {

Contributor

sachindshinde Sep 26, 2024

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.

internals-js/src/federation.ts

+                      `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()))) {

Contributor

sachindshinde Sep 26, 2024 •

edited

Loading

Note that .baseType() will unwrap all non-nulls and lists. You probably don't want to support [Int]!, so .ofType would be better here.

internals-js/src/federation.ts

+                  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`,

Contributor

sachindshinde Sep 26, 2024

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).

Contributor Author

tninesling Sep 26, 2024

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.

Contributor

sachindshinde Sep 26, 2024

@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".

internals-js/src/federation.ts

+                          `Sized field "${sizedFieldName}" is not a field on type "${parent.type.coordinate}"`,
+                          { nodes: sourceASTs(application, parent) }
+                        ));
+                      } else if (!sizedField.type || !isListType(sizedField.type)) {

Contributor

sachindshinde Sep 26, 2024

Note that this will error if sizedField.type is a non-null list, e.g. [Foo]!.

internals-js/src/federation.ts

+                    ));
+                  } else {
+                    for (const sizedFieldName of sizedFields) {
+                      const sizedField = parent.type.field(sizedFieldName);

Contributor

sachindshinde Sep 26, 2024

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).

Contributor

sachindshinde Sep 26, 2024

(Since the merge logic unions sizedFields together, it shouldn't block anyone/the user can just add a @listSize to the other subgraph.)

Contributor Author

tninesling Sep 26, 2024

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?

Contributor

sachindshinde Sep 26, 2024

@tninesling I mean if the field returned an entity type, and some other subgraph resolved a field for that entity.

Contributor

sachindshinde Sep 26, 2024 •

edited

Loading

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".

sachindshinde pushed a commit that referenced this pull request


          release: on branch main (#3153)

ad94371

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/composition@2.9.2

### Patch Changes

- Updated dependencies
\[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35),
[`e1e2605b30efc488b57f62ba43436606a38a3607`](e1e2605),
[`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]:
    -   @apollo/federation-internals@2.9.2
    -   @apollo/query-graphs@2.9.2

## @apollo/gateway@2.9.2

### Patch Changes

- Updated dependencies
\[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35),
[`e1e2605b30efc488b57f62ba43436606a38a3607`](e1e2605),
[`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]:
    -   @apollo/query-planner@2.9.2
    -   @apollo/federation-internals@2.9.2
    -   @apollo/composition@2.9.2

## @apollo/federation-internals@2.9.2

### 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/query-graphs@2.9.2

### 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/federation-internals@2.9.2

## @apollo/query-planner@2.9.2

### 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/federation-internals@2.9.2
    -   @apollo/query-graphs@2.9.2

## @apollo/subgraph@2.9.2

### Patch Changes

- Updated dependencies
\[[`2192f355f50db33fe0807d16153f357696b9f190`](2192f35),
[`5ac01b534318105e904c1e6598070f753add3bb1`](5ac01b5)]:
    -   @apollo/federation-internals@2.9.2

## apollo-federation-integration-testsuite@2.9.2

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet