Remove @inaccessible elements when converting to API schema

[martijnwalraven]

This is an initial implementation of support for @inaccessible when converting a composed schema to an API schema. For now, it removes any fields, object types, or interface types with an @inaccessible directive.

As part of this change, buildComposedSchema no longer returns an API schema. Instead, a separate toAPISchema function is used to convert the composed schema. The reason for this is that the gateway needs the complete schema for query planning, including elements that will be removed by @inaccessible.

There is ongoing work on better abstractions for inspecting and transforming schemas, including handling of core schema directives, and depending on the timing we may replace this somewhat hacky version before release.

[sachindshinde]

Looking good, just some questions about edge cases:

• Regarding what to do when accessible fields return an @inaccessible type, would you be fine erroring in that case instead of propagating @inaccessible to the field?
  • If we don't want gateway to perform this validation, we could also have this validation be done by Studio. That is, if a build for a contract/derived variant determines that the output core schema would have accessible fields that return @inaccessible types, then the build process would error.
  • More generally this goes back to a question of whether toAPISchema() should validate or propagate, and I'd rather have Studio do the propagation if possible to expose the effects of propagation more cleanly to the user.
  • If we're worried about this being a pain for non-federated users, I'd suggest adding a propagation function that they can call separately to ensure that their core schema is valid according to the @inaccessible spec. (This also allows them to choose whether to propagate or not, as opposed to embedding propagation in toAPISchema() and offering no propagation function which removes that choice.)
• Regarding the type references in implements clauses re-adding those types to theGraphQLSchema, it would be better here I think to remove @inaccessible interfaces from implements clauses, since there's no way to mark an implements clause with @inaccessible like there is for fields. (This goes back to a comment you made in this Quip thread).

More generally, I think the more future-proof solution around this particularly gnarly behavior around schema elements being re-added by some element reference somewhere in the GraphQLSchema may be:

1. Remove elements marked @inaccessible from the GraphQLSchema object via transformSchema(). (Propagating to references that can't be marked @inaccessible, e.g. implements clauses.)
2. On the output of transformSchema(), run validation to ensure the elements found in Step 1 were actually removed. If not, then there's a reference somewhere to that element in the schema, and you can error. (At that point, you could do a search through the GraphQLSchema to find such references and tell the user about them, and if not found you could output some error telling the user to contact us to figure out what we missed in our search.)

EDIT: @david-castaneda noticed that the generated API schema will error when used with printSchemaWithDirectives() from graphql-tools, but won't error with printSchema() (might be from directive usages without definitions). This might be fine, although it's probably worth pointing out in a comment.

[sachindshinde]

IIRC from the earlier talks we had in this Quip thread, we arrived at erroring when an accessible field returned an @inaccessible type. Would you be fine throwing here instead of skipping the field? (Alternatively, Studio can run the validation.)

Separately from that, we wanted to implement propagation from @inaccessible types to accessible fields that return that @inaccessible type in Studio, but that got marked as a stretch goal (and likely an optional feature that users can enable/disable).

[sachindshinde]

Note that if you add a type here that implements Foo (or e.g. change type Query to implement Foo), interface Foo won't be removed, since there will be a reference to Foo in the implements clause of the type. (This is similar to the issue with fields having references to @inaccessible output types in their return type.)

It would be good to modify this test to add such a type, and to change toAPISchema() to remove @inaccessible interfaces from implements clauses.

[timbotnik]

Looks to be resolved via inclusion of type Bar implements Foo now?

[sachindshinde]

So while doing some more testing, we've found some bugs in transformSchema() that are relevant here.

Specifically, when a root operation type is removed via transformSchema(), this results in a bug because:

1. The root operation type is not added to the typeMap.
2. However, calling const schemaConfig = schema.toConfig() returns an object with a reference to those root operation types via schemaConfig.query/schemaConfig.mutation/schemaConfig.subscription.
3. This results in the constructor call for GraphQLSchema re-adding those types to the new schema.
4. The constructor for GraphQLSchema then traverses those unmodified root operation types, leading it to find the unmodified versions of the GraphQL types (which triggers a name conflict if any of those types have been modified in transformSchema()).

So root operation types will be silently added back, and transformSchema() may error depending on whether a used type was modified. (Although the error message won't point to the root cause.)

[sachindshinde]

Some user feedback from @david-castaneda 's demo app surfaced a particular bug around union type handling. (See the comment below.)

[sachindshinde]

Note that if you add a union type here that has member Foo, object type Foo won't be removed, since there will be a reference to Foo in the members clause of the union type. (This is similar to the issue with interfaces and implements clauses.)

It would be good to modify this test to add such a union type, and to change toAPISchema() to remove @inaccessible object types from union member clauses, since there's no way to mark a member clause with @inaccessible like there is for fields. (This again goes back to a comment you made in this Quip thread).

[timbotnik]

Looks like this is resolved via inclusion of union Bar = Foo now?

[abernix]

I took the liberty of merging in main and resolving conflicts.

[pcmanus]

Had a run through the current branch. A few remarks:

• I think there is still a few cases where we may try to remove an innacessible type but it is still referenced (meaning that the patch doesn't remove those references and so the element is probably added back, though I haven't tested). @sachindshinde already mentioned root types, which I believe is still an issue, but I think this can also be an issue with input types referenced from field (or directive definitions) arguments.
• I'll also bump @sachindshinde remark about the current behavior of automaticaly propagating inacessibility versus erroring out. I do think my personal vote would probably go to erroring out (I'm not sure we've settled if propagation is a good idea in the first place and even if it is, toAPISchema is probably not the best place for it; erroring would also help surface holes in Studio's validation/propagation of contracts sooner). I get that throwing in toAPISchema runs the risk of that error not being handled cleanly by the gateway, but we can fix that if it happens (I'm a bit more worried how the risk of something ending up relying on propagation of toAPISchema, which we may not want).
• As of this patch, @inacessible can only be used on fields in subgraphs, but it can be used on types as well in the supergraph. Yet, the definition added (the one from directives.ts) is the same in both case, so in the supergraph, the "locations" of that definition is over restrictive. I think it means a supergraph that has some @inacessible on types might not strictly validate. Don't know if that has practical consequences.

[sachindshinde]

Note this is a partial review; I have not yet looked at the code outside toAPISchema() yet.

I'm also in the process of handing off the Gravity-review-side of this PR to @timbotnik , in the event this doesn't land by the end of this week (as I'm on vacation next week).

Some notes:

• Regarding how to fix the operation type issue, we could work around this as follows in removeInaccessibleElements():
  • We determine the query, mutation, and subscription types before looping through the schema's types.
  • If the user attempts to remove the query type, we throw, as GraphQL spec requires a query type.
  • If the user attempts to remove the mutation or subscription types, we track that (e.g. set those variables to undefined).
  • We reconstruct the schema we pass to transformSchema() to have undefined mutation and subscription types if they were removed.
• Regarding the input type issue, @pcmanus is right in that we'll encounter similar issues when an input type is removed but that type is referenced somewhere (whether that's field arguments, input object field types, or directive definition arguments). However, the version of toAPISchema() that needs to land for the initial version of contracts only has to remove object/interface fields and object/interface/union types. We intend to extend this to input object/enum/scalar types later though, so it's still worth being aware of.
  • This all makes me wonder if we're better off working with ASTs/DocumentNodes instead of GraphQLSchema objects, although there may be some specific reasons we're avoiding this that I'm unaware of.
• Regarding why printSchemaWithDirectives() fails on the API schema (as noted by @david-castaneda), my guess is that it's because the astNodes aren't modified at all by this code, but the core directive definitions are removed. This could also be alleviated by using ASTs instead of GraphQLSchemas (but as I mentioned earlier I'm not sure whether this is actually a problem for this specific use case).
• There's a bug in transformSchema() in that it's not aware of interfaces implementing interfaces, which makes this block of code incorrect. In particular, that block of code doesn't modify the interface list at all, which will potentially leave references to old types that should have been replaced by replaceNamedType() (and lead to types being leaked back into the schema).
  • In fairness, I'm not sure how ready the gateway is to support interfaces-implementing-interfaces more generally, so this may be part of a more general problem that will get tackled in the future (I'm assuming composition doesn't currently support interfaces-implementing-interfaces and that the supergraph schema currently can't use that feature).

[martijnwalraven]

I've changed the behavior to throw when a field returns an @inaccessible type but hasn't been marked as @inaccessible itself.

I also fixed the removal of root types, using the method suggested by @sachindshinde. Instead of throwing immediately when a query root type is removed however, it relies on normal schema validation to generate an appropriate error. To ensure that happens early (rather than during execution), toAPISchema now calls assertValidSchema.

[pcmanus]

Was it intentional to put this after we've cached the api schema? This means that if the schema is invalid, the first call to toAPISchema will throw, but followup calls will succeed with a presumably broken schema. Don't know the gateway code well enough to say if it matters in practice, but I intuitively would have put the validation earlier to avoid ever using an invalid schema.

[sachindshinde]

Since toAPISchema() is part of the public API, I'd suggest it'd be good to validate at the start of the function. You'd also presumably want to validate at the end of the function to catch user errors like e.g. removing all the fields of an object/interface type but not removing the object/interface type itself (i.e. empty object/interface types).

[queerviolet]

something that's struck me: what happens here?

interface Product {
  id: ID!
}

# products subgraph
type Query {
  products: [Product!]!
}

# books subgraph
type Book implements Product {
  id: ID!
  title: String!
}

# drugs subgraph
type Drug implements Product @inaccessible {
  id: ID!
  isLegal: Boolean!
}

currently it looks like we remove Drug, and remove it from Product's set of implementing types. that seems right. but then what do we do if a subgraph returns that type? as with this query:

query { products { __typename id } }

it seems like if products returns an array of Products which includes Drugs, we'll… probably just try to return them?

[sachindshinde]

@queerviolet
We had some long talks about that particular case a while ago. To summarize:

• Right now the intended behavior with our initial pilot release is that subgraphs returning object types that aren't in the API schema will result in a runtime error in the gateway.
  • It is expected that product (cc @jstjoe) will inform pilot customers of this particularly gnarly edge case.
• Soon-ish, we intend to have warnings shown by Studio to tell customers when their core schema possesses such edge cases (that is, core schemas with accessible fields that return an interface/union where an implementing object type is inaccessible).
  • If users desire it, we could have a graph-level toggle that would change these warnings into errors.
  • @jstjoe probably would know more about the timeline/sequencing there.
• There's been some more general thoughts on how we could solve this in a more user-controlled fashion, but we haven't settled/decided on anything there.

[queerviolet]

assuming that the intent is for these to get caught by graphql lastpass execution, i'd advocate that the gateway catch and sanitize these errors, as they currently leak the names of @internal types, which might expose e.g. names of unreleased products or internal services

> g = require('graphql'); g.execute(
  g.buildSchema(`
    interface Foo { val: Int }
    type Bar implements Foo { val: Int }
    type Query { foo: Foo }
  `),
  g.parse('query { foo { val } }'),
  { foo: { __typename: 'Barx', val: 42 } })
{
  errors: [
    GraphQLError [Object]: Abstract type "Foo" was resolve to a type "Barx" that does not exist inside schema.
        at ensureValidRuntimeType (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:698:11)
        at completeAbstractValue (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:680:42)
        at completeValue (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:584:12)
        at resolveField (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:472:19)
        at executeFields (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:292:18)
        at executeOperation (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:236:122)
        at executeImpl (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:116:14)
        at Object.execute (/Users/queerviolet/Desktop/hack/federation/node_modules/graphql/execution/execute.js:60:63)
        at repl:1:3
        at Script.runInThisContext (vm.js:123:20) {
      message: 'Abstract type "Foo" was resolve to a type ' +
        '"Barx" that does not exist inside schema.',
      locations: [Array],
      path: [Array]
    }
  ],
  data: [Object: null prototype] { foo: null }
}

a general if somewhat hacky approach might be:

const anyInaccessibleTypename = new RegExp(inaccessibleTypenames.map(name => '("' + name + '")').join('|'), 'g')
const result = graphql.execute(/* ... */)
result.errors.forEach(e => e.message = e.message.replace(anyInaccessibleTypename, '[inaccessible type]'))

which more or less guarantees that an error message will not leak such names

[sachindshinde]

Some comments below on the rest of the PR.

Regarding the leaking of inaccessible type names that @queerviolet brought up here, I would agree that for a security feature, we shouldn't be leaking inaccessible type names to users ideally.

It is worth noting, however, that:

• Product (cc @jstjoe @prasek ) may deem that releasing this faster but with a big warning around this may be more tolerable than properly hiding the type names if properly hiding the type names is difficult.
• I'm not that well-versed with the query planner, but would it be possible to hide inaccessible type names by changing the supplied field resolver to check whether an object type's __typename exists in the API schema and throw an opaque error if not? A cursory glance makes it seem like we'd just need to change defaultFieldResolverWithAliasSupport (the fieldResolver passed to execute()) in executeQueryPlan(), but again, someone more knowledgeable about executeQueryPlan() should chime in here (@trevor-scheer @martijnwalraven ).

[sachindshinde]

Should this be "blocks" or "unblocks"?

Suggested change

	- Capture and propagate `@tag` and `@inaccessible` directives during composition from subgraph to supergraph SDL. This blocks upcoming work for schema construction and enables schema filtering. [PR #756](https://github.com/apollographql/federation/pull/756)
	- Capture and propagate `@tag` and `@inaccessible` directives during composition from subgraph to supergraph SDL. This unblocks upcoming work for schema construction and enables schema filtering. [PR #756](https://github.com/apollographql/federation/pull/756)

[abernix]

Tracking @queerviolet's suggestion to redact errors in https://github.com/apollographql/polaris-planning/issues/94

[abernix]

I am surprised by the substantial changes to package-lock.json:

8,271 additions, 10,123 deletions not shown because the diff is too large. Please use a local Git client to view these changes.

... given that there are effectively no changes (ordering, only) to the package.json.

... but otherwise LGTM.

Thank you, @trevor-scheer, for getting this over the line!

[abernix]

Not asking for a change, but I'd be surprised if we were in an environment where URL wasn't available. I suppose raw V8, which I'm not sure if this works on yet.

[trevor-scheer]

Good catch, this import is technically borked since this package isn't listed as a dependency. Switching over to import from node native url.

[abernix]

LGTM. Happy to revisit later.

[trevor-scheer]

@abernix good callout on the package-lock changes, resolved this in c1ff284

Recommendations either addressed or captured.

[paulpdaniels]

I remember this being mentioned in one of the recent webinars, but I haven't seen any updates in the specification nor anything in terms of documentation. Is this meant for sub-graph implementers to take up so that users can mark types inaccessible?