gqlgen icon indicating copy to clipboard operation
gqlgen copied to clipboard

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

Open dariuszkuc opened this issue 2 years ago • 4 comments

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 avatar Sep 07 '22 18:09 dariuszkuc

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

StevenACoffman avatar Sep 15 '22 20:09 StevenACoffman

👋 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 avatar Sep 15 '22 21:09 dariuszkuc

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.

dariuszkuc avatar Sep 15 '22 21:09 dariuszkuc

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 avatar Sep 15 '22 22:09 StevenACoffman

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: https://github.com/99designs/gqlgen/issues/2559

@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!

StevenACoffman avatar Feb 22 '23 20:02 StevenACoffman

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.

mihirpmehta avatar Apr 21 '23 19:04 mihirpmehta

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?

StevenACoffman avatar Jun 14 '23 17:06 StevenACoffman

fixed in https://github.com/99designs/gqlgen/pull/2884

dariuszkuc avatar Feb 23 '24 00:02 dariuszkuc