feat(federation): Implement @core and @join directives to replace CSDL

[trevor-scheer]

WIP

Review by commit for simplicity - Initial baseline commit should make reviewing the changes from CSDL -> Core simpler to think about.

[martijnwalraven]

I'd suggest just @join__graph(endpoint: "https://foo")? That would allow us to add more arguments later if we need more bits of per-subgraph metadata.

I went with @join__endpoint(serviceName: "foo" url: "https://foo") for now in a09e433. I like endpoint as the directive name because that seems to be in line with what this is specifying. It also keeps open the possibility of including multiple @join__endpoint directives on a graph, if we ever wanted to perform load balancing or failover ourselves (probably something you'd want to leave to a service mesh or other infrastructure though).

We need the serviceName because the join__Graph enum value is uppercased, and that means we can't reverse that to get to the service name if that uses anything else than all lowercase (like productService). (If we decide to go with multiple endpoints, you may also want to use the service name to differentiate between different services for the same graph, like products-A and products-B.)

[martijnwalraven]

Do we need to continue to support both the old CSDL and core schema? If so, should we avoid printing both and expose those as separate print functions that act on the schema?

[trevor-scheer]

I consider composedSdl to be something that we ultimately remove, but I think you make a fair point that it's excessive to do both prints. Seems like we have a few reasonable options:

0. leave as is and just remove it soon (and mark composedSdl as @deprecated?)
1. remove composedSdl in this PR as a breaking change
2. composeAndValidate is just responsible for returning the schema, most consumers will then pass that to the printXyz fn of their choice - also breaking. Also, undeprecate schema. Not the most ergonomic end result, but maybe the most future proof?
3. ?

[martijnwalraven]

Is anyone using composedSdl, or was that added just to support CSDL? What would break if we removed that today?

[martijnwalraven]

Just returning a schema would only work if we were sure a GraphQLSchema could always fully represent a core schema. I'd love for that to be true, but currently there is no established mechanism for core schema metadata, and we just rely on our custom extensions for composition results. (I can see a world where we define a CoreSchema subclass that has further guarantees about how core schema metadata is represented under extensions, but that's not something we can do in the short term.)

[martijnwalraven]

Also, should coreSchema be coreSchemaSDL or coreSDL maybe?

[martijnwalraven]

I think we may want a separate Graph type (in TypeScript) that would encapsulate metadata about a graph, including the graph name and endpoints. That means we could store the graph name uppercased in there and don't have to remember to do that when printing.

[martijnwalraven]

The way this currently works we end up generating invalid SDL when the serviceName is undefined or null (which seems to happen for value types). I tried to fix this in a09e433, but handling value types correctly may require more thought (see 081c59e).

[trevor-scheer]

@glasser I went ahead and cherry-picked @martijnwalraven's changes (using @join__endpoint). Let me know if you have any concerns with that, but it seemed reasonable to me.

[glasser]

We need the serviceName because the join__Graph enum value is uppercased

@martijnwalraven Does it have to be uppercased? Does that mean that we are getting the subgraph ID from somewhere (the subgraph name in managed federation, the service list, etc) and then transforming it to uppercase just for the sake of the enum? Could we just... not do that?

Or I guess do we have to do some transform anyway because the service name might not be a valid GraphQL identifier? Are we doing some sort of cleanup somewhere?

Also can we not call it "serviceName"? I thought we are trying to put that terminology aside in favor of "subgraph" as much as possible.

Also, I am not going to super bikeshed this particular point too much, but it doesn't actually feel like the "subgraph name/service name" is a property of the "endpoint" (the server you're talking to) as much as it's a property of the subgraph itself, so it seems like it would go better on @join__graph or @join__subgraph than a @join__endpoint.

[glasser]

So specifically we might have to do something more special here (or at an earlier place in the process?) to ensure that these values are (a) valid GraphQL names and (b) are unique after upper-casing.

[glasser]

(no insight onto whether the proper fix if these fail should be reporting an error vs sanitizing/uniquifying. we should make sure though that if we uniquify that we do so in a deterministic fashion... though I guess these enum values technically don't need to be stable across composition runs, it would be nice if they were.)

[trevor-scheer]

Funny, we talked about the uppercasing issue today. Though it didn't cross our mind that the serviceName must now be a valid graphql name which is a great consideration. If we're going to go to the length of sanitizing/uniquifying, should we just generate the enum value itself with the knowledge that anything important is captured in the directive on it?

Suppose my services:
1products, 2products, etc.

We have to modify them, is there still a point in preserving some semblance of the original or...? I'm not sure, to be honest!

[glasser]

I think having the enum values be something a human can look at in the rest of the schema and have associations with is valuable.

Frankly if we don't want to do sanitizing/uniquifying I'd rather we just go back to providing them as strings to the other directives rather than enums... But I think a light dusting of sanitization and uniquification would be reasonable. Something like:

1: Replace all non-alphanumerics with _
2: If it starts with a digit, prefix one _
3: (MAYBE, not sure this actually improves things) Upper-case all letters
4: If any string ends with an underscore followed by a string of digits (_[0-9]+$), add another underscore at the end.
5: In case of conflicts, postfix all strings with another _ and a number. Choose the numbers based on the original ordering of the strings before all translations, 1-indexed.

What's the point of step 4? It's so that if you start with A a A_1 you end up with A_1 A_2 A_1_ rather than the third one colliding with the first one.

[martijnwalraven]

Yeah, this is definitely more complicated than I thought it would be. But if we want to use graph names as enum values, we're going to need at least some variant of this algorithm. Uppercasing isn't strictly needed, but using uppercase for enum values is a strong convention in GraphQL, so it does make the SDL more humanly readable.

[martijnwalraven]

Also can we not call it "serviceName"? I thought we are trying to put that terminology aside in favor of "subgraph" as much as possible.

That's a good point. We still use serviceName in our current composition and query planning code, but we should probably avoid that for new surface areas.

Also, I am not going to super bikeshed this particular point too much, but it doesn't actually feel like the "subgraph name/service name" is a property of the "endpoint" (the server you're talking to) as much as it's a property of the subgraph itself, so it seems like it would go better on @join__graph or @join__subgraph than a @join__endpoint.

My concern with @join__graph was that we already have a @join__Graph enum, so that seemed confusing. But thinking about it now, using a lowercased version of the enum name as a directive on values could also be a convention we want to adopt.

That would give us something like this:

enum join__Graph {
    ACCOUNTS @join__graph(name: "accounts" url: "https://foo")
    BOOKS @join__graph(name: "books" url: "https://bar")
}

[trevor-scheer]

@martijnwalraven and I have agreed to close this PR in favor of the https://github.com/apollographql/federation/compare/typescript-redux branch which has these changes merged and then some. We'll be collaborating on that branch.

Leaving this open because of comments which might still need to be resolved.

[abernix]

Superseded by (included in) #622