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

@inaccessible object types within fragment-nested @requires can cause Fed 2 composition to throw #1863

Closed
sachindshinde opened this issue May 17, 2022 · 2 comments

Comments

@sachindshinde
Copy link
Contributor

Description
When an object type is @inaccessible and is used in a fragment-nested @requires, it can cause Fed 2 composition to throw. (To be clear, I'm not sure what the root cause is; it may be something else.)

Example
When using @apollo/composition@2.0.2, composing the following two subgraphs will cause composeServices() to throw.

# Subgraph A
extend schema
@link(
  url: "https://specs.apollo.dev/federation/v2.0",
  import: ["@key", "@shareable"]
)

type Query @shareable {
  dummy: Entity
}

type Entity @key(fields: "id") {
  id: ID!
  data: Foo
}

interface Foo {
  foo: String!
}

interface Bar implements Foo {
  foo: String!
  bar: String!
}

type Baz implements Foo & Bar @shareable {
  foo: String!
  bar: String!
  baz: String!
}

type Qux implements Foo & Bar @shareable {
  foo: String!
  bar: String!
  qux: String!
}
# Subgraph B
extend schema
@link(
  url: "https://specs.apollo.dev/federation/v2.0",
  import: ["@key", "@shareable", "@external", "@requires", "@inaccessible"]
)

type Query @shareable {
  dummy: Entity
}

type Entity @key(fields: "id") {
  id: ID!
  data: Foo @external
  requirer: String! @requires(fields: "data { foo ... on Bar { bar ... on Baz { baz } ... on Qux { qux } } }")
}

interface Foo {
  foo: String!
}

interface Bar implements Foo {
  foo: String!
  bar: String!
}

type Baz implements Foo & Bar @shareable @inaccessible {
  foo: String!
  bar: String!
  baz: String!
}

type Qux implements Foo & Bar @shareable {
  foo: String!
  bar: String!
  qux: String!
}

The stacktrace emitted is:

node_modules/@apollo/federation-internals/src/definitions.ts:186
  return type.kind == 'InterfaceType';
              ^
TypeError: Cannot read property 'kind' of undefined
    at isInterfaceType (node_modules/@apollo/federation-internals/src/definitions.ts:186:15)
    at isAbstractType (node_modules/@apollo/federation-internals/src/definitions.ts:250:10)
    at advanceWithOperation (node_modules/@apollo/query-graphs/src/graphPath.ts:1862:27)
    at advanceSimultaneousPathsWithOperation (node_modules/@apollo/query-graphs/src/graphPath.ts:1502:19)
    at ConditionValidationResolver.advanceState (node_modules/@apollo/composition/src/validate.ts:492:65)
    at ConditionValidationResolver.validateConditions (node_modules/@apollo/composition/src/validate.ts:478:30)
    at node_modules/@apollo/composition/src/validate.ts:455:17
    at node_modules/@apollo/query-graphs/src/conditionsCaching.ts:39:26
    at canSatisfyConditions (node_modules/@apollo/query-graphs/src/graphPath.ts:1368:22)
    at advancePathWithDirectTransition (node_modules/@apollo/query-graphs/src/graphPath.ts:1217:33)
@pcmanus
Copy link
Contributor

pcmanus commented Jun 13, 2022

That issue is actually fixed by the patch in #1873. More precisely, it's the part of the commit there where I said:

There was also another place within query planning (in graphPath.ts more precisely) where the code was looking up the possible implementation types, in the supergraph, of the required @inaccessible type, and was using the supergraph API to do so, which was incorrect for the same reason. This commit fix that problem as well, passing the full supergraph instead.

I've pushed the repro of this ticket as a unit test on that #1873 PR for good measure, but I'll close both this and #1866 when than PR lands.

pcmanus pushed a commit to pcmanus/federation that referenced this issue Jun 28, 2022
…uire

The `@require(fields:)` argument can use some type conditions to require
data specific to a subtype. And those condition can even be on an
`@inaccessible` type since "inaccessibility is about the federated API
schema, not about subgraphs.

However, this wasn't properly working because when the execution code
was extracting the "requirements" data to pass it to the subgraph having
the `@require`, the (federated) API schema we used, and consequently
when the code tried to get the definition for the type condition in the
`@require`, it didn't find any definition (since the type in question is
`@inaccessible` and so not in said API schema) and as a result the
corresponding fields were not included in the subgraph query.

This commit fixes this issue, ensuring we pass the full supergraph
(which _has_ the `@inaccessible` elements) to the execution code so
that supergraph can be used when extracting required data.

There was also another place within query planning (in `graphPath.ts`
more precisely) where the code was looking up the possible
implementation types, in the supergraph, of the required `@inaccessible`
type, and was using the supergraph API to do so, which was incorrect
for the same reason. This commit fix that problem as well, passing
the full supergraph instead.

Fixes apollographql#1866, apollographql#1863
pcmanus pushed a commit that referenced this issue Jun 29, 2022
…uire (#1873)

The `@require(fields:)` argument can use some type conditions to require
data specific to a subtype. And those condition can even be on an
`@inaccessible` type since "inaccessibility is about the federated API
schema, not about subgraphs.

However, this wasn't properly working because when the execution code
was extracting the "requirements" data to pass it to the subgraph having
the `@require`, the (federated) API schema we used, and consequently
when the code tried to get the definition for the type condition in the
`@require`, it didn't find any definition (since the type in question is
`@inaccessible` and so not in said API schema) and as a result the
corresponding fields were not included in the subgraph query.

This commit fixes this issue, ensuring we pass the full supergraph
(which _has_ the `@inaccessible` elements) to the execution code so
that supergraph can be used when extracting required data.

There was also another place within query planning (in `graphPath.ts`
more precisely) where the code was looking up the possible
implementation types, in the supergraph, of the required `@inaccessible`
type, and was using the supergraph API to do so, which was incorrect
for the same reason. This commit fix that problem as well, passing
the full supergraph instead.

Fixes #1866, #1863
@pcmanus
Copy link
Contributor

pcmanus commented Nov 18, 2022

but I'll close both this and #1866 when than PR lands

Well, better late than never.

@pcmanus pcmanus closed this as completed Nov 18, 2022
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

2 participants