fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient#724
fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient#724pyramation wants to merge 1 commit intomainfrom
Conversation
…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 EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| } catch (_e2) { | ||
| console.error(`Error occurred whilst rolling back: ${e}`); |
There was a problem hiding this comment.
🟡 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.
| } catch (_e2) { | |
| console.error(`Error occurred whilst rolling back: ${e}`); | |
| } catch (_e2) { | |
| console.error(`Error occurred whilst rolling back (original error: ${e}): ${_e2}`); |
Was this helpful? React with 👍 or 👎 to provide feedback.
fix: add adapted pg client and bypass grafast middleware in test context
Summary
Fixes
graphile-test'srunGraphQLInContextto properly handle grafast v5's execution model. Two key problems are addressed:arrayMode→rowModetranslation: grafast sends{ arrayMode: true }to the pg client, but node-postgres expects{ rowMode: 'array' }. The old simple passthrough (callback(pgClient)) didn't translate this, causingPgSelectto see 0-length result arrays for mutations likesign_upandprovision_database_with_user.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. Addingmiddleware: nullto theexecute()call bypasses this, preserving the test's adapted client.The old
withPgClientwas a thin wrapper that simply passed the raw pgClientthrough and ignored caller-providedpgSettings. It is replaced with a full adapted client implementation that provides:pgSettingshandling with savepoint wrappingarrayMode→rowModetranslation for pg client compatibilityThis is the upstream fix for constructive-db PR #494.
Review & Testing Checklist for Human
middleware: nullis 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 thewithPgClientoverride.@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/pginstead to avoid drift.rowMode: 'array' as constcast: Theas QueryConfigassertion on the query object withrowModemay hide type mismatches ifpgtypes change.constructive-db/application/app:gql.test.ts,modules.test.ts,database-provision-graphql.test.ts,provision-database-with-user-graphql.test.ts. These all exercisesign_upandprovision_database_with_usermutations through grafast.$$queuesymbol-based serialization is complex concurrent code. Verify no deadlocks or race conditions under concurrent test execution.Test Plan
graphile-testversion (or use a local link)constructive-dbto use the new versionpnpm testinconstructive-db/application/app- all 4 GraphQL test suites should passNotes
Notification→Record<string, unknown>type change forconvertNoticeis correct -pgdoesn't actually emitNotificationobjects for notices, it emits plain objects with those fields.withPgClientFromPgServiceimport was removed as it's no longer used.