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

Conditional trapping of inhibits; assertion; improve Relay NodeID compatibility #2019

Merged
merged 44 commits into from
Apr 11, 2024

Conversation

benjie
Copy link
Member

@benjie benjie commented Apr 11, 2024

Description

Another PR relating to #2013; follow up to #2015.

For relay we need to be able to support a query like this:

query Q($authorId: ID) {
  allPosts(condition: { author: $authorId }) {
    nodes {
      body
    }
  }
}

When we do this, what we want changes based on the value of $authorId:

  • $authorId undefined: no condition applied, select all posts (WHERE true)
  • $authorId null: find posts which have no author (WHERE author_id is null)
  • $authorId valid Author ID: find posts by this author (WHERE author_id = $1)
  • $authorId is invalid or references the wrong type: raise an error to inform the user of their mistake

It wasn't possible to do this in plans previously (you had to do it in the step class' execute method instead, like with a resolver) but this PR makes it possible as demonstrated by the changes to PgNodeIdAttributesPlugin.

Note that this.addDependency(step, true) is no longer supported; instead you must use this.addDependency({ step, skipDeduplication: true }). I doubt this affects many people.

Performance impact

Unknown.

Security impact

None known.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

…s the output plan and the buckets used to produce it to help track down where issues are arising.
…, true)` is no longer supported; instead (and equivalently) please use: `this.addDependency({ step, skipDeduplication: true })`. Note `this.addDependency(step)` (with no additional arguments) is unaffected.
…an choose to only trap under certain circumstances.
…hout weird results if you pass an incompatible node id (e.g. a 'Post' ID where a 'User' ID was expected) - significantly improves the Relay preset.
Copy link

changeset-bot bot commented Apr 11, 2024

🦋 Changeset detected

Latest commit: cf8af9d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
grafast Patch
graphile-build-pg Patch
postgraphile Patch
@dataplan/pg Patch
@localrepo/grafast-bench Patch
@dataplan/json Patch
@grafserv/persisted Patch
grafserv Patch
ruru-components Patch
@localrepo/grafast-website Patch
graphile-build Patch
graphile-utils Patch
pgl Patch
graphile-export Patch
graphile Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benjie benjie merged commit eb3cd0b into main Apr 11, 2024
24 checks passed
@benjie benjie deleted the trap-if branch April 11, 2024 18:07
@benjie benjie mentioned this pull request May 10, 2024
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

1 participant