Skip to content

Latest commit

 

History

History
547 lines (519 loc) · 30.8 KB

2022-07-07.md

File metadata and controls

547 lines (519 loc) · 30.8 KB
Raw

GraphQL WG Notes - July 2022

Watch the replay: GraphQL Working Group Meetings on YouTube

Agenda

Determine volunteers for note taking (1m, Lee)

  • Benjie
  • Tim
  • Mike C

Review agenda (2m, Lee)

  • Note: We will take a 5m break, 5m before the end of the first hour. We’ll do so just before the hour so we come back precisely on the hour.

Review previous meeting's action items (5m, Lee)

  • Ready for review
  • All open action items (by last update)
  • All open action items (by meeting)
  • #971
    • Lee has done editorial; it’s now merged and is in the draft spec! 🎉
  • May
    • On Rob, stream & defer spec wording
    • Rob: Spec PR is slightly behind, a few more open questions that we should get to today, afterwards everything good to go
  • June
    • It seems like there were no specific actions from the June meeting
    • Benjie: One action item, Roman to create PR, but taken care of
    • Lee: Did editorial changes, pulled some threads
  • Feedback on CCN
    • Haven’t opened the thread yet.
    • Alex: Quick update.
      1. PR open for validation
      2. parsing/lexing merged into main (not yet released)
      3. No longer at Yelp, so not full time but will see it through (albeit slower)
      4. Discussions on error handling, and concerns around null propagation (discussions with Benjie / Meta)
      5. Will update you in future meetings
  • Still some historical items, which we can let sit for now.

Ability to add directives on extended enum values (10m, Benoit)

  • [19:17]
  • Benoit: [shares presentation]
  • Using reserved keywords may make compiling enums to certain programming languages problematic. We can escape reserved words, but unpleasant to use. Instead, can we allow users to choose the name to use?
  • Apollo tries to use GraphQL as much as possible as a way for users to configure the library
  • Apply @targetName(name: String!) directive via extend enum …
  • This isn’t valid because the spec doesn’t allow you to redefine values that already exist in the enum (only add)
  • The proposal is to allow this syntax (or any alternative that gets the job done):
extend enum ExistingEnum {
    ExistingValue @directive
}
  • Still disallow extending with an existing value if there are no directives - fail fast. Maybe disallow adding the same directive again (unless repeatable).
  • Michael: we use that in schema stitching - adding directives to fields that exist on an existing schema. (We use our own parser.)
  • Matt: This is basically a union on schemas. You recursively merge schemas. The same problem exists with input arguments, you can’t add an input argument to an existing field.
  • Michael: You’d have the same problem with argument merging. We need an algorithm for that.
  • Matt: Yes, it would make the schema merging more complicated
  • Benjie [chat]: same issue with fields? https://spec.graphql.org/draft/#sel-IAHZnCRFFABAB_Cq9e
  • Greg: enum extensions are potentially incompatible - new extra values can break clients.
  • Lee: schema extensions are an intentionally crippled version of schema merging. This wasn’t necessarily intentional, but is a side effect of that. Allowing this on enums would mean that this should also be available in other places like Benjie said with fields on object types. If you want to add an argument to a field, do you have to list the existing ones? If you don’t - does that imply removal?
  • Benjie: I wonder if schema coordinates might be useful here?
  • Tim: it could be a patch - no need to re-specify all the context.
  • Stephen: we could say that your definition has to be identical. For enums this is trivial because it’s just a single value, but another issue is that it’s impossible to remove that thing from the original schema - the original definition and the extension are kind of tied together.
  • Michael: you can build a custom merge today, what we build might not meet all the expectations. With the SDL you can already build these workflows, you just need custom merge logic. Maybe too big to add to spec?
  • Lee: what problem are we trying to solve? Benoit’s motivation was good, but it wasn’t really about extending schemas, more about applying additional metadata on a client for an upstream type. Are there more angles that might address this problem space?
  • Matt [chat]: I'd love to see a good description for three operations:
    • Schema merging
    • Schema intersections
    • Schema diffing (basically allowing add/remove/update)
    • Yeah I think the composition wg is a good space to work through these three issues
  • Yaacov [chat]: For composition wg?
  • Laurin [chat]:
    • seems like this is related to the composite schema working group
    • and might be better off discussed in there
  • Benoit: we could not use extend and instead use config files. We generally prefer to use extensions to the schema.
  • Matt: we have client-only schema files that we have a custom merge operation for. It’s not spec compliant, but it’s all on the client so it’s fine, whatever 🤷‍♂️ It feels like extensions give you all this power but in this one case doesn’t, but it’s because it’s hard to do.
  • Yaacov, Matt and Laurin suggested in the chat that this topic may be best handled in the Composition Working Group.
  • Lee: extension tools are there because people asked for them, perhaps they were less successful than we had hoped. Maybe we should follow Matt’s suggestion - rather than using the extension syntax, use the full syntax with a custom merge function that’s specific to the needs the Android client has. I don’t know this is a well explored space. It’s also valuable to explore solutions, and if it’s already specified it holds one hand behind your back. This feels like it might be a good opportunity for defining generalised schema merging rules.
  • Matt: We do a thing called schema syncing. We’d like to have a stack, where you see the changes in a diffable, small [atomic] way.
  • Stephen: this use case calls for merging in purely metadata on the graph - not trying to change anything from the client perspective. Merging metadata into the graph has been something we’ve built out a system for because different places might own different metadata and it becomes complex. We use schema coordinates for this. It might be useful to have a generalized way to apply metadata onto the graph that could accommodate this use-case without needing full merging semantics.
  • Lee: Benoit do you feel like you have enough feedback to move forward?
  • Benoit: yes definitely
  • [19:35]

