Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core v1.0 spec #20

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

core v1.0 spec #20

wants to merge 15 commits into from

Conversation

queerviolet
Copy link
Contributor

@queerviolet queerviolet commented Apr 26, 2022

this PR includes the core schema v1.0 specification (md).

the existing link documentation contains definitions for the @link directive. this is the document which end users will be taken to when they click on https://specs.apollo.dev/link/v1.0 e.g. in Studio, so this documentation is provides a high-level description of the directives' behavior and acceptable formats for their arguments. deeper discussion of the model is linked from elsewhere.

this pr's core schema spec is where we find that deeper discussion. it covers the fundamental concepts and properties of core schema documents, including algorithms and data primitives for processing them.

@queerviolet queerviolet changed the base branch from main to collect-specs April 26, 2022 00:51
@queerviolet queerviolet mentioned this pull request Apr 26, 2022
16 tasks
@queerviolet queerviolet changed the base branch from collect-specs to main April 26, 2022 01:25
@queerviolet queerviolet marked this pull request as draft April 26, 2022 01:47
@pcmanus
Copy link

pcmanus commented Apr 28, 2022

I only had a fairly quick read (haven't looked at "algoritms" at all in particular), so this is only early and high-level feedback.

this represents more of a conceptual shift than an implementation one

There is at least one "implementation" change, which is the @id directive. While I'm not contesting the directive make sense, it remains that it's not part of what we released as federation 2.0.0, and so publishing a link/v1.0 spec that have it would mean the released code does not match the spec, and I hope we can agree that it's undesirable. @id should be introduced in a link/v1.1 imo.

Regarding the conceptual parts, I don't think you're saying anything else here, but I'll note that concepts do matter quite a bit. My point being, a "conceptual shift" is something worth taking the time to discuss and getting consensus.

And I'll admit that I'm not, at first glance, sold on the "global graph" framing. Parts of it is probably that I'm not even that clear on what we mean exactly by this so I'm lacking the motivations behind such framing.

I mean, I think I intuit the rough idea behind the concept, but it's also kind of new to me. And I venture that most users coming to this might also initially wonder about it. We can of course add more description of that concept in the intro, and that would help, but I also wonder if we couldn't have another framing that is more intuitive for newcomer to this.

Personally, I've always and continue to think of @link as essentially an import system, and if we're starting discussing shift in how we want to present core schemas and their underlying concepts, then I would champion giving a shot at framing it as an import system. My hunch is that this much more directly convey to user why they might want to care about this and conjure more useful intuitions.

But anyway, I kind of don't want to get too much into such discussion here because I don't think that's the immediate focus.

And that's what bother me the most here: I think our focus should be on ensuring that people following the https://specs.apollo.dev/link/v1.0 url gets something that matches the exising release. That means no @id for now, but imo that would also imply avoiding conceptual shifts we haven't really had time to discuss and digest as a team. We can do all of this later.

In other words, I'd personally prefer the core v1.0 spec to:

  1. not have @id and
  2. to keep the concepts of the previous versions, essentially to minimize what we have to review and discuss.

@queerviolet queerviolet marked this pull request as ready for review April 28, 2022 21:19
@queerviolet queerviolet requested review from hwillson and glasser April 28, 2022 21:19
@queerviolet
Copy link
Contributor Author

thanks for the feedback. i've removed @id and refocused some of the intro content.

@benweatherman
Copy link

Personally, I've always and continue to think of @link as essentially an import system, and if we're starting discussing shift in how we want to present core schemas and their underlying concepts, then I would champion giving a shot at framing it as an import system. My hunch is that this much more directly convey to user why they might want to care about this and conjure more useful intuitions.

fwiw, as someone coming from this as a user of graphql via django/graphene and not very familiar with graphql specs/internals, core schemas/features were difficult to understand until they were presented as an import mechanism

@queerviolet is this PR updated with main? I wasn't sure if all these files are really changing or if that was taken care of by the previous change #21

@queerviolet
Copy link
Contributor Author

i just merged main.

to be clear, the intent here is a conceptual shift from the core v0.2 spec, where core schemas were not described as an import mechanism (because they weren't, because during development we decided that was out of scope for v0.2) towards one in which they are, explicitly, an import mechanism. that's the intent here. we're aligned on that.

i'm happy for feedback on making this more clear. i've added a yet more import-forward description.

i'm also open to changing the terminology of "global graphs," and "global graph reference(s)" if they aren't obvious for folks who haven't heard matt going on about the global graph for years. i would like it to be technically correct (they aren't urls, alas), but am otherwise flexible

@glasser
Copy link
Member

glasser commented May 12, 2022

I took a quick skim and from a really high level I like what I'm seeing. Looking forward to a world with this compiler!

I found the "global graph reference" terminology somewhat confusing. It doesn't seem to mesh with what I think other folks at this company mean when they say "global graph", though to be fair I don't quite understand exactly what they mean either (in the sense that I think it's a forward-looking concept that will mean something more in the future). But the term has felt to me to be more about "global set of data you can fetch by making GraphQL queries" and not "global set of declarations for graph metadata"...

@queerviolet
Copy link
Contributor Author

queerviolet commented May 12, 2022

i'm totally down to use different terminology! it recently occurred to me that "graph url" shortens to "gurl," which i find agreeable (despite them, technically, not quite being urls).

i will note that i don't think "global graph reference" is out of sync with the global graph meaning "the global set of data you can fetch by making GraphQL queries". it's true that our use cases have mostly focused on attributing metadata to one schema or another. but with v1.0, we're attempting to make metadata annotations as a subset of the things you can @link, with an eye towards global graph composition.

for example, in the spec as written, we can link types from other schemas:

@link(url: "https://subgraphA.example.com", as: "subgraphA")

# a foreign definition for the Product type within subgraphA:
type subgraphA__Product {
  # ...
}

what can we do with that? well, not much right now. the definition of subgraphA__Product will be located as https://subgraphA.example.com#Product, but that's basically the end of it. but including this solves the problem of having full access to subgraph field signatures within the supergraph, and helps solve some pain points we've run into with not having those (particularly around validating the correctness of subgraph queries). we can imagine a future version of the join spec designed with this mechanism in mind could take advantage of it, eliminating the join__Graph enum in favor of linking subgraphs and referring to their types with prefixed names:

@link(url: "https://specs.apollo.dev/join/v2.0")
@link(url: "https://specs.apollo.dev/federation/v2.0")
@link(url: "https://subgraphA.example.com", as: "subgraphA")
@link(url: "https://subgraphB.example.com", as: "subgraphB")

# it's clear from the links above that selections on subgraphA__Product can
# be acquired by querying https://subgraphA.example.com, and subgraph B from its schema url.
type Product @join__type(ref: subgraphA__Product) @join__type(ref: subgraphB__Product) {
  # ...
}

# annotations like federation's @key can remain on the foreign definition (tho this is a design question,
# and we are free to decide that we would like them to be copied or moved into arguments
# to @join__type/@join__field)
type subgraphA__Product @federation__key(...) {
  # ...
}

type subgraphB__Product @federation__key(...) {
  # ...
}

i think this would represent an improvement over the current state of the world vis a vis the current iteration of the join spec:

  • the query planner would have full access to the subgraphs and annotations on them
  • the composer can insert subgraph definitions with exactly the same definition-filling mechanism as we use to provide e.g. the correct definition for @link
    • which means that annotations on the subgraph types will also be correctly attributed to their schemas (e.g. the use of federation__key in the example above produces a @link to the federation spec in the supergraph). the supergraph will have appropriate @link headers for all of them

but what do you think? very open to feedback on this. also from @pcmanus

@queerviolet queerviolet changed the title link/v1.0 and core schema specs core v1.0 spec May 12, 2022
@hwillson hwillson removed their request for review May 30, 2022 18:40
@foo(url: "https://specs.apollo.dev/link/v1.0", import: [
{name: "@link", as: "@foo"},
])
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get the difference between this example and the one just above. Both seem to import @link with a different name?


## Partial Schemas

Partial schemas are schemas which use {@link} to reference definitions which they do not necessarily include. Partial schemas are invalid GraphQL, but can be turned into valid GraphQL by a [compiler](#sec-Appendix-Compilation) which inserts the appropriate definitions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got me confused at first because I was missing the distinction between including/inserting something and using @link (it feel weird to have both). I'd suggest something like:

A partial schema is a document before it is processed by a [compiler](#sec-Appendix-Compilation). 
Partial schemas may use {@link} and are invalid GraphQL but can be turned into valid GraphQL 
by a [compiler](#sec-Appendix-Compilation).

@glasser glasser removed their request for review October 17, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants