Postgraphile v5 Support (#58) - #66
Conversation
After bumping to the v5 package versions, tests would fail with the
following error:
```
● Test suite failed to run
ReferenceError: TextEncoder is not defined
> 1 | import * as pg from "pg";
| ^
2 |
3 | export async function withPgPool<T = any>(
4 | cb: (pool: pg.Pool) => Promise<T>
at Object.<anonymous> (node_modules/pg/lib/crypto/utils-webcrypto.js:22:21)
at Object.<anonymous> (node_modules/pg/lib/crypto/utils.js:8:20)
at Object.<anonymous> (node_modules/pg/lib/crypto/sasl.js:2:16)
at Object.<anonymous> (node_modules/pg/lib/client.js:5:12)
at Object.<anonymous> (node_modules/pg/lib/index.js:3:14)
at Object.<anonymous> (__tests__/helpers.ts:1:1)
at Object.<anonymous> (__tests__/schema.minimal_type.test.ts:2:1)
```
Based on [information from @SimenB](
jsdom/jsdom#2524 (comment)
), it seems like you probably should not be using
`jest-environment-jsdom` to start with if you need access to
`TextEncoder` or `TextDecoder`.
Based on [this](
https://stackoverflow.com/a/72369912/1137077
) answer on SO, I was able to verify that `@jest-environment node` at
the top of test files fixed the issue. However, it would seem more
logical to apply this as a global setting, given that postgraphile is
meant to run in a node context.
After setting `testEnvironment: jest-environment-node` in the global
config, I found that tests again started failing. However, the new
failures seem related to the v5 changes to plugins which is expected at
this stage.
The previous `moduleResolution: node` setting prevent importing types from `graphile-build-pg/pg-introspection`. `NodeNext` was chosen based on https://github.com/graphile/crystal/blob/91e87ab6516490a4cc7b7fc6400efb7623fbd331/graphile-build/graphile-build/tsconfig.json#L9
`postgraphile-core` is no more according to documentation: https://postgraphile.org/postgraphile/next/migrating-from-v4/#postgraphile-core
This seems to be better in line with other v5 plugins and the new `GraphileConfig.Preset` type.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
50762eb to
9e8dd58
Compare
| (attribute.extensions as any).postgisTypeModifier = | ||
| pgAttribute.atttypmod; |
There was a problem hiding this comment.
I'm as yet undecided on the best way to handle type modifiers in V5.
There was a problem hiding this comment.
I prototyped this against benjaie/crystal variants branch (graphile/crystal#3109) locally: built it and linked @dataplan/pg / graphile-build-pg (and their shared deps: grafast, graphile-build, graphile-config, pg-sql2, pg-introspection, graphql) into node_modules via symlinks, per your suggestion.
Reworked PostgisRegisterTypesPlugin to build the modified geometry/geography codecs in pgCodecs_findModifiedPgCodec (decoding the PostGIS typmod into subtype/Z/M, baseCodec: event.baseCodec), then map each one to its narrowed GraphQL type via setGraphQLTypeForPgCodec once the interfaces/object types are registered. That let me delete the manual per-field type lookup from PostgisColumnsPlugin; it now only overrides the SQL-computing plan, and gets its output type for free from core's normal codec→type resolution.
Results:
- All 65 existing tests pass unchanged.
- Since narrowing now lives on the codec instead of being recomputed per-attribute, it also correctly narrows
geometry(...)-typed view columns (spot-checked manually) with no extra code. - Side note: function return types don't carry a typmod in
pg_catalogat all, so that case isn't fixable on our end regardless of approach.
Prototype is on prototype/crystal-3109-codec-level-typmod (not part of this PR yet, since it depends on the unmerged pgCodecs_findModifiedPgCodec/baseCodec API and would silently regress the current, working attribute-level narrowing if run against the published graphile-build-pg).
Happy to open this as the real change the moment #3109 ships! Let me know if the shape looks right in the meantime.
There was a problem hiding this comment.
cc @TBA-Lucas
I've also fixed mutations and JSON geography inserts. My prod application works fine now 🙌
There was a problem hiding this comment.
Awesome, thanks for pushing this forward!
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Not a thorough review; but it's looking good! Try and minimize the difference with previous code as that makes review easier (and also eases migration for people who may have custom inflectors or other integrations/extensions). Thanks for your work on this!
945249d to
818de0b
Compare
818de0b to
a311f06
Compare
dargmuesli
left a comment
dargmuesli
left a comment
There was a problem hiding this comment.
I'm trying out real world use of this at the moment. There are still things to discuss for sure that I'm not certain about yet. I was a (pure) consumer of these things before and just relied on them working, now this is me looking into their core which I always like to do when I have the time. Now there is the missing 20% or 10% maybe that I would spend very much time on compared to its result so maybe I should not do this. But I'd definitely collaborate on this further! If more brains share how to get this done, I'll keep this PR up to speed.
1179293 to
25b9a29
Compare
|
@benjie I think this is ready for a full review now from my side, I have this running for some time in my dev setup now and outputs seem to fit! |
|
I tested the PR but I have issues with geometry(geometry, 4326) columns from Postgis returning "geom": null in postgraphile v5. I only get GeometryGeometry as subtype, it seems that the plugin is currently unable to infer the specific subtype like GeometryPoint. Since our postgraphile v4 server is doing that just fine with the same database, I assume that due to migrating it from postgraphile v4 to v5 the PostGIS plugin is now stricter about how it decides geometry subtypes? |
|
I noticed an issue writing to the DB too, reading worked fine for my use case. I'll look into it shortly. |
npm/pnpm run the prepare lifecycle script (not prepack) when installing a package directly from a git repository, since published tarballs already ship a built dist/ but git checkouts don't. Without this, depending on this branch directly (e.g. via a git URL) installs a package with no dist/index.js.
…raphy columns Two bugs found while investigating a reported GeoJSON input error: 1. PostgisColumnsPlugin's field plan assumed every record was a PgSelectSingleStep (which has `.select()`), but insert/update/delete mutation steps (PgInsertSingleStep/PgUpdateSingleStep/PgDeleteSingleStep) don't have that method - only `.get()`. So reading a geometry/geography field back from any create/update/delete mutation payload threw "$record.select is not a function". Fixed by branching on step type: `.select()` for queries (unchanged), and for mutations, build the same SQL expression via `pgClassExpression()` referencing the row's own `.alias` directly - the same approach these steps' own `.get()` uses for plain attributes. 2. The base geometry/geography codec's `toPg` was an identity function, so an incoming GeoJSON object (from the `GeoJSON` input scalar) was sent to Postgres as a JSON.stringify'd string. This happened to "work" for `geometry` columns only because `geometry_in` undocumentedly lenient-parses GeoJSON text - `geography_in` does not, and fails hard with "parse error - invalid geometry" (this was the actual server-side failure behind a mutation that looked, in GraphiQL/Ruru's variable editor, like a client-side "GeoJSON expects string/number/boolean" linter false positive - that lint warning is real but unrelated; this parse error is what actually broke the request). Added src/geoJsonToWkt.ts to convert GeoJSON to WKT/EWKT explicitly in `toPg`, which both `geometry_in` and `geography_in` accept identically - verified against Point, 3D Point (Z), LineString, Polygon, MultiPolygon, and GeometryCollection round-tripped through real create mutations. All 65 existing tests still pass.
4 participants
Description
Further work on the v5 migration started by @FelixZY in #60.
Resolves #58
Performance impact
unknown
Security impact
unknown
Checklist
yarn lint:fixpasses.yarn testpasses.RELEASE_NOTES.mdfile (if one exists).