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

Always add ...Exists field on virtual foreign key constraints #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NicolasMahe
Copy link

Hi everyone,

This PR forces the addition of the ...Exists field on virtual foreign key constraints.

As virtual foreign key constraints are not applied on the database level, it's possible to end up with empty connections on the GraphQL API when adding virtual foreign key constraints with smart tags.

The PgConnectionArgFilterForwardRelationsPlugin was only adding the ...Exists field if one of the columns used in the foreign key constraint is nullable. This is very good for database foreign keys, but not enough when adding virtual foreign keys to expose these connections on the GraphQL API, as the connection can still be null.


Here is the diff this PR produces on a simplified GraphQL schema:

type Ownership {
  """Reads a single `Asset` that is related to this `Ownership`."""
  asset: Asset
  assetId: String!
}

type Asset {
  """Reads and enables pagination through a set of `Ownership`."""
  ownerships(
    """Read all values in the set after (below) this cursor."""
    after: Cursor

    """Read all values in the set before (above) this cursor."""
    before: Cursor

    """
    A condition to be used in determining which values should be returned by the collection.
    """
    condition: OwnershipCondition

    """
    A filter to be used in determining which values should be returned by the collection.
    """
    filter: OwnershipFilter

    """Only read the first `n` values of the set."""
    first: Int

    """Only read the last `n` values of the set."""
    last: Int

    """
    Skip the first `n` values from our `after` cursor, an alternative to cursor
    based pagination. May not be used with `last`.
    """
    offset: Int

    """The method to use when ordering `Ownership`."""
    orderBy: [OwnershipsOrderBy!] = [PRIMARY_KEY_ASC]
  ): OwnershipsConnection!
}

input OwnershipFilter {
  """Filter by the object’s `asset` relation."""
  asset: AssetFilter

+  """A related `asset` exists."""
+  assetExists: Boolean

  """Filter by the object’s `assetId` field."""
  assetId: StringFilter
}

And the virtual foreign key constraint is defined as:

export default makeJSONPgSmartTagsPlugin({
  version: 1,
  config: {
    class: {
      Ownership: {
        tags: {
          foreignKey: [
            '("assetId") references "Asset" ("id")|@fieldName asset|@foreignFieldName ownerships',
          ],
        },
      },
    },
  },
})

@@ -245,7 +245,8 @@ const PgConnectionArgFilterForwardRelationsPlugin: Plugin = (builder) => {
);

const keyIsNullable = !keyAttributes.every((attr) => attr.isNotNull);
if (keyIsNullable) {
const isFake = (constraint as unknown as { isFake?: boolean }).isFake;
Copy link
Author

@NicolasMahe NicolasMahe Apr 1, 2023

Choose a reason for hiding this comment

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

Is there a type similar to PgConstraint but that includes isFake?: boolean?
Maybe it's actually missing from type PgConstraint as type PgType do have isFake?: boolean:
https://github.com/graphile/graphile-engine/blob/29fa78a91df5d9865420fee429918cf3f7010c1e/packages/graphile-build-pg/src/plugins/PgIntrospectionPlugin.d.ts#L75-L102

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.

1 participant