Skip to content

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

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@pyramation pyramation closed this Feb 18, 2026
devin-ai-integration Bot pushed a commit that referenced this pull request Apr 17, 2026
Adds regression coverage for constructive-io/constructive-planning#724
(PostGIS spatial filter operators emit SQL that binds GeoJSON as raw text
cast to geometry/geography, which PostgreSQL's input parsers reject).

Three layers:

1. ORM integration test (graphql/orm-test/__tests__/postgis-spatial.test.ts)
   — ~60 tests, all run through the generated ORM against live PG:
     A. GeoJSON input shape binding smoke test (Point, LineString, Polygon,
        MultiPoint, MultiLineString, MultiPolygon, GeometryCollection) on
        both geometry and geography Point columns.
     B. Every ORM-exposed operator (26 geometry, 6 geography) plus
        withinDistance on geometry+geography.
     C. Column-type showcase: one representative query per non-Point
        concrete subtype (Polygon, MultiPolygon, LineString, MultiPoint,
        MultiLineString, GeometryCollection, PointZ).
     D. Combinations with AND/OR/NOT logical filters and scalar filters.
     E. Edge cases: isNull, empty polygon, CRS-qualified GeoJSON.
     F. Explicit mirrors of the agentic-db #724 regression-guard.
   Seeded via __fixtures__/seed/postgis-spatial-seed.sql following the
   mega-seed convention (seed.sqlfile loader, no inline seed strings).

2. Unit-level structural guard (graphile/graphile-postgis/__tests__/
   connection-filter-integration.test.ts) — verifies every spatial
   operator spec defines resolveSqlValue, resolveInput, or
   resolveInputCodec so GeoJSON is wrapped with ST_GeomFromGeoJSON.

3. CI matrix wiring (.github/workflows/run-tests.yaml) — adds
   graphile/graphile-postgis and graphql/orm-test to the CI matrix. These
   packages were previously untested in CI, which is why the regression
   was able to land on main without tripping any workflow.
devin-ai-integration Bot pushed a commit that referenced this pull request Apr 17, 2026
… spatial operators

Fixes #724. All 26 PostGIS spatial operators registered by
createPostgisOperatorFactory() were relying on the default
resolveSqlValue pipeline, which binds the GeoJSON object as raw
text cast to ::geometry / ::geography. Postgres' geometry_in and
geography_in parsers do not accept GeoJSON text — they require
ST_GeomFromGeoJSON() wrapping.

Every spatial operator now overrides resolveSqlValue to sql.null and
wraps the input with ST_GeomFromGeoJSON($1::text) in the resolve
function, with an optional ::geography cast for geography-based
operators. This matches the pattern already used correctly by
within-distance-operator.ts.

Also:
- Updates connection-filter-operators.test.ts to assert the new
  ST_GeomFromGeoJSON wrapping contract (218 tests still pass).
- Fixes a handful of test-data issues in postgis-spatial.test.ts
  found while running against live PostGIS 3.4:
    * containsProperly(point, point) returns TRUE (PostGIS semantics)
    * bboxOverlapsOrLeftOf excludes LA (east of polygon's right edge)
    * SF_NY_MULTILINESTRING needs cities as explicit vertices so it
      works in both planar (geometry) and geodesic (geography) math
    * loc3D typo'd as loc3d in two tower tests
- Skips 2 withinDistance cases with xit + FIXME: separate
  schema-visibility issue where WithinDistanceInput is not surfaced
  on GeometryInterfaceFilter; unrelated to the #724 binding fix.
devin-ai-integration Bot pushed a commit that referenced this pull request Apr 17, 2026
… spatial operators

Fixes #724.

All 26 PostGIS spatial operators registered by
createPostgisOperatorFactory() were relying on the default
resolveSqlValue pipeline, which binds the GeoJSON object as raw text
cast to ::geometry / ::geography. Postgres' geometry_in and
geography_in parsers do not accept GeoJSON text — they require
ST_GeomFromGeoJSON() wrapping. Any spatial filter with a GeoJSON input
therefore failed at runtime with 'parse error - invalid geometry'.

Every spatial operator now overrides resolveSqlValue to sql.null and
wraps the input with ST_GeomFromGeoJSON($1::text) in the resolve
function, with an optional ::geography cast for geography-based
operators. This matches the pattern already used correctly by
within-distance-operator.ts.

Unit tests in connection-filter-operators.test.ts are updated to assert
the new ST_GeomFromGeoJSON wrapping contract. The broader regression
suite (graphql/orm-test/__tests__/postgis-spatial.test.ts) lands via
PR #989 and flips green once this change is merged.
devin-ai-integration Bot pushed a commit that referenced this pull request Apr 17, 2026
Found while running the suite against PostGIS 3.4; none are bugs in
the spatial operators — they were over-strict or incorrect assertions
in the test file itself:

- containsProperly(point, point): PostGIS returns TRUE for a point
  against itself. Expectation flipped from [] -> [SF].
- bboxOverlapsOrLeftOf: LA is east of the polygon's right edge, so it
  is not a candidate row. Removed LA from expected.
- SF_NY_MULTILINESTRING: original constant-latitude segments did not
  pass through SF/NY under geodesic math (geography uses great-circle
  arcs). Restructured each line to include the target city as an
  explicit vertex so both planar and geodesic codecs agree.
- loc3D column mis-cased as loc3d in two PointZ tower tests.
- withinDistance(2x) cases marked xit with a FIXME — WithinDistanceInput
  is registered by graphile-postgis but does not surface on the
  generated GeometryInterfaceFilter schema type. Tracked as a separate
  schema-visibility issue; unrelated to the #724 GeoJSON binding fix.
pyramation added a commit that referenced this pull request Apr 17, 2026
…-and-tests

fix+test(postgis): ST_GeomFromGeoJSON wrap for spatial filters + regression suite (#724)
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