Adding "meta" fields to SDL and introspection (20m, Ivan Goncharov)

  • Aim: it's super early proposal, so I want gather feedback if it's a better alternative to exposing directives through introspection.
  • Presentation
  • Ivan: [presents screen]
  • Ivan: currently we have two approaches to metadata. Exposing SDL (e.g. at Apollo), and exposing directives (e.g. GraphQL Java, HotChocolate) - gaining traction. I want to propose a third one.
  • This has evolved since the slides that I added last agenda. Since then I’ve discussed this more internally and with Benjie.
  • Benjie did a great talk about the different approaches at the GraphQL Conf.
  • Why not do SDL is an important topic, but lets skip it for now and concentrate on introspection based solutions.
  • Some directives don’t make sense in introspection.
  • Directives was designed as a kitchen sink for new syntax (syntax plus validation, no other meaning). Outside WG people use it for a lot of things, including internal things and schema transformations.
  • Neo4j lets you attach a Neo4j query to a field via a directive; not sensible to expose.
  • Directives are repeatable and chainable - if they’re transforms then they must be applied in order.
  • Therefore they need to be exposed in metadata as an array. You can’t group them.
  • Directive arguments are input types, like args. To expose them we could do an array of tuples: name, value. We use String for defaultValue already, but it’s not ideal - especially for metadata.
  • Directives are always optional - there’s no way to specify that every field must have a certain piece of metadata. If everything’s always nullable, it’s a path to disaster.
  • The best way to represent directives in introspection is the AppliedDirective approach, but it does introduce a lot of problems.
  • Introspection itself is a first-class type / first class query. Here we would have some untyped things.
  • Other issues:
    • name clashes - directives don’t have a specifiedBy and people like short names.
  • Design requirements:
    • Easy to dump all introspection including custom metadata. Useful for proxies.
    • Custom metadata should be fully representable in SDL. Metadata should be first class citizen.You should be able to run the server solely based on SDL
    • Should use native GraphQL types, enable sub-selection, deprecation, description, etc.
    • Need to be able to track who is using what metadata.
  • Analogy from other programming languages - are directives like decorators? They’re similar. TC39, they split up decorators and decorator metadata. Decorators are not preserved - they do a transformation (including setting up metadata).
  • We should decouple. Directives can set introspection properties without exposing the directive itself.
  • We already do that for @deprecate, @specifyBy, @oneOf
  • Parse it as a field, print it as a directive back.
  • [19:46]
  • Differences between directive and metadata:
  • In introspection: { __type(name: “Foo”) { fields { meta { permissions } } } }
  • How to represent in SDL? SDL really important.
  • Suggesting that we add a new type of directive (not really a directive) - you specify it with args, where it’s applied, and what the output type is to use for it.
  • Type should be compatible between input and output (we can add a validation rule)
  • Example:
