Postgraphile v5 Support (#58)

[dargmuesli]

Description

Further work on the v5 migration started by @FelixZY in #60.
Resolves #58

Performance impact

unknown

Security impact

unknown

Checklist

• My code matches the project's code style and yarn lint:fix passes.
• I've added tests for the new feature, and yarn test passes.
• I have detailed the new feature in the relevant documentation.
• I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• If this is a breaking change I've explained why.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	jest@​26.6.3 ⏵ 30.2.0			+2
	@​tsconfig/​node20@​20.1.9
	@​types/​pg@​7.14.11 ⏵ 8.16.0	+1		+1
	@​types/​jest@​26.0.22 ⏵ 30.0.0	+1		+1
	jest-serializer-graphql-schema@​5.0.0-1.1 ⏵ 5.0.0-rc.4	+3		+1	+15
	@​types/​debug@​4.1.5 ⏵ 4.1.12			+22
	debug@​4.3.1 ⏵ 4.4.3	+1		+1
	postgraphile@​4.11.0 ⏵ 5.0.0-rc.7	+9		-16	+1
	@​dataplan/​pg@​1.0.0-rc.5
	tslib@​1.14.1 ⏵ 2.8.1
	ts-jest@​26.5.5 ⏵ 29.4.6			+1	-1
	typescript@​3.9.9 ⏵ 5.9.3	+1		-10		+10
	prettier@​1.18.2 ⏵ 3.8.1	-1		-1
	pg@​8.6.0 ⏵ 8.18.0			-1

View full report

[socket-security]

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.

Action	Severity	Alert  (click "▶" to expand/collapse)
Warn		Obfuscated code: npm entities is 91.0% likely obfuscated Confidence: 0.91 Location: Package overview From: ? → npm/postgraphile@5.0.0-rc.7 → npm/entities@4.5.0 ℹ Read more on: This package | This alert | What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/entities@4.5.0. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Obfuscated code: npm markdown-it is 91.0% likely obfuscated Confidence: 0.91 Location: Package overview From: ? → npm/postgraphile@5.0.0-rc.7 → npm/markdown-it@14.1.1 ℹ Read more on: This package | This alert | What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/markdown-it@14.1.1. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

[benjie]

This is going to make the field scope quite noisy, might be better to wrap these all up in a single attribute instead?

[dargmuesli]

I refactored the types overall, please check again and let me know what you think

[benjie]

Ideally you want to provide more details to an inflector, so it has more choice as to what to call it. Passing just the type name is very limiting in choice, you can't for example attach smart tags or use the database schema from which it originates to make naming choices.

[dargmuesli]

I did some refactoring on this, let me know if that went into a good direction

[benjie]

I'm as yet undecided on the best way to handle type modifiers in V5.

[benjie]

A "postversion" script could be used to update the version to be used in each of the plugins; this would be helpful for consumers of the preset. Something like a scripts/postversion.mjs file:

#!/usr/bin/env node
import * as fs from "node:fs/promises";

const VERSION_TS = "src/vesion.ts";

const packageJsonString = await fs.readFile("package.json", "utf8");
const packageJson = JSON.parse(packageJsonString);

await fs.writeFile(
  VERSION_TS,
  `\
// This file is autogenerated by /scripts/postversion.mjs
export const version = ${JSON.stringify(packageJson.version)};
`,
);

and then add it to package.json:

npm pkg set scripts.postversion="node scripts/postversion.mjs"

[dargmuesli]

I'm currently proposing to just include the version via an package (JSON) import. That way the version should always match the package version, at least after a release, I think. What would the benefit of the script be in contrast? 🙌

[benjie]

The advantage of the script version is that it means you don't need to import JSON, which makes the plugin compatible with a wider array of runtimes and tooling. Importing JSON has complexities in certain bundlers/etc and consumes more memory.

[dargmuesli]

I see, thanks for sharing! I made the suggested change.

[benjie]

import { PostgisPreset } from "@graphile/postgis";

const preset: GraphileConfig.Preset = {
  extends: [
    // ... Amber/V4/etc presets
    PostgisPreset,
  ],
  // ...
};

export default preset;

[dargmuesli]

Done 👍

[benjie]

Shouldn't need this?

[dargmuesli]

For importing the version from package.json it should be needed, I'd say. Let's discuss above.

[dargmuesli]

Removed

[benjie]

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!

[dargmuesli]

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.

[dargmuesli]

@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!

[TBA-Lucas]

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?