nmaquet
commented
2 years ago Hey @gmac! Thanks for submitting this. What would you say, as an alternative to modifying the existing @boundary directive, to introduce a new one, as follows:
directive @getter(public: Boolean, types: [String!]) on FIELD_DEFINITIONIn this way, we can eventually deprecate @boundary without any potentially confusing or surprising overlap. Also, I think the name boundary makes sense when referring to a type, but it has always felt a bit weird to add it to a field definition. The name getter (implicitly boundary type getter) makes more sense IMHO.
Other than that, I support the change, which, as you said has quite a number of advantages:
- the DRYness is nice
- it's one fewer mistake users can make
- the supporting code will be simpler
- making some getter fields public can be useful
- it opens the door to abstract boundary types in the future
gmac
Bramble's present implementation of the
@boundarydirective 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
@boundarydirective 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.@boundaryconfiguration 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.@boundarydirective are hidden from the gateway; they should be allowed to remain public (https://github.com/movio/bramble/issues/84).Proposed SDL refactors
We propose remedying these problems by refactoring the
@boundarydirective and merge script to use the following signature:With this new signature, the following happens:
@boundarydirective no longer appears on types (we'll just ignore it on types, thus being backwards compatible)@boundaryquery somewhere in the graph, with some rules:@boundaryquery 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.idwithout a@boundaryquery 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
@namespacedirective, 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.