Skip to content

fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient#724

Open
pyramation wants to merge 1 commit intomainfrom
devin/1771210551-fix-graphile-test-adapted-client
Open

fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient#724
pyramation wants to merge 1 commit intomainfrom
devin/1771210551-fix-graphile-test-adapted-client

Conversation

@pyramation
Copy link
Contributor

@pyramation pyramation commented Feb 16, 2026

fix: add adapted pg client and bypass grafast middleware in test context

Summary

Fixes graphile-test's runGraphQLInContext to properly handle grafast v5's execution model. Two key problems are addressed:

  1. arrayModerowMode translation: grafast sends { arrayMode: true } to the pg client, but node-postgres expects { rowMode: 'array' }. The old simple passthrough (callback(pgClient)) didn't translate this, causing PgSelect to see 0-length result arrays for mutations like sign_up and provision_database_with_user.

  2. grafast middleware overriding test contextValue: grafast's middleware replaces the test-provided withPgClient (which uses the test transaction) with a new one from the pool. Adding middleware: null to the execute() call bypasses this, preserving the test's adapted client.

The old withPgClient was a thin wrapper that simply passed the raw pg Client through and ignored caller-provided pgSettings. It is replaced with a full adapted client implementation that provides:

  • Query queuing to serialize concurrent database calls
  • Proper pgSettings handling with savepoint wrapping
  • arrayModerowMode translation for pg client compatibility
  • Notice collection
  • Nested transaction support via savepoints

This is the upstream fix for constructive-db PR #494.

Review & Testing Checklist for Human

  • Verify middleware: null is acceptable: This disables ALL grafast middleware during test execution. Confirm this doesn't mask important production behavior that should be tested. Check if there's a more targeted way to prevent just the withPgClient override.
  • Review the adapted client implementation against @dataplan/pg's own adaptor: The ~200 lines of new code (newNodePostgresPgClient, makeNodePostgresWithPgClient_inner, etc.) reimplement parts of @dataplan/pg's node-postgres adaptor inline. Consider whether this should import from @dataplan/pg instead to avoid drift.
  • Verify rowMode: 'array' as const cast: The as QueryConfig assertion on the query object with rowMode may hide type mismatches if pg types change.
  • Test end-to-end via constructive-db: Run the 4 previously-failing test suites in constructive-db/application/app: gql.test.ts, modules.test.ts, database-provision-graphql.test.ts, provision-database-with-user-graphql.test.ts. These all exercise sign_up and provision_database_with_user mutations through grafast.
  • Check for race conditions in query queuing: The $$queue symbol-based serialization is complex concurrent code. Verify no deadlocks or race conditions under concurrent test execution.

Test Plan

  1. Publish this graphile-test version (or use a local link)
  2. Update constructive-db to use the new version
  3. Run pnpm test in constructive-db/application/app - all 4 GraphQL test suites should pass
  4. Verify CI passes on constructive-db PR fix: close prompter before exportMigrations to avoid double Inquirerer instances #494

Notes

  • The NotificationRecord<string, unknown> type change for convertNotice is correct - pg doesn't actually emit Notification objects for notices, it emits plain objects with those fields.
  • The withPgClientFromPgService import was removed as it's no longer used.
  • This fix was developed and tested in Devin session 90f948ec61dc4d53996f9f0fcf2f9b17 by @pyramation.

Open with Devin

…thPgClient

The grafast middleware was overriding the test's contextValue, replacing
the adapted withPgClient (which handles arrayMode -> rowMode translation)
with a new one from the pool. This caused PgSelect to receive 0-length
result arrays for mutations like sign_up and provision_database_with_user.

Setting middleware: null in the execute call bypasses the middleware,
allowing the test's contextValue with the adapted client to be used
directly.
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment on lines +303 to +304
} catch (_e2) {
console.error(`Error occurred whilst rolling back: ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Rollback error handler logs original error instead of the rollback failure error

When a withTransaction callback throws error e and the subsequent rollback query also fails with _e2, the console.error on line 304 logs the original error e rather than the rollback error _e2. The rollback failure reason is silently discarded.

Root Cause

In the catch block at graphile/graphile-test/src/context.ts:302-305:

} catch (_e2) {
    console.error(`Error occurred whilst rolling back: ${e}`);
}

The message "Error occurred whilst rolling back" implies the logged value explains why the rollback failed, but it actually logs e (the original callback error), not _e2 (the rollback failure). The original error e is already re-thrown at line 306 and will be caught/logged by callers, so logging it here adds no new information. Meanwhile, the actual rollback error _e2 — the useful diagnostic — is completely lost.

Impact: When debugging transaction rollback failures (e.g., connection dropped during rollback), the operator sees the original error repeated rather than the rollback-specific error, making it harder to diagnose the root cause of the rollback failure.

Suggested change
} catch (_e2) {
console.error(`Error occurred whilst rolling back: ${e}`);
} catch (_e2) {
console.error(`Error occurred whilst rolling back (original error: ${e}): ${_e2}`);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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