Merge pull request #992 from constructive-io/feat/postgis-spatial-fix-and-tests

pyramation · web-flow · commit 1281a5342985 · 2026-04-17T14:30:16.000-07:00
fix+test(postgis): ST_GeomFromGeoJSON wrap for spatial filters + regression suite (#724)
diff --git a/.github/workflows/run-tests.yaml b/.github/workflows/run-tests.yaml
@@ -73,6 +73,10 @@ jobs:
             env: {}
           - package: graphile/graphile-connection-filter
             env: {}
+          - package: graphile/graphile-postgis
+            env: {}
+          - package: graphql/orm-test
+            env: {}
           - package: graphql/server-test
             env: {}
           - package: graphql/env
diff --git a/graphile/graphile-postgis/__tests__/connection-filter-integration.test.ts b/graphile/graphile-postgis/__tests__/connection-filter-integration.test.ts
@@ -147,6 +147,43 @@ describe('Integration: connection-filter OperatorSpec compatibility', () => {
       }
     }
   });
+
+  // Regression guard for constructive-io/constructive-planning#724.
+  //
+  // Spatial filter operators receive GeoJSON input that must be wrapped with
+  // ST_GeomFromGeoJSON(...)::<codec> before hitting PostgreSQL — PG's
+  // geometry_in / geography_in parsers do NOT accept GeoJSON text cast to
+  // geometry / geography directly.
+  //
+  // graphile-connection-filter's operatorApply falls through to
+  // `sqlValueWithCodec(resolvedInput, inputCodec)` (a raw text bind cast to
+  // the codec's sqlType) unless the operator spec overrides the binding via
+  // `resolveSqlValue`, `resolveInput`, or `resolveInputCodec`. Without one
+  // of those overrides the operator is broken end-to-end on both codecs.
+  //
+  // See plugins/within-distance-operator.ts for the correct pattern
+  // (`resolveSqlValue: () => sql.null` + manual ST_GeomFromGeoJSON wrap in
+  // resolve()).
+  it('every spec overrides value binding so GeoJSON is wrapped with ST_GeomFromGeoJSON', () => {
+    const { registered } = runFactory();
+
+    for (const { spec, operatorName, typeName } of registered) {
+      const hasBindingOverride =
+        typeof spec.resolveSqlValue === 'function' ||
+        typeof spec.resolveInput === 'function' ||
+        spec.resolveInputCodec !== undefined;
+
+      expect({
+        operatorName,
+        typeName,
+        hasBindingOverride,
+      }).toEqual({
+        operatorName,
+        typeName,
+        hasBindingOverride: true,
+      });
+    }
+  });
 });
 
 describe('Integration: type name generation matches graphile-postgis', () => {
diff --git a/graphile/graphile-postgis/__tests__/connection-filter-operators.test.ts b/graphile/graphile-postgis/__tests__/connection-filter-operators.test.ts
@@ -236,34 +236,49 @@ describe('PostGIS operator factory (createPostgisOperatorFactory)', () => {
 
   describe('SQL generation', () => {
     describe('function-based operators', () => {
-      it('generates schema-qualified SQL for public schema', () => {
+      // The input-binding contract (regression guard for #724): every
+      // operator must wrap the GeoJSON input with ST_GeomFromGeoJSON(...)
+      // — PostgreSQL's geometry_in / geography_in parsers reject raw
+      // GeoJSON text. The compiled SQL must therefore always contain the
+      // schema-qualified function call binding the JSON-encoded input as
+      // `::text` before any further casting.
+
+      it('wraps input with ST_GeomFromGeoJSON (public schema)', () => {
         const { registered } = runFactory({ schemaName: 'public' });
         const containsOp = registered.find(r => r.operatorName === 'contains');
         expect(containsOp).toBeDefined();
 
         const i = sql.identifier('col');
         const v = sql.identifier('val');
-        const result = containsOp!.resolve(i, v, null, null, {
+        const input = { type: 'Point', coordinates: [-122.4194, 37.7749] };
+        const result = containsOp!.resolve(i, v, input, null, {
           fieldName: null,
           operatorName: 'contains'
         });
         const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"public"."st_contains"("col", "val")');
+        expect(compiled.text).toBe(
+          '"public"."st_contains"("col", "public"."st_geomfromgeojson"($1::text))'
+        );
+        expect(compiled.values).toEqual([JSON.stringify(input)]);
       });
 
-      it('generates schema-qualified SQL for non-public schema', () => {
+      it('wraps input with ST_GeomFromGeoJSON (non-public schema)', () => {
         const { registered } = runFactory({ schemaName: 'postgis' });
         const containsOp = registered.find(r => r.operatorName === 'contains');
         expect(containsOp).toBeDefined();
 
         const i = sql.identifier('col');
         const v = sql.identifier('val');
-        const result = containsOp!.resolve(i, v, null, null, {
+        const input = { type: 'Point', coordinates: [-122.4194, 37.7749] };
+        const result = containsOp!.resolve(i, v, input, null, {
           fieldName: null,
           operatorName: 'contains'
         });
         const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"postgis"."st_contains"("col", "val")');
+        expect(compiled.text).toBe(
+          '"postgis"."st_contains"("col", "postgis"."st_geomfromgeojson"($1::text))'
+        );
+        expect(compiled.values).toEqual([JSON.stringify(input)]);
       });
 
       it('lowercases function names in SQL', () => {
@@ -273,89 +288,86 @@ describe('PostGIS operator factory (createPostgisOperatorFactory)', () => {
 
         const i = sql.identifier('a');
         const v = sql.identifier('b');
-        const result = op3d!.resolve(i, v, null, null, {
+        const input = { type: 'Point', coordinates: [-122.4194, 37.7749, 254] };
+        const result = op3d!.resolve(i, v, input, null, {
           fieldName: null,
           operatorName: 'intersects3D'
         });
         const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"public"."st_3dintersects"("a", "b")');
+        expect(compiled.text).toBe(
+          '"public"."st_3dintersects"("a", "public"."st_geomfromgeojson"($1::text))'
+        );
       });
-    });
 
-    describe('SQL operator-based operators', () => {
-      it('generates correct SQL for = operator', () => {
+      it('casts to geography when the operator is registered on a geography type', () => {
         const { registered } = runFactory();
-        const exactOp = registered.find(r => r.operatorName === 'exactlyEquals');
-        expect(exactOp).toBeDefined();
+        // `intersects` is registered for both geometry and geography. We
+        // want the geography variant to append `::geography` after the
+        // ST_GeomFromGeoJSON wrap so PostGIS picks the geography overload.
+        const geogIntersects = registered.find(
+          r => r.operatorName === 'intersects' && r.typeName === 'GeographyPoint'
+        );
+        expect(geogIntersects).toBeDefined();
 
         const i = sql.identifier('col');
         const v = sql.identifier('val');
-        const result = exactOp!.resolve(i, v, null, null, {
+        const input = { type: 'Point', coordinates: [-122.4194, 37.7749] };
+        const result = geogIntersects!.resolve(i, v, input, null, {
           fieldName: null,
-          operatorName: 'exactlyEquals'
+          operatorName: 'intersects'
         });
         const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"col" = "val"');
+        expect(compiled.text).toBe(
+          '"public"."st_intersects"("col", "public"."st_geomfromgeojson"($1::text)::"public"."geography")'
+        );
       });
+    });
 
-      it('generates correct SQL for && operator', () => {
+    describe('SQL operator-based operators', () => {
+      const runOp = (operatorName: string) => {
         const { registered } = runFactory();
-        const bboxOp = registered.find(r => r.operatorName === 'bboxIntersects2D');
-        expect(bboxOp).toBeDefined();
+        const op = registered.find(r => r.operatorName === operatorName);
+        expect(op).toBeDefined();
 
-        const i = sql.identifier('col');
-        const v = sql.identifier('val');
-        const result = bboxOp!.resolve(i, v, null, null, {
-          fieldName: null,
-          operatorName: 'bboxIntersects2D'
-        });
-        const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"col" && "val"');
+        const input = { type: 'Point', coordinates: [-122.4194, 37.7749] };
+        const result = op!.resolve(
+          sql.identifier('col'),
+          sql.identifier('val'),
+          input,
+          null,
+          { fieldName: null, operatorName }
+        );
+        return sql.compile(result);
+      };
+
+      it('generates correct SQL for = operator', () => {
+        expect(runOp('exactlyEquals').text).toBe(
+          '"col" = "public"."st_geomfromgeojson"($1::text)'
+        );
       });
 
-      it('generates correct SQL for ~ operator', () => {
-        const { registered } = runFactory();
-        const bboxContainsOp = registered.find(r => r.operatorName === 'bboxContains');
-        expect(bboxContainsOp).toBeDefined();
+      it('generates correct SQL for && operator', () => {
+        expect(runOp('bboxIntersects2D').text).toBe(
+          '"col" && "public"."st_geomfromgeojson"($1::text)'
+        );
+      });
 
-        const i = sql.identifier('col');
-        const v = sql.identifier('val');
-        const result = bboxContainsOp!.resolve(i, v, null, null, {
-          fieldName: null,
-          operatorName: 'bboxContains'
-        });
-        const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"col" ~ "val"');
+      it('generates correct SQL for ~ operator', () => {
+        expect(runOp('bboxContains').text).toBe(
+          '"col" ~ "public"."st_geomfromgeojson"($1::text)'
+        );
       });
 
       it('generates correct SQL for ~= operator', () => {
-        const { registered } = runFactory();
-        const bboxEqOp = registered.find(r => r.operatorName === 'bboxEquals');
-        expect(bboxEqOp).toBeDefined();
-
-        const i = sql.identifier('col');
-        const v = sql.identifier('val');
-        const result = bboxEqOp!.resolve(i, v, null, null, {
-          fieldName: null,
-          operatorName: 'bboxEquals'
-        });
-        const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"col" ~= "val"');
+        expect(runOp('bboxEquals').text).toBe(
+          '"col" ~= "public"."st_geomfromgeojson"($1::text)'
+        );
       });
 
       it('generates correct SQL for &&& operator', () => {
-        const { registered } = runFactory();
-        const ndOp = registered.find(r => r.operatorName === 'bboxIntersectsND');
-        expect(ndOp).toBeDefined();
-
-        const i = sql.identifier('col');
-        const v = sql.identifier('val');
-        const result = ndOp!.resolve(i, v, null, null, {
-          fieldName: null,
-          operatorName: 'bboxIntersectsND'
-        });
-        const compiled = sql.compile(result);
-        expect(compiled.text).toBe('"col" &&& "val"');
+        expect(runOp('bboxIntersectsND').text).toBe(
+          '"col" &&& "public"."st_geomfromgeojson"($1::text)'
+        );
       });
     });
   });
diff --git a/graphile/graphile-postgis/src/plugins/connection-filter-operators.ts b/graphile/graphile-postgis/src/plugins/connection-filter-operators.ts
@@ -221,6 +221,9 @@ export function createPostgisOperatorFactory(): ConnectionFilterOperatorFactory
     const { inflection } = build;
     const { schemaName, geometryCodec, geographyCodec } = postgisInfo;
 
+    const sqlGeomFromGeoJSON = sql.identifier(schemaName, 'st_geomfromgeojson');
+    const sqlGeographyType = sql.identifier(schemaName, 'geography');
+
     // Collect all GQL type names for geometry and geography
     const gqlTypeNamesByBase: Record<string, string[]> = {
       geometry: [],
@@ -254,6 +257,7 @@ export function createPostgisOperatorFactory(): ConnectionFilterOperatorFactory
       typeNames: string[];
       operatorName: string;
       description: string;
+      baseType: 'geometry' | 'geography';
       resolve: (i: SQL, v: SQL) => SQL;
     }
     const allSpecs: InternalSpec[] = [];
@@ -267,6 +271,7 @@ export function createPostgisOperatorFactory(): ConnectionFilterOperatorFactory
           typeNames: gqlTypeNamesByBase[baseType],
           operatorName,
           description,
+          baseType: baseType as 'geometry' | 'geography',
           resolve: (i: SQL, v: SQL) => sql.fragment`${sqlGisFunction}(${i}, ${v})`
         });
       }
@@ -281,6 +286,7 @@ export function createPostgisOperatorFactory(): ConnectionFilterOperatorFactory
           typeNames: gqlTypeNamesByBase[baseType],
           operatorName,
           description,
+          baseType: baseType as 'geometry' | 'geography',
           resolve: (i: SQL, v: SQL) => buildOperatorExpr(capturedOp, i, v)
         });
       }
@@ -292,23 +298,40 @@ export function createPostgisOperatorFactory(): ConnectionFilterOperatorFactory
     // Convert to ConnectionFilterOperatorRegistration format.
     // Each InternalSpec may target multiple type names; we expand each
     // into individual registrations keyed by typeName.
+    //
+    // The default operatorApply pipeline binds the filter value as a raw
+    // text parameter cast to the column codec's sqlType (geometry /
+    // geography). PostgreSQL's geometry_in / geography_in parsers reject
+    // GeoJSON text, so we must wrap the input with ST_GeomFromGeoJSON
+    // ourselves — see within-distance-operator.ts for the pattern.
+    //
+    // We disable the default bind via `resolveSqlValue: () => sql.null`
+    // and construct the geometry value from `input` inside resolve(),
+    // mirroring the ST_DWithin implementation.
     const registrations: ConnectionFilterOperatorRegistration[] = [];
     for (const spec of allSpecs) {
+      const geographyCast = spec.baseType === 'geography'
+        ? sql.fragment`::${sqlGeographyType}`
+        : sql.fragment``;
+
       for (const typeName of spec.typeNames) {
         registrations.push({
           typeNames: typeName,
           operatorName: spec.operatorName,
           spec: {
             description: spec.description,
             resolveType: (fieldType) => fieldType,
+            resolveSqlValue: () => sql.null,
             resolve(
               sqlIdentifier: SQL,
-              sqlValue: SQL,
-              _input: unknown,
+              _sqlValue: SQL,
+              input: unknown,
               _$where: any,
               _details: { fieldName: string | null; operatorName: string }
             ) {
-              return spec.resolve(sqlIdentifier, sqlValue);
+              const geoJsonStr = sql.value(JSON.stringify(input));
+              const geomSql = sql.fragment`${sqlGeomFromGeoJSON}(${geoJsonStr}::text)${geographyCast}`;
+              return spec.resolve(sqlIdentifier, geomSql);
             }
           } satisfies ConnectionFilterOperatorSpec,
         });
diff --git a/graphql/orm-test/__fixtures__/seed/postgis-spatial-seed.sql b/graphql/orm-test/__fixtures__/seed/postgis-spatial-seed.sql
diff --git a/graphql/orm-test/__tests__/postgis-spatial.test.ts b/graphql/orm-test/__tests__/postgis-spatial.test.ts
