apollographql / specs-join

The specification for the `@join` core feature
https://specs.apollo.dev/join
3 stars 3 forks source link

Allow `join__field` to specify the type of the field in the subgraph #18

Open pcmanus opened 3 years ago

pcmanus commented 3 years ago

In relaxed composition we want to allow to compose a field that doesn't have the same type in all subtypes in at least a few particular case. For instance if a field f has type A in subgraph G1 and type !A in subgraph G2, we'd like to (at least optionally) let it compose with type A in the supergraph. But in that case, it might be inefficient/undesirable if the query planner does not know that f in G1 is never null.

Not that nullability is just one example, but it is imaginable that it could be allowed to merge 2 types when one is a subtype of another for instance out of convenience.

To preserve that information, we can simply add an optional type: String argument to join__field that would allow to specify which type the field has in the particular subgraph (not having that argument would mean, as today, that the field has the same type than in the supergraph)).

martijnwalraven commented 3 years ago

Should this also include @default values? The query planner needs to know about them to avoid requesting a field that isn't known by a subgraph, and to add the default value to the result. I think we could either add a separate @join__default directive (if the default value is guaranteed to be the same for all subgraphs) or make it part of @join__field (if the default value can differ by subgraph).

pcmanus commented 3 years ago

Should this also include @default values?

We definitively will need something for support @default. I didn't include it here just because I wanted to focus on our initial pre-alpha/alpha targets and those supposedly don't involve new directives. Not that I'm in anyway against anticipating that need a bit in the spec if it's not a point of contention.

if the default value is guaranteed to be the same for all subgraphs

That's the kind of question that suggest we might want to discuss @default separately in a bit more detail. That said, unless I'm misunderstood what we wanted for @default, it's a directive one would put in a subgraph to indicate the value the field should have in other subgraphs (that don't define the field), so it does feel like it's bound to be the same in all subgraphs (that don't define the field). Unless that is we let the user specify which of the "other" subgraphs the @default actually applies to, but my hunch is that adding such complexity is a bad idea, especially upfront.

martijnwalraven commented 3 years ago

To close the loop on this, the original proposal sounds good to me!

queerviolet commented 3 years ago

@martijnwalraven — if @default can't differ by subgraph, there's no reason not to just use @default in the supergraph schema. there's no law that the query planner should get everything it needs to run from join directives, specifically

queerviolet commented 3 years ago

just listing some cases here:

  1. supergraph returns T or [T], subgraph returns T! or [T!] — these have the same selection shape, so no coercion is required. i agree we could skip a null check if we knew those fields would never be null, but i'm dubious about the performance boost there. is jnz very expensive?
  2. supergraph returns [T], subgraph return T — here we do need to coerce the result. but it's not obvious to me that the query planner should be responsible for inferring this, as opposed to specifying the reshaping directly.
  3. supergraph returns I, subgraph returns T implements I — these have different selection shapes, so we do need them during query planning (if I is not known to the subgraph and the subgraph contains multiple types which have been extended to implement I, then we will incorrectly expand I into impossible types and generate invalid fragments in the subgraph query)

my feeling: seems fine, particularly to cover (3). i would like us to avoid having the query planner implicitly perform reshapings like (2) and instead consider ways to instruct the query planner on how to reshape responses.

i believe we also don't need any of this for the fed 1/2 launch? (i.e. these implicit conversions aren't in the fed 2 ux afaik)

pcmanus commented 3 years ago

these implicit conversions aren't in the fed 2 ux afaik

They are (now): https://apollographql.quip.com/86PaA6LjrRLd/WIP-Federation-2-UX-changes#CcCACA5TH8d.

i would like us to avoid having the query planner implicitly perform reshapings like (2)

As mentioned in a comment on the Fed 2 UX doc linked above, I'd like to leave this one out as well.

So here, I propose this addition restricted in the case (1) and (3) listed above.

but i'm dubious about the performance boost there

Sure. And to be clear, afaict, no code in the query planner or gateway execution existing today cares if a subgraph type is nullable or not, and I'm not advocating a short term change on that front. But my POV is: what's the harm of keeping the information around in that case, especially if we do add this type argument to cover (3) anyway? Letting the query planner/gateway/router have an as-precise-as-possible view of the subgraph API feels like a good idea to me in general.