type Query {
  field: String
    +foo(arg: "bar")
    +tag(name: "staging")
    +tag(name: "internal")
    @deprecate
}
  • If it is marked as “required” then field is non-nullable.
  • This was a last minute change to the slides.
  • It’s compatible with the current directive-based approach. Benjie suggested using the + symbol - it just means “expose through introspection” and it creates its own namespace so it does not compete with directives.
  • Metadata fields would become a first class citizen.
  • Challenges: no interfaces/unions, no arguments. Restriction on introspection because entire introspection needs to be dumpable. It’s similar to Benjie’s proposal, we’re split up in opinions. Benjie proposes to use the same type on input and output (struct types) whereas I propose two types.
  • Michael: why not have directive as output type?
  • Ivan: as only makes sense if it’s metadata; but people use directives for a bunch of things. We could create a subclass but it’s confusing, and the ability to distinguish between what’s shared and what isn’t shared is desired.
  • I feel this is RFC-(minus)1 or RFC0, there’s still lots to discuss.
  • Laurin: What are some examples of stuff you’d want to expose as metadata?
  • Ivan: Apollo has https://github.com/apollographql/specs which uses a lot of metadata that needs to be accessible by client.
  • Laurin: why not just be a field that the client can request rather than adding to introspection?

Counter-arguments to meta-fields proposal, (5 minutes, Roman Ivantsov). Just adding 5 minutes to the previous discussion, to have legit time

  • [20:01]
  • Roman: [shares screen]
  • Not a counter-proposal, just counter arguments against the applied directives approach.
  • My concern is with the background of the issue. What will happen in the future - we will have 2 metadata facilities because directives don’t go anywhere. Directives is recognised as a metadata facility, and then we have this new metadata thing. This is equivalent to what we have in Java - same syntax even. Used for the same purposes. From 20 years of .NET experience (and a little less in other languages); this is a very successful syntax. Meta types seem more complicated than directives. I couldn’t make sense of the example from the previous presentation.
  • We’ll need to explain why we didn’t go with directives, because it was very good with other platforms.
  • It seems that meta types don’t solve the problems, it just moves them - input vs output, etc.
  • Each team instead of adding their own directive add their own field in a huge metadata type.
  • The arguments against appliedDirectives don’t work for me - directives are limited in other languages too.
  • “Directive” word is irrelevant; historical accident.
  • Returning arg values: structs mgith help. Maybe an Any, or Map?
  • “Benjie gave example of there being too many directives” [inaccurate] “normally the number of directives is very limited - it’s never 100 if you have a reasonably competent developer.”
  • Another concern is that directives contain secrets. SDL is stored into git, and git cannot include secrets, so therefore SDL cannot include secrets.
  • If developer wants to, they can trivially filter the directives that they expose in the schema.
  • I think we should explore appliedDirectives to the very end.
  • We have a chicken and egg problem with the SDL - introspection is source of truth and doesn’t represent directives so where do directives come from?
  • Lee: I agree with a lot of the ideas and problem statements from both presentations. To be respectful for time, we should have a discussion thread to start discussing this async
  • ACTION - Ivan - open discussion thread.
  • We should have a breakout group to discuss this more, like for the composite schemas WG.
  • I’m compelled that there’s something at the end of this journey.
  • Ivan: I agree it’s a bigger topic; I don’t want to create a huge meta-discussion about everything - is it safe to say we have two separate discussions - one is introspection vs SDL, second is extending introspection with additional metadata. I’m personally only interested in the second one.
  • Roman: let’s focus only on introspection, I agree SDL is separate. I’ve already opened a discussion thread about SDL.
  • [20:14]
  • Ivan: I’ll open a second thread. We need to use the same approach as oneOf but I don’t think we’d abandon introspection in any case - the requirement for me is that any solution should be 100% compatible within SDL and introspection. I’ll put this in the list of requirements. We have 3 proposals: appliedDirective, what I’m proposing, and what Benjie’s proposing. Like we did for input unions I think we should put the competing solutions in the same document.

Formally allow recursion within ResolveAbstractValue with interfaces that implement interfaces (10m, Yaacov Rydzinski)

  • See: graphql/graphql-spec#960
  • [20:16]
  • graphql/graphql-spec#960
  • We have a step in the execution algorithm in which we resolve the abstract type to a concrete type. We allow interfaces to implement other interfaces. I’m requesting that ResolveAbstractValue could resolve to another abstract type, and we’d call ResolveAbstractType recursively.
  • If we were to do this, does this actually need to be a spec change for GraphQL.js etc to support it?
  • We can implement this in GraphQL.js without making a change in the spec; I wasn’t expecting to need a change in the spec text for this, but there were other opinions that suggested that it would.
  • Lee: why would this require a change to the spec? An outside observer would still see an abstract type resolve to a concrete type.
  • Yaacov: I don’t think there’s an observed change; but by adding it to the spec every implementation would have to allow it. That might not make sense in other languages? We wanted feedback on whether this is necessary as a spec change for GraphQL.js.
  • Michael: what feature is blocked?
  • Benjie: if I understand correctly; you’d like to add this feature to GraphQL.js but because this is the reference implementation the GraphQL.js maintainers are not in agreement that this change can be made without the spec being updated to reflect this.
  • Ivan: spec text is very precise, there doesn’t seem to be a language dependent thing here. Why should JS and C# behave differently here?
  • Michael: why do you need the abstract type? In HotChocolate we create an execution plan that resolves to concrete types.
  • Ivan: going from field type to concrete type: is it one step, or can it be many? Pet, Mammal, Cat/Dog. Pet cannot resolve to Mammal currently - must resolve directly to Cat/Dog.
  • Michael: is this not a utility function? Why should it be multiple steps? We can optimise resolve currently, but if we change this we’d have to add multiple steps.
  • Lee: adding recursion like this is a liability or at worse an attack vector.
  • Spec language is that an “internal method” is called for this resolution. There’s no reason the method on the type couldn’t call that method in another type - do it within the internal method instead of outside of it.
  • Ivan: my question: whatever we merge, other implementations follow and this doesn’t seem necessary. Something that avoids recursion in execution seems more desirable. I need objective criteria to say yes or no on the PR. Is the complexity worth adding to GraphQL.js or not?
  • Yaacov: the current PR doesn’t allow infinite recursion because the interface returned must be a member of the chain, and we can’t have cycles in the chain. I agree that it could be added in user-land.
  • Lee: I’m on the fence, but I’m of the opinion this may be reasonable. Specifically limiting it to only allowing it to reference an implemented interface. I’ll give you some feedback on your PR. No explicit conclusion.

Small update on the GraphQL-over-HTTP working group (10m, Benjie)

  • Updated specification includes "legacy watershed"
  • Restarting meetings
  • Benjie:
  • GraphQL over HTTP working group got resurrected!
  • We might get ready to ship, optimally with the next version of the spec itself
  • There was a GraphQL over HTTP WG
  • A lot of contention about the new content-type
  • There’s an issue that we can’t trust errors unless they have a 200, because it might come from a Proxy
    • Because of that, we added application/graphql+json so we know it comes from a source that knows GraphQL
  • That’s the only addition. Anything else has just been taking what people do anyways and just writing it down.
  • We basically did a watershed. Nobody should be incompliant if they don’t implement it.
  • Giving everyone 2.5y to implement (until 2025)
  • We also went through some restructuring
  • Still some feedback coming in from Apollo & Dennis regarding SSE & Websockets -> Doesn’t go into v 0, but want to make sure we are aware of it
  • Aiming to do another meeting in the next couple of weeks, as Benjie would like to present a spec soon
  • In a week, please have a look!
  • Aims: Specify which is already the case. Just talking about Query + Mutation operations, not subscription operations
  • Lee: What’s the call to action?
  • Benjie: I’d like to present next month, please get re-involved. If people can cast their eyes over it just before the next WG. Do we have a target for the next spec?
  • Lee: No, but usually we want it once a year. Last one was October. We should probably aim at October again.
  • Benjie: Please chase up all the legal changes, Lee, as we need that for the GraphQL over HTTP spec.
  • Lee: Biggest piece is to get the contributions assignment signed by everyone. We might have to do some bookkeeping, as the repo existed for quite some time

Defer/Stream (20m, Rob Richard)

  • Yielding batches of payloads from defer/stream GraphQL requests
  • Stream payload "data" field
  • [20:39]
  • (Item was moved ahead in the agenda)
  • Rob:
  • Batching of payloads for defer/stream: executing a query with a large number of payloads for defer and/or stream could cause the client to thrash.
  • Last consensus was that we want some kind of batching.
  • Async Generator or similar for payloads.
  • We might delay the first render when batching things up.
  • Could be expensive for the runloop.
  • Array of payloads being yielded felt to some like a weird API. Another option to return a payload or array of payloads; but an untyped language might crash when it sees an array unexpectedly. Ivan suggested returning an object with “incremental” containing a list of payloads. hasNext would move to the containing payload rather than on each object. At GraphQL conf we discussed this. “Data” and “incremental” could be delivered together. The incremental property could even be added to the initial payload.
  • Lee: is it always there, or only when there’s more than one.
  • Rob: Always on subsequent payloads, on initial it may be there.
  • Lee: so all non-initial payloads will have “incremental” instead of “data” giving the list of all incremental objects.
  • Rob: Yes, and Ivan points out the type definition of the response now differentiates between the initial payload and future payloads.
  • Yaacov: and with this format we can switch from a SHOULD to a MUST.
  • Michael: we’re exploring the same thing as with Apollo - it can only be done on (entity definitions)? It should be SHOULD.
  • Rob: So far we landed on SHOULD so clients still work, but servers might not support it. But let’s discuss the payload format.
  • Yaacov: if the server is choosing not to honour stream/defer on the first payload then incremental might/might not appear, or how would it intersect with the new format.
  • Rob: MUST/SHOULD is whether the directive is ignored entirely.
  • Michael: good middle ground, if you do not defer then you still put it in the incremental property and the client will get the same patches even if it would have been a MUST.
  • Lee: defer can’t be a MUST because clients that haven’t implemented the directive, and making it MUST would break it.
  • Lee: clients should not be in the business of specifying how the server behaves - they say what they need, and the server chooses how to deliver. The server might have to take a significant performance hit versus not deferring in teh first place and leaving it inline.
  • Michael: I agree 100%. We should keep it a SHOULD. In distributed schemas, it could be very beneficial to not defer in certain circumstances.
  • Yaacov: if incremental does not appear, that doesn’t mean that the data only includes what’s included in the initial request. It doesn’t help with the typings issue.
  • Ivan: I suggested this because if a client receives an initial response with hasNext: true, it needs to know which parts to return or not. Without saying MUST client must go inside the data, know all possible locations, and check if the value is there or not. This is extra work for the client. Incremental rendering, client should know if this part is finished or not.
  • Matt [chat]: One alternative way of viewing the incremental key: the client is telling you the shape of the response it wants. When it has defer it says we want the shape such that the stuff underneath belongs under "incremental"
  • Michael [chat]: I agree with that
  • Matt [chat]: So the server can choose to not promisify or delay the result but can't choose to ignore the response shape change
  • Matt [chat]: So the server can choose to not promisify or delay the result but can't choose to ignore the response shape change
  • Laurin: once a GraphQL cache has resolved a streamed/deferred response once it could resolve and rewrite the data to be more efficient.
  • Do the clients that implement defer/stream currently require that it be in this format, or are they happy receiving it all as one batch. Does it have implications having array of patches versus whole result merged.
  • Rob: relay supports receiving an array of payloads and does one render once they’re all resolved.
  • Lee: seems like we’re coming to consensus that this form is right, but the question over whether stream/defer can be ignored is separate.
  • Michael: we use label to indicate a section of data is present; but this is a crutch because we don’t have fragment modularity.
  • Matt: the key insight here that I remember from the conference is that defer actually changes the response shape that the client is expecting; if we don’t require things to end up in incremental then the client must support both possible response shapes. In everything else we do the client specifies the response shape and the server returns this one guaranteed response shape.
  • Lee: I’m still concerned about the performance degradation potential from this.
  • Matt: I don’t believe that in practice that we ever don’t defer. We do provide multiple in the same network response; we don’t use arrays - just bytes - but I don’t believe we’ve seen real problems with this.
  • Lee: the high level sense I have is that a service might want to ignore it for performance reasons is if the deferred fragments overlaps significantly with non-deferred things.
  • Matt: to some extend I think it’s the client’s responsibility to not ask for a duplicated shape like that.
  • Lee: yeah, that’s tricky for a number of reasons.
  • Rob: should vs must - a client sends a defer/stream, server can ignore it if we have SHOULD - then the response won’t be in the incremental field, it will just be in the parent payload (which could be the main data, or another defer/stream that wasn’t ignored). With MUST it would always be incremental and omitted from the parent payload.
  • Lee: nit-picking, for the typical case where we’re adding a single thing to a list there’s a lot of JSON junk, which may have some overhead.
  • Rob: one last thing, I think we should stick with “should” because it will force clients to handle both cases, and if we later change it to MUST then it won’t break existing clients. We can’t go the other way. I’m a little on the fence too, but maybe this can be argued on its own merit after this lands.
  • Rob: async feedback please on discussion #42 in robrichard/defer-stream-wg. Should we change name of “data” to “items” to make it clearer that it’s a different type.
  • Lee: thanks again for all your work on this!
  • ACTION - everyone - drop in on this.
  • [21:05]
  • Lee: lets take the other topics async.
  • Benjie: just a heads up, struct might be a replacement for oneOf, so if you’re using that… 🤷‍♂️
  • ACTION - Benjie - open discussion thread for struct
  • Roman: Lee, we’re done with the first round of fixes, should I start raising the next ones?
  • Lee: yes

[21:07]

Meeting ended, other two topics are bumped to next month:

  • Short update/reminder/discussion on previous topics: GraphQL definition, order/titles of chapters, algorithms. (10m, Roman Ivantsov)
  • Structured, Type-safe, Returnable and Union-Capable Type; and more meaty metadata musings (20m, Benjie)