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

Allow @boundary enum, @boundary scalar, and @boundary input #10

Open
nmaquet opened this issue Mar 8, 2021 · 13 comments
Open

Allow @boundary enum, @boundary scalar, and @boundary input #10

nmaquet opened this issue Mar 8, 2021 · 13 comments
Labels
2.0 release enhancement New feature or request
Milestone

Comments

@nmaquet
Copy link
Contributor

nmaquet commented Mar 8, 2021

Shared enums would be really useful to define standard values across services.
For example:

enum Gender @boundary {
    MALE
    FEMALE
    OTHER
    UNKNOWN
}

Similarly, shared scalars would be really useful to standardize the encoding of dates and timestamps for example.
For example:

scalar ISODate @boundary

Shared input types are a bit less useful, but one great use case is to standardize pagination:

input PageInput @boundary {
     cursor: ID
     pageSize: Int! = 100
}

I think the proper semantics for these new boundary types is that they each appear exactly once in the resulting merged schema. The schema merge operation would fail if the definitions of the boundary enums / scalars / inputs differ in any way. This does mean that changing the definition of any of these down the line requires a simultaneous upgrade of all the affected services, but I think that's a price worth paying.

Thoughts @Ackar @pkqk ?

@nmaquet nmaquet added the enhancement New feature or request label Mar 8, 2021
@pkqk
Copy link
Member

pkqk commented Mar 8, 2021

For things like PageInput I think the ability to have a shared bit of schema that you could include would help.

It also needs the ability to be defined in multiple services so will need @boundary or similar directive.

@nmaquet
Copy link
Contributor Author

nmaquet commented Mar 8, 2021

Yeah I think reusing @boundary is fine here.

@pkqk
Copy link
Member

pkqk commented Mar 8, 2021

Yeah having the same directive is a good idea I think, since the semantics are essentially the same.

There's two things here, we want to be able to have scalars, enums and inputs with the same type name in various services, and possibly share definitions to reduce duplication of definition and the associated drift.

In the scalar case this is easy as it's just a statement that this type name exists.

In the enum case we can union all the values from various services into the final enum, although this means that services could get values they are not expecting in their own subset of type system?

Same with input types, if we do a union of the inputs fields then again one service can receive fields it doesn't expect.

It might be safer to have one service that defines the common types we use, mark these with a @boundary directive, and then have a way to include this services schema into others as a common package?

@nmaquet
Copy link
Contributor Author

nmaquet commented Mar 8, 2021

In the enum case we can union all the values from various services into the final enum, although this means that services could get values they are not expecting in their own subset of type system?

I would only allow @boundary enums to be defined as the exact same set of values in all services. The alternative causes more issues than is worth dealing with IMHO.

@nmaquet
Copy link
Contributor Author

nmaquet commented Mar 8, 2021

Same with input types, if we do a union of the inputs fields then again one service can receive fields it doesn't expect.

I was confused here for a sec, I thought you meant the GraphQL union, not the set union :)

I think we should take the more conservative approach of only allowing @boundary inputs that are exactly the same in all services, just like enums.

@nmaquet
Copy link
Contributor Author

nmaquet commented Mar 8, 2021

and then have a way to include this services schema into others as a common package?

I don't think this is necessary. I think it's fine (and even preferable) for each service schema to be defined independently. Checking that a service's schema is mergeable in the graph should be done during CI.

@pkqk
Copy link
Member

pkqk commented Mar 9, 2021

My reasoning is there is already quite a bit of boilerplate that needs to be included in each service to join the gateway. eg.

directive @boundary on OBJECT
directive @namespace on OBJECT

interface Node {
  id: ID!
}

type Service {
  name: String! # unique name for the service
  version: String! # any string
  schema: String! # the full schema for the service
}

type Query {
  service: Service!
  node(id: ID!): Node
}

The locations which we have to update this if we start adding/changing things will start growing, it may even make it difficult to roll out an addition. Eg:
If we add a value to an enum, then that services definition doesn't match the others so it won't be accepted into the gateway, meaning we'd have to pull all services out of the gateway that define it, deploy the new service, update all the other services that also use that enum and roll them out.

@nmaquet
Copy link
Contributor Author

nmaquet commented Mar 9, 2021

@pkqk I totally get your point that rolling out changes to @boundary will be a tricky operation. I do believe, however, that centralizing the definition in one spot will make this worse and not better. The issue is that, by centralizing the enum definition, you get lured into a false sense of security - you may think that updating the one common definition is sufficient. In reality, though, each service will need to be carefully and individually reviewed when such a change takes place.

@pkqk
Copy link
Member

pkqk commented Mar 9, 2021

Summary from IRL discussion:

We decided for our use case we could handle the downtime of removing some services from the gateway and rolling out schema updates in a co-ordinated way.

We might later want to be able to do this in a highly available way, one idea we thought of was to have a way to tell bramble a schema change was acceptable and any new services joining with the change would be accepted.

@gmac
Copy link
Contributor

gmac commented Oct 23, 2021

FWIW, I wrote these sorts of validations for the Node GraphQL Tools schema stitching package, and landed on the following rules:

  • boundary scalars are fine as long as they’re treated as strings. Limiting custom scalars from appearing across serivces is prohibitively restrictive. These aren’t even really boundaries, IMO. We actually went so far as to allow scalar mappings, so that —say— an ID in the gateway could proxy a String in a subservice.

  • for boundary inputs, schema stitching traditionally allowed input divergence because inputs are always filtered to a subschema, and invalid input was dropped. We kept that for backwards compatibility, but added strong warnings. My preference would be to always require arguments and inputs to be consistent.

  • then, boundary enums with divergent values are fine in read-only contexts, meaning an enum is allowed divergent values as long as it never appears as an argument or input value. Once an enum is used as input, it must be consistent across services.

@nmaquet nmaquet modified the milestone: Bramble 2.0 Nov 7, 2021
@gmac
Copy link
Contributor

gmac commented Nov 10, 2021

To distill this down to a basic 2.0 spec, my suggestion would be:

  • Custom scalars are allowed to appear across services, and they shouldn’t require a boundary declaration. They’re just leaf values of a common type.
  • Input objects should be verified consistent across services with no divergence allowed, IMO.
  • Enum values SHOULD allow divergence… if for no other reason than the logistics of rolling out a new value across services one at a time. Apollo even broke on this logistical hurtle in Federation v2. A validation against divergent enums appearing as input values safeguards against practical repercussions.

@pkqk
Copy link
Member

pkqk commented Nov 10, 2021

@gmac wouldn't disallowing divergence in Input objects also making rolling out changes to the object one service at a time difficult. Our current plan is removing the affected services and adding all the new ones but it's not a method I'm happy with.

If one service adds a new field to an Input object, but it's being passed to a service that doesn't yet accept it, the two outcomes I can think of are:

  • deny the request, even though it matches the merged schema
  • remove the field values before passing to the service that doesn't recognize them

Both options could be surprising, though removing the fields is similar to your comment about divergent Enum values?

@gmac
Copy link
Contributor

gmac commented Nov 10, 2021

That’s a fair point on the input divergence also being blocking. I know GQL Tools handles this by always filtering inputs to the subservice schema, but I don’t love that the approach requires subservice schemas to be individually present for gateway operations (which works against #99). Maybe the gateway composition could cache a minimal delta of fields that must be removed from specific services when this applies? Denying requests feels like a tricky outcome that could have small mistakes translate into large outages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 release enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants