Skip to content

Fix bug with trap() around lists/connections - #3128

Merged
benjie merged 14 commits into
mainfrom
fix-trap-list-interaction
Aug 7, 2026
Merged

Fix bug with trap() around lists/connections#3128
benjie merged 14 commits into
mainfrom
fix-trap-list-interaction

Conversation

@benjie

@benjie benjie commented Aug 7, 2026

Copy link
Copy Markdown
Member

Consider a plan that wants to skip unnecessary work, such as:

function myPlan($parent, fieldArgs) {
  const { $ids } = fieldArgs;
  const $records = myResource.find();
  $records.where(sql => sql`${$records}.id = ANY(${$records.placeholder(
    inhibitOnEmpty($ids),
    listOfCodec(TYPES.int)
  )})`);
  return trap($records, TRAP_INHIBITED, { valueForInhibited: "EMPTY_LIST" });
}

$records, being a PgSelectStep, is a .items() capable step. It actually represents at runtime an object, with the items list inside of it. But now we're returning trap() (which is a __FlagStep), which did not support .items() - so when the dependency is not inhibited, we just return the PgSelectStep value verbatim... and that's not a list, so we return null! trap() breaks our list!

The fix is straightforward: have __FlagStep implement the $step.items() pattern, re-wrapping the resulting access to the underlying step with the same flag.

Connections

For connections things are much more complex. Connections need to not just represent an empty list but potentially deal with aggregates and other complexities. We can't know which values to use for aggregates (string_agg over an empty list may be "", array_agg maybe [], count() may be 0, etc) so we can't sensibly create a default. Having the user trap(..., valueForInhibit: "EMPTY_LIST") doesn't make sense for a connection because it expects an object with more details: cursors, etc. Basically... I timed out figuring out a good way to make this work. In the end I decided to forbid trap() with connections; instead we should teach the underlying steps how to avoid doing work if it's unnecessary without leaning into the inhibit system. If you use inhibit in a field that returns a connection, you may well end up with that field returning null rather than an empty connection. Recommendation: don't inhibit with connections for now.

@changeset-bot

changeset-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3101d99

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

This PR includes changesets to release 3 packages
Name Type
@dataplan/pg Patch
grafast Patch
postgraphile 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 force-pushed the fix-trap-list-interaction branch 2 times, most recently from 9556296 to 71aa5ad Compare August 7, 2026 13:00
@benjie
benjie force-pushed the fix-trap-list-interaction branch from 71aa5ad to 3be0b58 Compare August 7, 2026 13:03
@benjie
benjie enabled auto-merge August 7, 2026 13:19
@benjie
benjie merged commit c9ad4b2 into main Aug 7, 2026
66 checks passed
@benjie
benjie deleted the fix-trap-list-interaction branch August 7, 2026 13:38
@github-actions github-actions Bot mentioned this pull request Aug 7, 2026
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