payloadcms / payload

Payload is the open-source, fullstack Next.js framework, giving you instant backend superpowers. Get a full TypeScript backend and admin panel instantly. Use Payload as a headless CMS or for building powerful applications.
https://payloadcms.com
MIT License
25.76k stars 1.64k forks source link

(db-postgres): Non-standard column names between `_rels` tables and tables created from arrays (i.e. `_parent_id` and `parent_id`) #8035

Closed joshuaja closed 2 months ago

joshuaja commented 2 months ago

Link to reproduction

https://github.com/DapperMountain/payload-3-bun-template

Environment Info

Binaries:
  Node: 22.2.0
  npm: 10.7.0
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  payload: 3.0.0-beta.97
  next: 15.0.0-canary.104
  @payloadcms/db-postgres: 3.0.0-beta.97
  @payloadcms/email-nodemailer: 3.0.0-beta.97
  @payloadcms/graphql: 3.0.0-beta.97
  @payloadcms/next/utilities: 3.0.0-beta.97
  @payloadcms/richtext-lexical: 3.0.0-beta.97
  @payloadcms/translations: 3.0.0-beta.97
  @payloadcms/ui/shared: 3.0.0-beta.97
  react: 19.0.0-rc-e56f4ae3-20240830
  react-dom: 19.0.0-rc-e56f4ae3-20240830
Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6031
  Available memory (MB): 36864
  Available CPU cores: 14

Describe the Bug

_rels tables and tables created from arrays have different column naming syntax for some reason.

Additionally (nit): The id field in the array table (users_tenants) should be positioned as the first column in the table and uses a different UUID format than the other id fields.

Example

users_rels table

users_tenants table

Reproduction Steps

Set up a users collection like this. It should create the tables for us to reproduce the issue.

Adapters and Plugins

db-postgres

BlainMaguire commented 2 months ago

In the postgres adaptor I've seen code like this:

packages/drizzle/src/postgres/schema/traverseFields.ts line 661

          const baseExtraConfig: BaseExtraConfig = {
            orderIdx: (cols) => index(`${selectTableName}_order_idx`).on(cols.order),
            parentFk: (cols) =>
              foreignKey({
                name: `${selectTableName}_parent_fk`,
                columns: [cols.parent],
                foreignColumns: [adapter.tables[parentTableName].id],
              }).onDelete('cascade'),
            parentIdx: (cols) => index(`${selectTableName}_parent_idx`).on(cols.parent),
          }

So I wonder in the case of arrays is what you're seeing just an empty string in that case, hence the leading _.

joshuaja commented 2 months ago

So I wonder in the case of arrays is what you're seeing just an empty string in that case, hence the leading _.

Good find! I was thinking the same thing - it smells of string concatenation.

joshuaja commented 2 months ago

@BlainMaguire - looks like a little above that in the file is where the array column names are specified:

const baseColumns: Record<string, PgColumnBuilder> = {
  _order: integer('_order').notNull(),
  _parentID: parentIDColumnMap[parentIDColType]('_parent_id').notNull(),
}
DanRibbens commented 2 months ago

We used _ anytime there was a chance that the table could have a conflicting column name due to field names from the config being mixed with payload internal column names. This is by design. Suppose in your tenants array you also had a field with name: 'order'.

Does that answer the question?

It would be trivial for us to add to the db adapter configuration additional properties for orderColumnName, parentColumnName, etc. But I don't think it would add a whole lot of value and makes our code slightly more complex for little gain.

I'm interested in your feedback. I'm changing this to a discussion since I don't see any issue here.