Add support for inaccessible v0.2

[sachindshinde]

This PR introduces inaccessible v0.2, which adds support for @inaccessible in more locations (arguments, scalars, enums, enum values, input objects, input object fields).

This PR should be ready for review. The only thing missing at the moment is tests for complex default values, but I've encountered some bugs with Schema and friends while writing tests there, so I'm going to wait for those to be resolved before writing those.

To summarize the PR:

• Some various bugs in remove() implementations have been fixed.
• Anything that would result in the API schema being invalid GraphQL is validated early and given a hopefully more helpful error message.
  • In addition, we perform some validations that aren't technically required for the API schema to be valid GraphQL:
    • All schema default values are validated to an extent, in accordance with the Values of Correct Type validation that's used for input values in operation ASTs.
      • The GraphQL spec omits input coercion for default values. This means they're not validated to match their expected type.
      • We perform this validation in order to compute references to inaccessible input fields and enum values.
      • We additionally forbid variable references (which are allowed in operation ASTs, but don't make sense in schemas).
      • By "to an extent", I mean we explicitly skip the following:
        • We do not perform scalar input coercion. We can't easily do this for custom scalars, and some built-ins (e.g. Float) have machine-specific input coercion rules.
        • The GraphQL spec forbids AST string nodes to be provided where an enum type is expected (as part of enum input coercion rules). However, the representation of default values in Schema and friends uses a JS string for both AST strings and AST enum values, so you can't distinguish them.
          • I'm still new to this codebase, so there may be some validation elsewhere that addresses this. But if there isn't, we should be careful about fixing this later, as we don't want to break gateway/router after Fed 2 GA.
    • Arguments and input fields cannot be @inaccessible if that element is required.
      • This is needed for subgraph queries to succeed GraphQL operation validation.
• @inaccessible is banned on some element kinds for v0.2:
  • Specifically, those element kinds are:
    • Built-in schema elements (and their descendants).
    • Core feature schema elements (and their descendants).
    • The descendants of directive definitions with any type-system locations (the definitions themselves can't have directives)
  • In inaccessible v0.1, by coincidence, @inaccessible generally couldn't be applied to these element kinds because the locations weren't supported.
    • The directive definition case is defacto forbidden in v0.1 by virtue of arguments not being supported.
    • We can edit the inaccessible v0.1 spec to say that built-ins are forbidden, since the set of built-ins can't change for old gateways (but could change in the future, so we should edit the spec).
    • For core feature schema elements, existing core specs don't introduce object/interface/union types, but there's an edge case if the user introduced a user-defined core spec (via core v0.2) that had those types. I'm leaning toward editing the inaccessible v0.1 spec to declare it as undefined behavior. That way we can perform the ban for both v0.1 and v0.2 in gateway/router.

[netlify]

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	1035186

[codesandbox-ci]

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.

[pcmanus]

Had a pass and had to go fast on a few details because it's a big patch, but have overall covered it.

At the high level, the general logic seems pretty solid to me.

The one aspect that I'd like to remove, and I discussed this a bit offline already, is the validation of default values, and that's because:

1. It's already handled by the fed 2 code, which means the related code in this PR shouldn't get triggered. The one caveat to this, and thanks a lot to @sachindshinde for discovering that in time, is that said existing code is a bit broken in a few ways, so we should fix it and I've created Fix default value validation for input fields #1692 for that.
2. In principle, it also feels to me that "validating default value" shouldn't be something an inaccessible spec has to dictate/defined. I think it's fine if the spec just said that it assumes valid default values and that its behaviour is unspecified when that's not true (and since we do validate them ...).

The rest of my remarks on the patch are more related to code organisation, and mainly I find the code lacking DRYness. And while I'm not religious on DRY at all, I do think the readability/maintainability of the patch would improve (for me at least) if it was DRYer.

That said, and while I did left a bunch of remarks in that sense,it's also not directly impacting external behaviour and given our impeding deadlines, I'm happy to not block on most of my remarks (which is why I prefixed most of them as "nit", even though I do consider many of them as not that nitpicky if I'm being honest). But I might propose future commits to fix those remark sometime later.

The one thing I do would prefer addressing now however is related to the elements we add to extension for inaccessible errors: the inaccessible_element, inaccessible_element_referencer, disallowed_element, inaccessible_element_requirer, etc.

I'd really prefer we consolidate those. All inaccessible errors are variations on "some element(s) is inaccessible and it's not ok due to some (other) related element(s)", and so while having various error codes to distinguish cases in more fine grained level make sense, I think it's worth making sure the extension has always the same shape, something like (and feel free to tweak naming):

extensions: {
  // coordinates of the problematic inaccessible element(s): can be a one or more elements depending on the case
  inaccessible_elements: string[],
  // whichever elements (coordinates) that references some of `inaccessible_elements`, making it invalid. May be zero,
  // one, or multiple elements depending on the error (zero when the inaccessible element is invalid just because of
  // what it is itself; for example a mandatory argument).
  inaccessible_referencers?: string[],
}

Doing so would at least make it possible to have some generic handling of those elements when that make sense. And one can still use the error code when generic handling is not appropriate.

Note that the reason I'd rather do that one change now is because that's the one thing that is potentially visible externally.

[pcmanus]

Nit: was surprised to find unknown here. And it's weird that the method essentially silently return nothing if it's anything else than a string. I assume that it's because what we pass here is the coordinates passed in the error extensions, and those aren't really typed, but it'd feel better to me to validate those right away (when we extract them from the extensions) and have stuffs well-typed afterward.

I'll also note that it's imo particular error prone here where we have 2 methods, sourceASTsForCoordinate and sourceASTsForCoordinates that differ by a single letter but both take unknown and silently do the wrong thing if you call the wrong one inadvertently.

[pcmanus]

Nit: a general remark here is that I think we ought to be consolidating at least some of the logic, because most errors behave pretty similarly. As I'll suggest in another comment, I think a first would be to make the element naming we put in the err.extensions consistent for all the errors (because if nothing else, the 2-3 first branches of the switch are essentially the same).

[pcmanus]

Let's me make sure I understand why we have this filtering. Is that because of the use of subtyping when merging types? That is, is it "just" so that if say A is the inaccessible type referenced by some field f: A in the supergraph and some subgraph have f: B where B is a subtype of A, then we don't include that subgraph node? This does make sense, but is there reasons for it?

Asking because I initially didn't understand why we needed the filtering, and only realised the need for the case above later, so want to make I'm not missing another reason, and I guess suggest to document this a bit more precisely as it's not trivial.

[pcmanus]

Nit: there is some pre-existing methods to deal checking with validation errors in other tests, typically subgraphValidation.test.ts. It could be nice to reuse that for consistency (said method also extract/check the error codes, which might be a good addition too).

[pcmanus]

Fwiw, the ONLY_INACCESSIBLE_CHILDREN is a good example of what I mean when I say the patch could be more DRY. Fundamentally, what it does, is that for each accessible type having children, it errors if all those children are inaccessible. But it does it 3 times, which is both longer to read, but also doesn't surface the communality. It hurts maintainability imo because it would be really easy to mistakenly modify one of those case and forget to do the same for the 2 other case (and even if you don't forget, it's 3 times more work).

Instead, we could add a simple directChilds method to NamedType and make that a simple case (we may still want to tweak the error message to say either "field" or "values" or "members", but that's easy to solve, maybe with a simple switch on the type.kind to set just that part of the message).

[pcmanus]

That could be replaced by type.allImplementations().

[pcmanus]

We have a MultiMap type (in utils.ts) that would simplify this a bit.

[pcmanus]

It'd make sense to me to have this in definitions.ts (nit: I'd rename as hasNameOfBuiltIn because it's not the name that is built-in). That said, the implementation is a bit inefficient as it. I think this could just be:

return isNamedType(element)
  ? schema.builtInType(element.name) !== undefined
  : schema.builtInDirective(element.name) !== undefined;

though we do need to add the Schema::builtInType(name: string), but not having it is an oversight since we do have the Schema::builtInDirective(name: string) equilvalent.

[pcmanus]

Nit: Ideally this method could be rewritten more simply as:

for (const element of schema.allNamedSchemaElement()) {
  if (element.hasAppliedDirective(inacessibleDirective)) {
    element.remove();
  }
}

though I admit I haven't double-checked whether removing elements while looping on allNamedSchemaElements() works as it should. We should fix it if it doesn't.

[sachindshinde]

@pcmanus

The one aspect that I'd like to remove, and I discussed this a bit offline already, is the validation of default values, and that's because:

1. It's already handled by the fed 2 code, which means the related code in this PR shouldn't get triggered. The one caveat to this, and thanks a lot to @sachindshinde for discovering that in time, is that said existing code is a bit broken in a few ways, so we should fix it and I've created Fix default value validation for input fields #1692 for that.
2. In principle, it also feels to me that "validating default value" shouldn't be something an inaccessible spec has to dictate/defined. I think it's fine if the spec just said that it assumes valid default values and that its behaviour is unspecified when that's not true (and since we do validate them ...).

FWIW, it was the first point that mainly convinced me to not add default value validations to this PR. (Would be bad to get into habit of coding so defensively that different parts of the codebase keep revalidating the same things.)

I'm not necessarily convinced that default value validations shouldn't be performed at all, mainly because @inaccessible is a security-based feature and I'm tempted to err toward rejecting things that might result in security vulnerabilities (default values are exposed in introspection after all). At the very least, I do think the inaccessible spec has to specify what a "valid" default value is in this circumstance (but that definition should certainly lean on another spec, GraphQL or otherwise).

The rest of my remarks on the patch are more related to code organisation, and mainly I find the code lacking DRYness. And while I'm not religious on DRY at all, I do think the readability/maintainability of the patch would improve (for me at least) if it was DRYer.

That said, and while I did left a bunch of remarks in that sense,it's also not directly impacting external behaviour and given our impeding deadlines, I'm happy to not block on most of my remarks (which is why I prefixed most of them as "nit", even though I do consider many of them as not that nitpicky if I'm being honest). But I might propose future commits to fix those remark sometime later.

Apologies about the lack of DRY, it's indeed not fun in terms of readability/maintainability. The current code organization was part of a tradeoff I considered when writing the code/considered the deadline. Basically:

• There were cases where I wanted to modify or add to existing code for Schema and friends that would reduce repetition, but if I went that route in the PR, that may introduce some complications:
  • I'm new to the codebase, so I don't necessarily know how to modify the code safely/what parts of the codebase are affected when I want to make a change. (Or rather, it would take me time to read and understand it, and the time was lacking.)
  • If I did suggest a change to Schema and friends, because these changes may end up becoming part of the public API/may be used elsewhere, I'd have to be relatively more careful about them, and they could hold up the PR if they were controversial/required some thought.
• If I go the route I did with the current PR and avoid touching existing code except for bugfixes, there's definitely going to be ugly parts (as you've noticed), but as long as the code is private, it can be fixed up later by PRs where we can take our time.

Accordingly, I'll help file PRs tomorrow and next week to help modularize/reduce repetition.

The one thing I do would prefer addressing now however is related to the elements we add to extension for inaccessible errors: the inaccessible_element, inaccessible_element_referencer, disallowed_element, inaccessible_element_requirer, etc.

I'd really prefer we consolidate those. All inaccessible errors are variations on "some element(s) is inaccessible and it's not ok due to some (other) related element(s)", and so while having various error codes to distinguish cases in more fine grained level make sense, I think it's worth making sure the extension has always the same shape, something like (and feel free to tweak naming):

extensions: {
  // coordinates of the problematic inaccessible element(s): can be a one or more elements depending on the case
  inaccessible_elements: string[],
  // whichever elements (coordinates) that references some of `inaccessible_elements`, making it invalid. May be zero,
  // one, or multiple elements depending on the error (zero when the inaccessible element is invalid just because of
  // what it is itself; for example a mandatory argument).
  inaccessible_referencers?: string[],
}

Doing so would at least make it possible to have some generic handling of those elements when that make sense. And one can still use the error code when generic handling is not appropriate.

Note that the reason I'd rather do that one change now is because that's the one thing that is potentially visible externally.

Consolidating extension names sounds fine to me (wasn't really sure what our conventions were here). Would also be nice if we documented these somewhere, e.g. alongside error codes, although I imagine that comes with establishing stability guarantees (so I can imagine why we'd be reluctant).

I've updated the PR to remove default value validations, and to use only the above extension names (also consolidated some logic in updateInaccessibleErrorsWithLinkToSubgraphs()). Tackled some of the easier nits, specifically renames and moving code around. Will file PRs for the remaining ones. Also made a quick additional bugfix in the PR for CoreFeature.isFeatureDefinition().

Also thanks again for all the good feedback here, it was a large patch to review and I imagine you already have your hands full with the rest of Fed 2.

[pcmanus]

Last changes looks great, more than happy to commit as is and deal with anything remaining in followups.

Apologies about the lack of DRY

No need. Don't get wrong here,: I'm well aware of the time constraint you had to deal with, and that it's coupled with being new to the code. I completely agree that "make it correct, then make it clean" is the right order of operation, and I applaud setting aside some of the "make it clean" given the constraints (I'll further qualify that "clean" has many aspects, and the PR is already pretty clean imo on most aspects).

If anything, it's my bad for not expressing this better, but great job. It's an amazingly thorough contribution given the time crunch. Let's get this shipped and follow up on a few cleanups afterwards.

mainly because @inaccessible is a security-based feature and I'm tempted to err toward rejecting things that might result in security vulnerabilities

Fair, I'm all for being careful, though I do think it'll be worth defining precisely what security guarantees inaccessible presumably provides, because I'm still unsure being too precise on what valid/invalid value means will be that important.

And for context, what I'm about to discuss from that point on doesn't impact this PR (we're validating default value anyway, and while there is a point below I wish we could consider, it unfortunately will have to be dealt with later (if ever)). We should probably continue that thread of discussion some other place, but not knowing of such places currently, dumping my initial thinking here so it's recorded.

My (very initial) reasoning is that the security-guarantee for inaccessible will probably sound something along the line of "inaccessible elements are not leaked externally" (we obviously need to be way more precise than this :)).

Now, what role validating default value play in this? That is, what risk does the presence of invalid default value adds? I assume what we're trying to be mindful of is cases like:

type Query {
  f(i: String = { x: 1, y : 2 })
}

input I {
  x: Int @inaccessible
  y: Int
}

where due to the invalidity of the default value, we're unsure if the default value is essentially leaking I.x (but this might be a completely different x; that's the point, we don't know if we don't have proper typing). And certainly, rejecting the possibility of invalid default value eliminates that question.

But at the same time, that problem does show up for custom scalar, and I don't think there is way to side-step the question in that case. That is, the example above is not really very different from:

type Query {
  f(i: Foo = { x: 1, y : 2 })
}

scalar Foo

input I {
  x: Int @inaccessible
  y: Int
}

Is that leaking I.x, or is that simply passing a completely different x to Foo which happens to be a scalar that accepts random objects?

I will note that enum values are maybe a slightly simpler problem because they have to be unique per document. So if you have

type Query {
  f(i: Foo = V1)
}

scalar Foo

enum MyEnum {
  V1 @inaccessible
  V2
}

then we could take a stand and say it's leaking V1 with some confidence. But I'll note the current patch does not do that.

Of course, the alternative (the one essentially currently implemented) is to say that values of custom scalar are never considered as referring to any schema elements and are thus never considered as "leaks".

I think both are possible options, though I suspect that while users will understand that we can't be rejecting fields by name in default values of custom scalar (we could imagine to provide custom warnings in our own implementation for this, but it cannot be a hard rejection of the spec), they would probably feel safer if inaccessible enum value were getting rejected, even in default values of custom scalars.

BUT, I'll caveat that by remark by also saying that, unfortunately, I don't think we can implement that rejection of enum values in custom scalar default values without some fairly big code changes since enum values are currently handled as strings internally. So I think we have to stick with the currently implemented semantic for inaccessible/v0.2 and see if we want to change that longer term in an inaccessible/v0.3.

Back to default values, I guess my point is that I don't think invalid default value create new security risks because default value of custom scalar are essentially unvalidated and so any security guarantee will have to have caveats for it. Still, the spec should of course discuss those stuffs and thus discuss invalid default values, which means it should explain valid/invalid means in that context, no disagreement here.

[pcmanus]

For info, variables in default values are actually not even allowed syntactically by the GraphQL grammar (the [const] in https://spec.graphql.org/draft/#DefaultValue). And I mean, don't care having/not having that check, it's just I hadn't realised it before and only when while testing it, so sharing the knowledge :).