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

Optimize multiple @boundary fields lookup #51

Closed
pkqk opened this issue Jul 8, 2021 · 2 comments
Closed

Optimize multiple @boundary fields lookup #51

pkqk opened this issue Jul 8, 2021 · 2 comments
Assignees

Comments

@pkqk
Copy link
Member

pkqk commented Jul 8, 2021

Currently undocumented is the ability to use a @boundary resolver that looks up a list of objects instead of a single object, i.e.

type Query {
  getFoos(ids: [ID!]!): [Foo!]! @boundary
}

instead of

type Query {
  getFoo(id: ID!): Foo @boundary
}

However there are a few issues:
While there is some code that expects the list boundary lookup, the validator complains about the signature incorrectly. Seems like it also expects the return type signature to be [Foo!] instead of [Foo!]!.

In some cases it then fails to merge the fields onto the object from another service.

There are some example services on the federation-id-deduplication branch. toomanyfoos exposes some lookup fields to get a list of Foo objects. zoot adds some fields to the Foo object.

Run with:

PORT=8091 go run ./examples/toomanyfoos/ &
PORT=8092 go run ./examples/zoot/ &
BRAMBLE_SERVICE_LIST="http://localhost:8091/query http://localhost:8092/query" go run ./cmd/bramble

With the foos resolver present in zoot the query:

query {
  toManyFoos {
    id
    bar
  }
}

returns

{
  "errors": [
    {
      "message": "Cannot query field \"bar\" on type \"Foo\".",
      "locations": [
        {
          "line": 8,
          "column": 5
        }
      ],
      "extensions": {
        "code": "GRAPHQL_VALIDATION_FAILED"
      }
    }
  ],
  "data": null
}

with the ID list resolver removed it returns the correct:

{
  "data": {
    "toManyFoos": [
      {
        "id": "0",
        "bar": false
      },
      {
        "id": "1",
        "bar": false
      },
      {
        "id": "2",
        "bar": false
      },
      {
        "id": "3",
        "bar": false
      },
      {
        "id": "4",
        "bar": false
      },
      {
        "id": "5",
        "bar": false
      },
      {
        "id": "6",
        "bar": false
      },
      {
        "id": "7",
        "bar": false
      },
      {
        "id": "8",
        "bar": false
      },
      {
        "id": "9",
        "bar": false
      },
      {
        "id": "0",
        "bar": false
      },
      {
        "id": "1",
        "bar": false
      },
      {
        "id": "2",
        "bar": false
      },
      {
        "id": "3",
        "bar": false
      },
      {
        "id": "4",
        "bar": false
      },
      {
        "id": "5",
        "bar": false
      },
      {
        "id": "6",
        "bar": false
      },
      {
        "id": "7",
        "bar": false
      },
      {
        "id": "8",
        "bar": false
      },
      {
        "id": "9",
        "bar": false
      }
    ]
  }
}

Once this is working it would also be good if bramble could deduplicate the ids it looks up on the second service if the set is not unique, reducing the number of looks sent downstream. i.e. TooManyFoos returns a list of 20 items but there are only 10 unique items, this should cause either 10 foo lookups on zoot instead of 20, or a lookup of id ids on foos if the bulk boundary resolver is present.

@gmac
Copy link
Contributor

gmac commented Oct 29, 2021

FWIW, the signature for boundary array queries would best use [Foo]! as the return type. We should always assume that a collection will be returned, but it may contain null positions when mapping IDs to potentially missing records. Using not-null for list items will corrupt the entire results set in response to a single record-not-found.

@nmaquet
Copy link
Contributor

nmaquet commented Nov 1, 2021

This was fixed by d9902cc
@gmac Good point I'll create a separate issue for this.

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