Creation of @override directive along with relevant tests

[clenfest]

There are three new hints and two new errors to deal with some of the error conditions. Note that override can be defined both at the object as well as the field level.

[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]

A few points on top of the other inline comments:

• I think @provides and @requires should essentially be handle consistently with @key. All those directives reference fields and so when we override fields, we might "break" of one those reference in the overridden subgraph if we remove the field (and so, like for @key such field should be made @external). It is true that @provides and requires usually applies to external fields in the first and hence are "less" impacted than @key, but due to nesting they can still reference some non-external fields, and overriding those create issues. I've push a commit with tests (and a few related notes/remarks) for both @provides and @requires if you want: pcmanus@d9cf0a4.
• It's possible that overriding a key fields breaks composition. Which is ok, but I think the error message we currently issue in that case is going confusing to user and should be amended to take overrides into account. I pushed a test for that as well with some more details here: pcmanus@9dae950.

[pcmanus]

In general, I think this method doesn't really do what the name implies and is still too tied to shareable (and there is still a bunch of mention to shareable which highlight this function is probably the wrong thing to use to compute keys).

Overall, the way I'd organise this to first have a method that precompute the list of field (coordinates) that appears in any @key. That code should not bother with provides or even have to care about extensions in any ways. It's probably something along the lines of just:

metadata.keyDirective().applications().flapMap((key) => collectTargetFields(...)).map((f) => f.coordinate);

In fact, that method should probably be made generic to also allow pre-compute all the requires and provides fields (separately) since as mentioned in another comment, we should handle those consistently with how key fields are handled for override.

Then shareability can be defined using the precomputed list of keys since shareable fields are fields that either:

• key fields
• or provided fields
• or field with @Shareable or part of a type with @Shareable.

[clenfest]

I restored your original version of computeShareables. Let me know if I got the right message on computeKeys. If not, we should talk about it in person tomorrow.

[netlify]

✅ Deploy Preview for apollo-federation-docs canceled.

Name	Link
🔨 Latest commit	5a0a66e
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/6247197c8fbd4e00091e1f08

[pcmanus]

I'm seeing tests for overriding a field that has a @provides or @requires on it, which currently errors, but I'm not seeing tests for overriding a field that is "used" by a @provides or @requires or to satisfy an interface, which should compose successfully (and use @external in the supergrah).

That is, as far as I can tell, those case should now work correctly, that's the point of isUsedField, but we should have tests to double-check it. If that helps, I pointed in a previous comment to some stubs for such tests for at least @provides and @requires that I put at pcmanus@d9cf0a4. And we should add an equivalent one for interfaces.

[pcmanus]

Thanks for all the updates. I still have a few remaining remarks, but I took the liberty to create concrete proposals for fixing those that I pushed at https://github.com/pcmanus/federation/commits/override_review-edits. Free free to push back on some of those obviously, but here's descriptions of the problem those commits fix:

1. the tests for @provides and @requires that I had written weren't fully complete, mostly not validating as much as they should. I fixed this with pcmanus@1b17e1f.
2. I mentioned that previously and it's probably been lost in the noise, but I think we should have a test that show what happens when you try to override a key field, but doing so means composition validation cannot succeed anymore. I pushed such a test as pcmanus@9aae345. There is TODO in that test because I think the error message is a bit confusing, but I'll come back to this as it's fixed by the last commit below.
3. the "field is used" logic is still broken for fields that are used to satisfy an interface and I've added a test to show the problem in pcmanus@6605e49.
4. the next commit fixes the test added in the previous point, that's pcmanus@5d4fce3 (the fix is trivial, it just need to remove the remaining external check in collectUsedFields, but that made me realise there was a now unecessary check in validateAllExternalFieldsUsed so I removed that too).
5. there is really 2 options once you've "successfully" overridden a field: either you remove the overridden field completely, or it is still used and you mark it external explicitely. The first case was generating a proper hint, but the 2nd one wasn't. This is fixed/tested by pcmanus@c2d6edb.
6. last but not least: as mentioned in the test added in point 2, if you override a key field and that breaks composition, the error message returned will confuse users because it says that the overridden field is external, which is true in the subgraph extracted from the supergraph, but false for the user subgraph. So pcmanus@e945ac6 ensures the message is amended when some field/key is not resolvable due to being overriden. To make that work properly however, we need to be able to say if an external field in an extracted subgraph is external because the user explicitely marked it external, or because we forced it external due to being overridden. What I decided to do for this is to add a new (completely optional) reason argument to @external which we use when extracting from the supergraph to indicate when a field is forced external due to being overridden. The reasons I went that route is that 1) it's a pretty simple solution and 2) I actually think having an optional reason to @external is not crazy as even users could want to use it for documentation purposes (after all, it can sometimes take a bit of searching the schema to figure out why a field was added as external, and some user may want to use that reason to facilitate reading).

[pcmanus]

Awesome work, and thanks for the patience.