Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(db-postgres): add point field support #9078

Merged
merged 20 commits into from
Nov 11, 2024
Merged

Conversation

r1tsuu
Copy link
Member

@r1tsuu r1tsuu commented Nov 8, 2024

What?

Adds full support for the point field to Postgres and Vercel Postgres adapters through the Postgis extension. Fully the same API as with MongoDB, including support for near, within and intersects operators.

Additionally, exposes to adapter args:

Why?

It's essential to support that field type, especially if the postgres adapter should be out of beta on 3.0 stable.

How?

  • Bumps drizzle-orm to 0.36.1 and drizzle-kit to 0.28.0 as we need this change feat: add tablesFilter to pushSchema api drizzle-team/drizzle-orm#3141
  • Uses its functions to achieve querying functionality, for example the near operator works through ST_DWithin or intersects through ST_Intersects.
  • Removes MongoDB condition from all point field tests, but keeps for SQLite

Resolves these discussions:
#8996
#8644

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see all the work here. I didn't pull this down, int test coverage is likely all we need.

docs/database/overview.mdx Show resolved Hide resolved
"console-table-printer": "2.12.1",
"drizzle-kit": "0.26.2",
"drizzle-orm": "0.35.1",
"console-table-printer": "2.11.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unexpected. Was there a problem?

Copy link
Member Author

@r1tsuu r1tsuu Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflict mistake ✅

@@ -100,6 +100,11 @@ export const connect: Connect = async function connect(
process.exit(1)
}

if (this.extensionsFilter.has('postgis') && !this.postgisCreated) {
await this.drizzle.execute(`CREATE EXTENSION IF NOT EXISTS postgis`)
this.postgisCreated = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an extensions object for future things like pg_search so that the root of the adapter doesn't get messy. For now we'll just have this.extensions.postgis = true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ + made it a bit more universal with createExtensions

@@ -97,6 +98,7 @@ export function postgresAdapter(args: Args): DatabaseAdapterObj<PostgresAdapter>
pgSchema: adapterSchema,
pool: undefined,
poolOptions: args.pool,
postgisCreated: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use extensions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -29,6 +29,7 @@ export type Args = {
* @default false
*/
disableCreateDatabase?: boolean
extensionsFilter?: string[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think, is this redundant if we have extensionsFilter? Ideally somebody could enable extensions that Payload doesn't know or care about but still passes this to drizzle. 🤔

Copy link
Member Author

@r1tsuu r1tsuu Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually drizzle's extensionsFilter supports only postgis https://github.com/drizzle-team/drizzle-orm/blob/83daf2d5cf023112de878bc2249ee2c41a2a5b1b/drizzle-kit/src/cli/validations/cli.ts#L26 so it's kinda useless property. I updated how we handle extensions and now you can pass for example:
extensions: ['vector'] and it'll actually create the vector extension on connect / when running migrations. I think it's useful

Comment on lines 10 to 19
customType<{ data: Point; driverData: string }>({
dataType() {
return `geometry(Point,4326)`
},
fromDriver(value: string) {
return parseEWKB(value)
},
toDriver(value: Point) {
return `SRID=4326;point(${value[0]} ${value[1]})`
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have much to learn. Does this support DEFAULT? Any other gotchas going this low level with a custom data type for postgres?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we ever need to get away from this, it seems like it'd be a breaking change/require migration will be needed. We might want to get a status update on the underlying issues you found from Drizzle before we roll this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT - yes, but actually even in MongoDB we had a bug with it, so now we support both

The geometry export from drizzle-or/pg-core has fully the same implementation regardless geometry(Point) (uppercase vs lowecase https://github.com/drizzle-team/drizzle-orm/blob/83daf2d5cf023112de878bc2249ee2c41a2a5b1b/drizzle-orm/src/pg-core/columns/postgis_extension/geometry.ts#L38).
I made a PR to Drizzle which will solve our issue drizzle-team/drizzle-orm#3517, but we can use this fine as well, migration won't be needed as the type is the same.

packages/drizzle/src/queries/parseParams.ts Show resolved Hide resolved
@r1tsuu r1tsuu requested a review from jmikrut as a code owner November 11, 2024 03:21
@DanRibbens DanRibbens merged commit 0a15388 into beta Nov 11, 2024
51 checks passed
@DanRibbens DanRibbens deleted the feat/point-field-postgres branch November 11, 2024 14:31
Copy link

🚀 This is included in version v3.0.0-beta.127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants