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

Improve support for @requires Federation directives #2991

Open
clayne11 opened this issue Apr 7, 2024 · 0 comments
Open

Improve support for @requires Federation directives #2991

clayne11 opened this issue Apr 7, 2024 · 0 comments

Comments

@clayne11
Copy link
Contributor

clayne11 commented Apr 7, 2024

What happened?

#2884 landed recently to add support for nested selection sets in @requires directives. This implementation isn't great, however. There are three significant gaps:

  1. The resolved fields are rendered as a map[string]interface{}. We know the types for this model, however, from the schema. We should be generating a typesafe model for these fields.
  2. I would expect a separate resolver function to be generated for each @requires field since they should be treated like independently derived fields, at least in my opinion.
  3. There's no way to access any errors that occurred when fetching the upstream fields. This is important because an upstream error can mean something very different from null. For example, if I’m trying to compute driverName and do @requires(fields: "driver { name }" and driver comes back null due to an error the correct response might be to throw an error and return null, but if the driver is truly null I might return “No driver assigned”.

Example

From the tests:

type PlanetRequiresNested @key(fields: "name") {
    name: String! @external
    world: World! @external
    worlds: [World!] @external
    size: Int! @requires(fields: "world{ foo }")
    sizes: [Int!] @requires(fields: "worlds{ foo }")
}

Current generated code:

// PopulatePlanetRequiresNestedRequires is the requires populator for the PlanetRequiresNested entity.
func (ec *executionContext) PopulatePlanetRequiresNestedRequires(ctx context.Context, entity *PlanetRequiresNested, reps map[string]interface{}) error {
	entity.Name = reps["name"].(string)
	entity.World = &World{
		Foo: reps["world"].(map[string]interface{})["foo"].(string),
	}
	return nil
}

What did you expect?

In an ideal world, I would like to see something like the following be generated:

type PlanetRequiresNestedSizeRequires interface {
	Worlds() (PlanetRequiresNestedSizeWorldRequires, error)
}

type PlanetRequiresNestedSizeWorldRequires interface {
	Foo() (string, error)
}

// PopulatePlanetRequiresNestedRequires is the requires populator for the PlanetRequiresNested entity.
func (ec *executionContext) PopulatePlanetRequiresNestedRequiresSize(ctx context.Context, requires PlanetRequiresNestedSizeRequires) (int32, error) {
	worlds, err := requires.Worlds()
	if err != nil {
		return 0, err
	}

	foo, err := worlds.Foo()
	if err != nil {
		gqlError, ok := err.(*gqlerror.Error)
		if ok {
			return 0, gqlError.Err
		}

		return 0, err
	}

	switch foo {
	case "":
		return -1, nil

	case "Bar":
		return 10, nil

	default:
		return int32(len(foo)), nil
	}
}

type PlanetRequiresNestedSizesRequires interface {
	Worlds() (PlanetRequiresNestedSizeWorldRequires, error)
}

type PlanetRequiresNestedSizesWorldRequires interface {
	Foo() (string, error)
}

// PopulatePlanetRequiresNestedRequires is the requires populator for the PlanetRequiresNested entity.
func (ec *executionContext) PopulatePlanetRequiresNestedRequiresSizes(ctx context.Context, requires PlanetRequiresNestedSizesRequires) ([]int32, error) {
	worlds, err := requires.Worlds()
	if err != nil {
		return nil, err
	}

	foo, err := worlds.Foo()
	if err != nil {
		gqlError, ok := err.(*gqlerror.Error)
		if ok {
			return nil, gqlError.Err
		}

		return nil, err
	}

	switch foo {
	case "":
		return []int32{}, nil

	case "Bar":
		return []int32{10}, nil

	default:
		return []int32{1, 2, 3}, nil
	}
}

This would meet all of the requirements from above:

  1. I have type safety for my inputs.
  2. The fields I'm @requires-ing are treated independently.
  3. I can distinguish upstream errors from null values.

versions

Running on master of gqlgen

Changes to the Federation specification

This will also require an extension to the Federation specification to actually forward the errors to the gqlgen server. This could look something like this on the wire:

{
  "query": "query ($representations: [_Any!]!) {\n  _entities(representations: $representations) {\n    ... on GeoLocation {\n      latitude\n      longitude\n    }\n  }\n}",
  "variables": {
    "representations": [
      {
        "__typename": "GeoLocation",
        "name": "Foo",
        "bar": "First"
      },
      {
        "__typename": "GeoLocation",
        "name": "Blah",
        "bar": null
      },
      {
        "__typename": "GeoLocation",
        "name": null,
        "bar": null
      }
    ]
  },
  "extensions": {
    "representations": [
      {
        "errors": []
      },
      {
        "errors": [
          {
            "message": "Couldn't fetch bar due to an error",
            "path": [
              "bar"
            ]
          }
        ]
      },
      {
        "errors": [
          {
            "errors": [
              // no error for "bar" since it was actually null, not null due to an error
              {
                "message": "Couldn't fetch foo due to an error",
                "path": [
                  "foo"
                ]
              }
            ]
          }    
        ]
      }
    ]
  }
}

We add representations an array in extensions that mirrors the representations array. Each entry in the array maps by index to the entry in the representations array.

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

1 participant