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

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

Open
Quantumplation opened this issue Nov 18, 2023 · 6 comments

Comments

@Quantumplation
Copy link
Contributor

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:

  • when merging, if two types are identical (same name, same fields, fields have the same types), choose either one as the merged type; if they differ, throw an error
  • add a config option for a shared.graphql, a file with common type definitions that can be ignored in upstream services

but perhaps the project maintainers will have better ideas.

@Quantumplation
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

pkqk commented Jan 22, 2024

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
Copy link
Contributor

gmac commented Mar 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants