-
Notifications
You must be signed in to change notification settings - Fork 55
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
Refactor merge script to streamline @boundary
directives
#134
Comments
Hey @gmac! Thanks for submitting this. What would you say, as an alternative to modifying the existing directive @getter(public: Boolean, types: [String!]) on FIELD_DEFINITION In this way, we can eventually deprecate Other than that, I support the change, which, as you said has quite a number of advantages:
|
Sounds good! |
FWIW, I ended up doing pretty much this implementation for Ruby schema stitching: https://github.com/gmac/graphql-stitching-ruby. Huge compliments to Bramble, you folks provided a lot of inspiration for that project. |
@gmac Wow, what a cool project! Thanks for sharing and for the shoutout. Multi-key support, configurable directive name, implicit boundary type declaration, support for both schema-first and code-first schemas, ... this looks really comprehensive. Very impressive! |
@nmaquet thanks! Under the hood, it uses flat json-based delegation mapping with a very similar schema to Bramble; I really like how you folks did that. The skeleton of the planner is also very much Bramble. It pulls in GraphQL Tools ideas for remote delegation strategies where overlapping locations are permitted. I was looking to hit the simplicity of Bramble with some more of the flexibility in Tools. Fun new toy for our Ruby ecosystem. That said, I am working on the official Shopify gateway these days and alas we went with Rust. I do know Bramble got some use in one of our smaller API offerings though. Hope all is well with your team. Still a big fan of your work and I keep an eye on what you’re up to over here. |
Bramble's present implementation of the
@boundary
directive has several indirections, and could be greatly streamlined. Myself and @exterm are considering some uses and would be interested in pursing some refactors of the merge script. While there's discussion of supporting config alternatives to directives, we believe that a refined SDL could work in tandem with this config objective, and improve the overall implementation of the tool in a backwards-compatible manner.Current SDL limitations
@boundary
directive is required on both types and queries. This mapping is complex and allows devs to create config errors for themselves. It should only be necessary on boundary queries, and the types inferred from the queries.@boundary
configuration is required on types and queries in all services. This is awkward when a service has an ID-only boundary type (type Gizmo { id:ID! }
) that will never require an inbound request to fetch it, yet must provide a query for the boundary contract.@boundary
directive are hidden from the gateway; they should be allowed to remain public (Allow public boundaries #84).Node
interface-based federation. #96).Proposed SDL refactors
We propose remedying these problems by refactoring the
@boundary
directive and merge script to use the following signature:With this new signature, the following happens:
@boundary
directive no longer appears on types (we'll just ignore it on types, thus being backwards compatible)@boundary
query somewhere in the graph, with some rules:@boundary
query that yields a type appearing in only one service isn't actually a boundary; we'll allow the directive though because you may be incrementally rolling out service schemas with this new boundary.id
without a@boundary
query is an error (a boundary query is required to access unique data).@boundary(public: true)
will include the query in the gateway schema, and omit the boundary directive.@boundary(types: ["Gizmo", "Gadget"])
will limit the scope of boundary types provided by a query, allowing for boundary queries to (eventually) return abstract types. This is mainly thinking around the scenario where a type may be available through multiple queries in the service, and we want to direct the gateway to a specific query for the type. This is kind of an edge case, and probably doesn't need to be a first-round feature.All told these rules are backwards compatible, simplify the SDL, and lend themselves to being represented in a configuration-only manner.
Namespace
The downside here is that this does create inconsistencies with the
@namespace
directive, which I’d probably leave untouched for now. In general, namespace seems finicky to me because it makes assumptions about type root-ness, which isn’t guaranteed in the GraphQL type system (the object type assigned as the root query may again be returned from anywhere in the graph, thus root access is circumstantial).Curious on @nmaquet and the Movio team’s take on this refactor before pursuing it. This has been gnawing me for a while, and now we have multiple teams prototyping with Bramble. Would be nice to get boundaries spruced up for them.
The text was updated successfully, but these errors were encountered: