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

non-nullable @external field that are not part of @key fieldset blows up federation #2357

Closed
dariuszkuc opened this issue Sep 7, 2022 · 9 comments
Labels
federation Related to Apollo federation help wanted Extra attention is needed

Comments

@dariuszkuc
Copy link
Contributor

What happened?

If schema specifies non-nullable @external fields that are not part of the @key field set, entity resolution will blow up trying to coerce nil to a target type even if they are not part of the query. Those fields will only be correctly coerced if end user select a field that references those external fields using @requires field set. It appears that the current logic assumes that those fields will always be populated in entity representation.

Example use case from https://github.com/apollographql/apollo-federation-subgraph-compatibility. Given a type definition

extend type User @key(fields: "email") {
  averageProductsCreatedPerYear: Int @requires(fields: "totalProductsCreated yearsOfEmployment")
  email: ID! @external
  name: String @override(from: "users")
  totalProductsCreated: Int @external
  yearsOfEmployment: Int! @external
}

@requires directive allows to reference external fields that are necessary for calculating its value. Those @external fields will be provided in the entity representation ONLY if field referencing them is requested (i.e. router will only include totalProductsCreated and yearsOfEmployment in the user entity representation if averageProductsCreatedPerYear is in query selection set). Attempting to run a query without @requires field blows up the execution, e.g.

# blows up
query userEntity ($representations: [_Any!]!) {
    _entities(representations: $representations) {
        ...on User { email name }
    }
}

# works
query userEntity ($representations: [_Any!]!) {
    _entities(representations: $representations) {
        ...on User { email name averageProductsCreatedPerYear }
    }
}

What did you expect?

Query is executed successfully even if those fields are referenced by the selection set (i.e. they are not populated).

Minimal graphql.schema and models to reproduce

Start example reviews server from https://github.com/99designs/gqlgen/tree/v0.17.16/_examples/federation

Execute following query

query userEntity ($representations: [_Any!]!) {
    _entities(representations: $representations) {
        ...on User { id }
    }
}

Variables

{
    "representations": [
        {
            "__typename": "User",
            "id": "123"
        }
    ]
}

Execution blows up at https://github.com/99designs/gqlgen/blob/v0.17.16/_examples/federation/reviews/graph/generated/federation.go#L124 with interface conversion: interface {} is nil, not map[string]interface {}

versions

  • go run github.com/99designs/gqlgen version v0.17.16
  • go version go1.19 darwin/arm64
dariuszkuc added a commit to dariuszkuc/apollo-federation-subgraph-compatibility that referenced this issue Sep 7, 2022
Update `gqlgen` integration with latest schema changes.

Related:

* resolves: apollographql#184
* `@requires` is not supported due to 99designs/gqlgen#2357
dariuszkuc added a commit to apollographql/apollo-federation-subgraph-compatibility that referenced this issue Sep 7, 2022
Update `gqlgen` integration with latest schema changes.

Related:

* resolves: #184
* `@requires` is not supported due to 99designs/gqlgen#2357
@StevenACoffman
Copy link
Collaborator

Can you please see if #2371 addressed your issue? If not, can you submit a PR?

@dariuszkuc
Copy link
Contributor Author

dariuszkuc commented Sep 15, 2022

👋 referenced PR fixed directive definitions which is great but does not solve the underlying issue.

Issue here is that codegen assumes that ALL @external fields will always be populated in the _entities query. Existing code works fine if external fields are nullable but blows up on external non-nullable @required fields (which are not part of the @key fieldset), i.e. code from the example repro

case "findUserByID":
  // correct, resolves ID from the @key(fields: "id")
  id0, err := ec.unmarshalNID2string(ctx, rep["id"])
  if err != nil {
    return fmt.Errorf(`unmarshalling param 0 for findUserByID(): %w`, err)
  }

  entity, err := ec.resolvers.Entity().FindUserByID(ctx, id0)
  if err != nil {
    return fmt.Errorf(`resolving Entity "User": %w`, err)
  }

  // BELOW WILL BLOW UP
  //  -> while host is not nullable, _entities query may not specify it in the representation as query may not be referencing this field
  entity.Host.ID, err = ec.unmarshalNString2string(ctx, rep["host"].(map[string]interface{})["id"])
  if err != nil {
    return err
  }
  // THIS WILL ALSO BLOW UP due to the same reason as above
  entity.Email, err = ec.unmarshalNString2string(ctx, rep["email"])
  if err != nil {
    return err
  }
  list[idx[i]] = entity
  return nil
}

While this use case may not be clear from the example app schema as gateway would never attempt to resolve just the User.id from this subgraph. If we slightly modify the schema

extend type User @key(fields: "id") {
    id: ID! @external
    host: EmailHost! @external
    email: String! @external
    reviews: [Review] @requires(fields: "host {id} email")
    # new local field resolved by this subgraph
    foo: String
}

If we now get a query asking for just User.foo, the query will blow up due to the reason stated above. You will only be able to resolve User.foo if query also request User.reviews data.

@dariuszkuc
Copy link
Contributor Author

dariuszkuc commented Sep 15, 2022

IMHO the simplest solution would be to check whether the representation map contains entries for those non-nullable fields and only then attempt to populate corresponding fields.

As for the PR -> sorry but won't be able to work on this anytime soon.

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Sep 15, 2022

Ok, I burned through my OSS hours for this week, but would fast-track reviewing any PR for this. Thanks for providing more information!

@StevenACoffman StevenACoffman added help wanted Extra attention is needed federation Related to Apollo federation labels Sep 15, 2022
@vamshiaruru-virgodesigns

Hi people, other than the issue discussed in this issue, there's another issue with requires as well, that I have mentioned in this issue: #2559

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Feb 22, 2023

@shawnhugginsjr @jclyons52 @lleadbet Hey, if one of you has a chance to work on fixing our outstanding issue with Apollo Federation support, I would love a PR!

@mihirpmehta
Copy link

Facing similar issue where getting "interface conversion: interface {} is nil, not map[string]interface {}" error in subgraph's federation go when that subgraph has dependency on other subgraph entity's field.

@StevenACoffman
Copy link
Collaborator

According to https://www.apollographql.com/docs/federation/v1/federation-spec/#scalar-_fieldset the selection can only be a fieldset

Is this different in v2 of the federation spec?

@dariuszkuc
Copy link
Contributor Author

fixed in #2884

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
federation Related to Apollo federation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants