movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
497 stars 55 forks source link

Feature Request: Shared "utility" types (distinct from boundary types) #232

Open Quantumplation opened 10 months ago

Quantumplation commented 10 months ago

There are a few types that I want to use consistently across all services, but which aren't "owned" by any specific service.

For example, I have a DateTime type defined as

type DateTime {
  unix: Int!
  format(layout: String! = "2006-01-02 15:04:05")
}

It doesn't make sense to give this an ID and make it a boundary type, since it's not "owned" by any particular service; there's simply a common library that defines the struct and resolver methods that we import and use in other resolvers.

If I just redefine it across multiple services, I get a "conflicting non boundary type" error. It'd be nice to define some utility types that can appear and be identical across multiple services.

Two ways I can think of to do this:

but perhaps the project maintainers will have better ideas.

Quantumplation commented 10 months ago

After digging through the code a bit, from an execution side, I think this could be implemented by setting the location on fields of a valid utility type to nil, and when building the query plan, if location is nil, use the location from the parent.

Quantumplation commented 10 months ago

I know I'm probably lighting up your notifications, but I got it working; however, it feels super hacky, and I don't know the codebase well enough to know if I'm introducing other problems, so I'd love some guidance there.

As a proof of concept though, it boiled down to:

In merge.go:

func areIdentical(a, b *ast.Definition) bool {
  if a.Name != b.Name {
    return false
  }

  if a.Kind != b.Kind {
    return false
  }

  if len(a.Fields) != len(b.Fields) {
    return false
  }

  for _, f := range a.Fields {
    hasField := false
    for _, g := range b.Fields {
      if f.Name == g.Name {
        hasField = true
        if f.Type.String() != g.Type.String() {
          return false
        }
        break
      }
    }
    if !hasField {
      return false
    }
  }
  return true
}

// ... in mergeTypes

    if newVB.Kind == ast.Scalar {
      result[k] = &newVB
      continue
    }

    // Allow redeclaring types across services, so long as they're identical
    if areIdentical(va, &newVB) {
      va.Position.Src.Name = "multiple"
      continue
    }

    if !hasFederationDirectives(&newVB) || !hasFederationDirectives(va) {

then in plan.go:

// ... In RegisterUrl(...)
func (m FieldURLMap) RegisterURL(parent string, field string, location string) {
  key := m.keyFor(parent, field)
  if _, existing := m[key]; existing {
    // A given type/field combination exists in multiple locations, so set location to empty string
    // When executing the plan, this will cause it to inherit the parent location
    m[key] = ""
  } else {
    m[key] = location
  }
}

// ... In URLFor(...)
  if !exists {
    return "", fmt.Errorf("could not find location for %q", key)
  }
  if value == "" {
    // The entry exists, meaning we recognize the field
    // but is empty string, meaning there's not a canonical location
    // so inherit the parent location to query from the service we're querying from
    return parentLocation, nil
  }
  return value, nil

I can now have value types that are shared across services.

asger-noer commented 8 months ago

Hi @Quantumplation,

Sorry for not getting back to you sooner.

Shared value types seems like something that could be solved with a @shareable directive. This would make it very clear that the type coming from the individual services are the same and can be merged into one type on the federated graph. I think I'd prefer the clarity of this approach rather than the silent "magic" way of merging types if they are identical.

type DateTime @shareable {
  unix: Int!
  format(layout: String! = "2006-01-02 15:04:05")
}

I think this would be a great addition to bramble. Since I'm not completely sure how much work would need to be done to make this work I'd like to get @pkqk opinion as well.

Quantumplation commented 8 months ago

@asger-noer Oh, that's an excellent solution :) Not sure why I didn't think of that.

I'm currently running a fork that uses the silent magic merging solution, but it's a little brittle (good enough for my use case, but I wouldn't want to submit it as a PR). Having it be explicitly opt-in would be great.

pkqk commented 8 months ago

There's a few other things to consider. Mostly about dynamic service changes. If we enforce that a type has to be identical it makes it difficult to extend the type without removing all the services that expose it and adding them all back in, which makes zero downtime deployments difficult.

It might be better to ensure that a shared type is a subset of the current combined fields, and then null the fields from the query if they're not available on the service being queried, it will also be useful for input types.

I'd also suggest a word like @common for the directive as that feels like it matches the usage of @boundary as descriptive noun rather than an adjective like @shareable.

gmac commented 7 months ago

Apollo handles this incremental merging of identical shared types across services with an “inaccessible” directive. A field marked anywhere as inaccessible is omitted from the schema. In this way, a new field can be added to a shared type in one service as inaccessible, and then added incrementally to all other services, and finally have the “inaccessible” directive removed to activate it everywhere all at once.