Unexpected Rover check behaviour around Enum members

[wabrit]

Using:

• rover version 0.6.0 or 0.7.0
• apollo federation v2

According to the docs, composition of enums uses either the union or intersection strategy depending on whether the enum is used in only “outbound” operations or “inbound” ones too.

I assumed this implied that rover would never treat a difference in enum membership as an error when checking a subgraph schema.

However I’m getting an error from performing a rover check of the form:

+ npx -p @apollo/rover rover subgraph check MyGraph@demo --name foo --schema ./schema.gql
Checking the proposed schema for subgraph foo against MyGraph@demo
error[E029]: Encountered 1 build error while trying to build subgraph "foo" into supergraph "MyGraph@demo".

Caused by:
    UNKNOWN: Enum type "Color" is used as both input type (for example ...) and 
	output type (for example, ...), but value "RED" is not defined in all the subgraphs defining "Color": 
	"RED" is defined in subgraph "bar" but not in subgraph "foo"
    
    The changes in the schema you proposed for subgraph foo are incompatible with 
	supergraph MyGraph@demo

Is this expected, or is it a bug. It seems that if there is a composition strategy for enums, then as long as the strategy does not result in an empty enum it should not result in a subgraph check error - perhaps either a warning, or the configurable ability to treat such conditions as either as desired.

[pcmanus]

Unfortunately the doc is incorrect and needs to be fixed. Not sure what miscommunication happened there and sorry about that.

But the behaviour is expected.

The composition strategy for enums is that:

• if the enum is used only as an output type, them merging is done by union (like all other input types).
• if the enum is used only as an input type, them merging is done by intersection (like other input types).
• otherwise, if the enum is used as both output and input type, then the merging has to be "intersection" of both of those strategy, which implies the definitions have to match in all subgraphs (so I guess, back to the limitations of federation 1).

The later rule is certainly not as flexible as one would hope, but well, it's kind of a necessity.

To expand a bit, there is a reason we use union for output types but intersection for input types. That is:

• output types are what the subgraph returns. And it's ok if 2 subgraph returns different set of values for a given field, we just say that the corresponding supergraph returns the union of those values (and of course, we could be more restrictive than that, but we want to offer flexibility when possible).
• input types however are values ultimately provided by the "client" to the supergraph. And in general, those values could end up passed to any subgraph (at least that use the type as input). So that's why we need to use an intersection here: we can only accept from the external client values that all the subgraph know how to deal with, or we might run into execution problems. To be clear, we don't use intersection in that case because we like it, intersection is overall less flexible and it would be great if we could union instead, but it just doesn't work and intersection is the best (general) strategy we can do.

But types that can be both input and outputs (scalar and enums) unfortunately get the worst of both worlds. It's a non-issue for scalars since, well, they are just a name and so there is nothing to merge (another way to put it is that they are a trivial case where union and intersection are the same thing).

But enum are the odd case where, when used as both inputs and outputs, we are constrained to use an "equality" strategy (erroring if they are not equal), or we'll compose something that couldn't work at execution.

Let me illustrate with an example to make it concrete. Say we have:

# Subgraph A

enum E {
  A
  B
}

type Query {
  q1(e: E): Int
}

and

# Subgraph B

enum E {
  B
  C
}

type Query {
  q2: E
}

Suppose we merge the enum E with a union strategy. We would get supergraph API:

enum E {
  A
  B
  C
}

type Query {
  q1(e: E): Int
  q2: E
}

but that's a problem for query:

{
  q1(e: C)
}

because subgraph A has no clue about value C. So that doesn't work.

Now suppose we merge E with an intersection strategy. We get supergraph API:

enum E {
  B
}

type Query {
  q1(e: E): Int
  q2: E
}

but that's a problem too, because what when subgraph B returns value C from q2? This is an invalid response from the supergraph API standpoint and will also fail at runtime.

So overall, composition point is not compose something that is bound to fail at runtime, and so it has to error on that example above.

Side-note: we cannot accept the example above and cannot use either a union or intersection strategy "at the type level" for enums that are both input and output. That said, we could improve our current strategy a bit for such enums and look where each enum value is defined and compare that to where the enum is used as input vs output. That is, if you take the example above and remove value C from subgraph B, it still doesn't compose due to value A in subgraph A, but technically we could allow A to be merged because it's defined in all the subgraph that use the enum as input. I'm ok with relaxing the rule with that at some point, but it does the rule a bit more complex to understand and predict, which needs to be considered and is why we started with a simpler rule.

Anyway hope all this helps understand the reasoning/situation. That doesn't excuse the incorrect doc of course, and pinging @StephenBarlow on that. Hopefully this comment provide enough info to fix the documentation.

[wabrit]

Thanks @pcmanus for that very helpful reply; I understand the issues involved with resolving "dual purpose" enums.

My particular case is probably quite edge, as it involves subgraphs which dynamically define the enum content (i.e. the literals of that enum) on startup based on some common external resource. In the event of some change to the latter, its possible that a new literal may appear; that gets picked up on subgraph restart (I don't know of a way to dynamically update a subgraph schema in the same way as works for a federated supergraph). Eventually everything becomes consistent, but there is a window of inconsistency where e.g. subgraph A has picked up the change but subgraph B has not - my concern is that if A republishes its schema then this will be rejected by managed federation.

I guess one approach is to split dual-use enums into separate inbound and outbound (an "input" and "type" enum)?

[pcmanus]

I guess one approach is to split dual-use enums into separate inbound and outbound (an "input" and "type" enum)?

That would work if it's an option.

But I'll also note that, as you point out, requiring all subgraphs to have the same values gets in the way if you're just trying to add a new value to the enum and simply cannot guarantee that it's getting added to all subgraphs simultaneously. The intended "process" for that case is to temporarily use @inaccessible, which hides a value from the supergraph API but as a result remove the equality constraint for that value. That is, suppose you have:

# Subgraph A
enum E {
  A
}

type Query {
  q1(e: E): Int
}

and:

# Subgraph B
enum E {
  A
}

type Query {
  q2: E
}

and you now want to add a B value everywhere. Then the steps would be to first add the value as "inaccessible" to one of the subgraph, so say, change subrgaph A to be:

# Subgraph A
enum E {
  A
  B @inaccessible
}

type Query {
  q1(e: E): Int
}

and push that. This will compose, but B will not be exposed in the supergraph API yet. Then you can add value B to subgraph B:

# Subgraph B
enum E {
  A
  B
}

type Query {
  q2: E
}

Don't have to add @inaccessible there because the one in subgraph A is "enough". You can push that and that still compose but value B is still not "visible", so the last step is to remove the @inaccessible in subgraph A and push that.

Those are the step that allow to add a new enum value "incrementally" in subgraphs without breaking composition in that "dual purpose" enum case.

But I don't know if that help in you case of generating the enum. Maybe it complicates the automation a bit too much.

[wabrit]

Its certainly worth considering @inaccessible as another possible approach, although I suspect it would make orchestration of subgraph restarts/republishing quite complex (and it may be that the subgraph would have difficulty figuring out which literals should be marked as inaccessible). It feels like handling "enum evolution" is tricky even in a less dynamic architecture; you still have to orchestrate across multiple subgraph deployments if e.g. you want to rover publish each subgraph schema on deployment (or rover check).

My guess right now is that splitting enums is going to be the least painful way to deal with this, but I'm interested in your suggestion as to how the checks might be expanded, because I'm pretty sure my use case falls into that example (i.e. multiple subgraphs share the enum for types, only one uses it for input).

Thanks for your help and advice on this.

[pcmanus]

because I'm pretty sure my use case falls into that example (i.e. multiple subgraphs share the enum for types, only one uses it for input).

Sounds like it. Essentially, in that case (input in a single subgraph, outputs in other subgraphs), as long as the one single graph where it's an input is the first one to get new values (that is, as long as that subgraph as all the values defined anywhere), then we could allow the other subgraphs to be a subset. Which does give a simpler evolution path: just make sure you push the subgraph that has the enum as input first.

Again, happy to expand rule if it proves useful in practice. Didn't do it initially because I wasn't sure it would prove useful and didn't wanted to make complex without a good reason, but ... thanks for providing a good reason.

In any case, I suggest we fix the documentation first here, and I'll open a separate issue for expanding the rule.

[pcmanus]

Created https://github.com/apollographql/federation/issues/2006 for following up on a more flexible rule. Fair warning that I'm about to leave on vacation for a few weeks so won't personally follow up more on this for a bit.

[StephenBarlow]

We've updated the docs to correct this error in describing enum composition logic. Apologies for the confusion!

[pcmanus]

Closing this since the docs have been updated and we have #2006 to follow up on some improvements, and so I don't ther is anything to be done here (but feel free to yell if I'm missing something).