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

Ensure transient key dependencies are applied #29

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

gwardwell
Copy link
Contributor

@gwardwell gwardwell commented Apr 26, 2024

Description

When one entity's key references another entity's key transitively, and the object in-between is a value type, the compound key for the referenced entity is not created correctly.

Observed result

Here's an example schema:

type Foo @key(fields: "bar { baz { bazKey } }") {
  bar: [Bar!]!
}

type Bar {
  baz: [Baz!]!
}

type Baz @key(fields: "bazId bazKey") {
  bazId: Int!
  bazKey: String!
}

This is the node-froid schema that would currently be generated:

type Bar {
  baz: [Baz!]!
}

type Baz implements Node @key(fields: "bazId bazKey") {
  "The globally unique identifier."
  id: ID!
  bazId: Int!
  bazKey: String!
}

type Foo implements Node @key(fields: "bar { __typename baz { __typename bazKey } }") {
  "The globally unique identifier."
  id: ID!
  bar: [Bar!]!
}

Note that Foo doesn't get a compound key where Foo.bar.baz includes the bazId field. This means node-froid is failing to find the transient dependency.

Expected result

The intended schema should be:

type Bar {
  baz: [Baz!]!
}

type Baz implements Node @key(fields: "bazId bazKey") {
  "The globally unique identifier."
  id: ID!
  bazId: Int!
  bazKey: String!
}

type Foo implements Node @key(fields: "bar { __typename baz { __typename bazKey bazId } }") {
  "The globally unique identifier."
  id: ID!
  bar: [Bar!]!
}

Note the bazId in the key.

Cause and fix

If we add a key to Bar like so:

type Foo @key(fields: "bar { baz { bazKey } }") {
  bar: [Bar!]!
}

type Bar @key(fields: "baz { bazKey }") {
  baz: [Baz!]!
}

type Baz @key(fields: "bazId bazKey") {
  bazId: Int!
  bazKey: String!
}

Then the transient dependency is honored:

type Bar implements Node @key(fields: "baz { bazId bazKey }") {
  "The globally unique identifier."
  id: ID!
  baz: [Baz!]!
}

type Baz implements Node @key(fields: "bazId bazKey") {
  "The globally unique identifier."
  id: ID!
  bazId: Int!
  bazKey: String!
}

type Foo implements Node @key(fields: "bar { __typename baz { __typename bazKey bazId } }") {
  "The globally unique identifier."
  id: ID!
  bar: [Bar!]!
}

This points to the value type breaking the inheritance of keys. This PR solves the problem, allowing the transient key dependencies to pass through value types resulting in the desired schema:

type Bar {
  baz: [Baz!]!
}

type Baz implements Node @key(fields: "bazId bazKey") {
  "The globally unique identifier."
  id: ID!
  bazId: Int!
  bazKey: String!
}

type Foo implements Node @key(fields: "bar { __typename baz { __typename bazKey bazId } }") {
  "The globally unique identifier."
  id: ID!
  bar: [Bar!]!
}

To achieve this, we calculate keys based on dependencies for value types so the transient dependencies can be determined and skip applying the keys to the value types when we output their final schema. So, essentially, all objects get keys, but we only include keys on entities in the actual schema.

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change
  • Refactor
  • Documentation
  • Other (please describe)

Checklist

  • I have read the
    contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@@ -549,7 +549,7 @@ describe('FroidSchema class', () => {
bookId: String!
}

type Magazine implements Node @key(fields: "magazineId publisher { __typename address { __typename country } }") {
type Magazine implements Node @key(fields: "magazineId publisher { __typename address { __typename country postalCode } }") {
Copy link
Contributor Author

@gwardwell gwardwell Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key now correctly inherits the postalCode field in its key. This was a previously unidentified gap.

You may notice that the Book entity is not getting the country field. This is because Author.address is external and, therefore, its fields won't be resolved through the Book. Since Author.address fields are never resolved (because they're external) the Address fields don't need to be merged into the Author or Book keys.

@gwardwell gwardwell marked this pull request as ready for review April 29, 2024 14:40
@gwardwell gwardwell requested a review from a team as a code owner April 29, 2024 14:40
@gwardwell gwardwell merged commit fa1a6dc into main Apr 29, 2024
5 checks passed
@gwardwell gwardwell deleted the gwardwell_fix_transitive_compound_key_generation branch April 29, 2024 17:22
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

Successfully merging this pull request may close these issues.

2 participants