Composing subgraphs with different @deprecated args should succeed with a hint instead of error

[benweatherman]

Behavior

Composing subgraphs with different reason arguments for @deprecated cause a composition error:

import gql from 'graphql-tag';
import { composeServices } from '@apollo/composition';

const service1 = {
  name: 'sad-service',
  typeDefs: gql`
    type Query {
      hi: String! @deprecated(reason: "it's sad")
    }
  `,
};
const service2 = {
  name: 'mad-service',
  typeDefs: gql`
    type Query {
      hi: String! @deprecated(reason: "it's mad")
    }
  `,
};
const { supergraphSdl, errors, hints } = composeServices([service1, service2]);
console.error(errors);
console.warn(hints);
console.info(supergraphSdl);

Expected behavior

I'd expect those schemas to compose. A potential output could be the the concatenation of the service names and their respective reasons:

sad-service: it's sad
mad-service: it's mad

Version

I'm using federation 2.0.1

[kzlsakal]

Looks like we don't get this composition error with rover schema checks (managed) or apollo-router. It's only propagated if using JS gateway's IntrospectAndCompose methods.

[pcmanus]

So the initial reasoning was to have a single general rule for merging directive with @deprecated following that, and it wasn't clear what to do when a non-repeatable directive appeared with different arguments in multiple graphs.

Re-@deprecated specifically, as the deprecation reason is somewhat exposed externally by GraphQL, it felt like a good idea to "nudge" toward agreeing on the deprecation reason across subgraphs.

But to be fair, erroring out is more than a simple "nudge" here. And another argument brought forth for changing this is that even if you do agree on the deprecation reason across subgraphs, the current behaviour makes it very hard to change that reason since you'd have to coordinate such change and that's highly undesirable.

Long story short, I agree that this should become a warning rather than a hard error.

The remaining questions for me however are:

1. what exact strategy should we use to merge
2. should we do that only for @deprecated, or should we change the general rule really?

And on those I'm proposing that:

1. for the merge strategy, we copy what we currently do descriptions.
2. that instead of making @deprecated a special, we instead change the general rule we implement for all non-repeatable-with-arguments directive.

Let me now justify those propositions.

Merge strategy?

First, I tend to not like the idea of concatening reasons. First, we'd have to decide how exactly we join the strings exactly, and I'm not sure there is an always good choice (but we shouldn't leak subgraph names in doing so in particular). But I'm also not convinced this is going to even be desirable in many case. I suspect that in many cases, deprecation reasons won't match between subgraphs simply because different teams will have worded it slightly differently. It may be that every so often there is multiple different reasons for deprecating a field, but how much reasons could you have for that really?

So picking one of the reason to be in the supergraph, and being clear in the warning generated which one was picked and which ones were discarded, would be my preference.

That said, which one do we pick?

And while the exact choice is probably not too consequential in practice, I will remark that we already have a strategy for a very similar case and that's how we deal with non-matching descriptions.

And what we do there is that if there is different descriptions in different subgraphs, we check if one of the description is "more popular", that is if a subset of subgraph do agree on some value, and pick such value if it is the case (if it isn't, it's essentially a random choice). The reasoning behind was to try to avoid, when most subgraph do agree on the description of an element, that a single "bad actor" subgraph ends up "winning".

Anyway, because that strategy exists and is already in use, I suggest using it for handling the case where the @deprecated reason is mismatching between subgraphs too. I think the strategy makes as much sense here than in the description case, and It'll increase the consistency of our behaviours.

special case or general rule?

As said, one motivation in fed2 had been to try to have common rules for all the directives we use as much as possible, avoid unecessary special cases.

Of course, if a special case is justified, it is justified.

But I'm wondering if the reasoning for changing the behaviour here doesn't apply more generally. In particular, while we don't merge much directives thus far, we do merge another "non-repeatable-with-argument" directive and that's @specifiedBy. Now, you'd expect that all subgraph should agree on the url of @specifiedBy for a given scalar, but what if you need to change the url for some reason (maybe the spec pointed to hasn't change but the url did, or any other reason). Here too, erroring out makes such renaming very tricky, and so warning instead would be needed. And I think this argument carries on to any other future non-repeatable directive with argument we end up merging. Hence the suggestion of simply changing the general rule and not simply make @deprecated a special case.

[pcmanus]

There didn't seem to be push-back to what I suggested above, so I pushed a PR to do that at #1840.