payload icon indicating copy to clipboard operation
payload copied to clipboard

feat(db-postgres): allow for custom table names

Open r1tsuu opened this issue 2 years ago • 7 comments

Description

Discussion #3593

Added: tableName property to collection / global / array field / block type / select with hasMany: true config. enumName property to select field / radio field config.

Example: Given collection:

{
    slug: 'collection',
    tableName: 'c',
    fields: [
      {
        name: 'blocks',
        type: 'blocks',
        blocks: [
          {
            slug: 'variant-first',
            tableName: 'vf',
            fields: [
              {
                name: 'array',
                tableName: 'a',
                type: 'array',
                fields: [
                  {
                    type: 'select',
                    name: 'select',
                    enumName: 's',
                    options: ['A', 'B'],
                  },
                ],
              },
            ],
          },
          {
            slug: 'variant-second',
            tableName: 'vs',
            fields: [
              {
                type: 'select',
                hasMany: true,
                enumName: 's',
                name: 'select',
                tableName: 's',
                options: ['B', 'C'],
              },
            ],
          },
        ],
      },
    ],
 },

Tables:

  • c - from collection
  • c_blocks_vf - from collection_blocks_variant_first
  • c_blocks_vf_a - from collection_blocks_variant_first_array
  • c_blocks_vs - from collection_blocks_variant_second
  • c_blocks_vs_s - from collection_blocks_variant_second_select

Enums:

  • enum_c_blocks_vf_a_s - from enum_collection_blocks_variant_first_array_select
  • enum_c_blocks_vs_s - from enum_collection_blocks_variant_second_select

Tests: if i need them, would like to get a help how to structure them. Currently i just temporary randomly added tableName and enumName to relationships / localization config tests and it passed (i deleted them after all)

  • [x] I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • [x] New feature (non-breaking change which adds functionality)
  • [x] This change requires a documentation update

Checklist:

  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [x] Existing test suite passes locally with my changes
  • [x] I have made corresponding changes to the documentation

r1tsuu avatar Dec 16 '23 03:12 r1tsuu

This one check https://github.com/payloadcms/payload/actions/runs/7234869919/job/19711676350 on commit fix: add tableName property for select with hasMany: true is very weird and i don't think it could be related to my PR. Didn't change anything that could appear that in this commit, and on commit chore: getTableName move prefer to options it was gone. Are there some side effects?

r1tsuu avatar Dec 16 '23 23:12 r1tsuu

This looks great!

This one check payloadcms/payload/actions/runs/7234869919/job/19711676350 on commit fix: add tableName property for select with hasMany: true is very weird and i don't think it could be related to my PR.

We've been having test flakyness for postgres in CI since it was added that has been alluding us. I wouldn't worry about it for your PR. As long as it passes locally or re-run on github a few times if needed.

I'll be reviewing it this week and testing it out. First, I want to give you some quick initial thoughts I have though.

  • The way you have added tableName property needs some consideration:
    • This is only for Postgres and (soon) sqlite, yet you have added it to the Payload config and we should either support it on MongoDB or use declare module in the adapter to extend the Payload config only for projects with postgres.
    • Alternatively, we could use a generic name or accept that it isn't a perfect fit for mongodb and use it as-is for any db adapter.
    • On block fields, MongoDB will never need the property.
  • enumName could also be omitted for MongoDB.

DanRibbens avatar Dec 17 '23 20:12 DanRibbens

This looks great!

This one check payloadcms/payload/actions/runs/7234869919/job/19711676350 on commit fix: add tableName property for select with hasMany: true is very weird and i don't think it could be related to my PR.

We've been having test flakyness for postgres in CI since it was added that has been alluding us. I wouldn't worry about it for your PR. As long as it passes locally or re-run on github a few times if needed.

I'll be reviewing it this week and testing it out. First, I want to give you some quick initial thoughts I have though.

  • The way you have added tableName property needs some consideration:

    • This is only for Postgres and (soon) sqlite, yet you have added it to the Payload config and we should either support it on MongoDB or use declare module in the adapter to extend the Payload config only for projects with postgres.
    • Alternatively, we could use a generic name or accept that it isn't a perfect fit for mongodb and use it as-is for any db adapter.
    • On block fields, MongoDB will never need the property.
  • enumName could also be omitted for MongoDB.

Thanks for reply, i've just updated, didn't do anything for MongoDB for now, but declared types for CollectionConfig, GlobalConfig, ArrayField, RadioField, Block, SelectField in db-postgres/src/types. The way i did this is the same as with DatabaseAdapter that extends BaseDatabaseAdapter. I used interfaces instead of types in declaration because with types seems like it's not overriding payload/types package, maybe it's something with my environment.

Should be also easy to do with mongo adapter in the same way, as it's only about to adding collectionName for CollectionConfig if we need this. The problem with SQL is that it's not your preference in naming, it's about 63 characters limit, which is easy to exceed with nesting and enums.

The only thing that still in payload package for this feature is joi schema validation.

I think this way with separate declaration is better than generic one, because not only MongoDB, but even for SQLite could be few differences, because there aren't enums.

r1tsuu avatar Dec 17 '23 22:12 r1tsuu

In talking with @jmikrut, we want to steer away from having different PayloadConfig properties for different adapters. Instead of tableName this can be dbName, however I'm unsure what we would should do about blocks which have tables in relationships but not MongoDB. The same question comes up for enumName which doesn't apply to MongoDB.

I've added tests which should apply either way with a little config adjustment wherever we land on this.

DanRibbens avatar Dec 21 '23 16:12 DanRibbens

Hey @r1tsuu - yes, @DanRibbens and I are talking now and have some direction for you.

First up, YOU ARE A BADASS - we have wanted to do this but then you came in and did it. Nice.

Let's have the new properties be called dbName and enumName - just put them right into Payload Core, no declare pattern necessary. In MongoDB, as Dan mentioned, dbName on blocks / arrays / etc. won't do anything but I don't care. That's fine.

Our goal with this feedback is to make the DX as simple as possible and I want to retain declare for cases where we absolutely, 100% need it. Here seems like it isn't necessary.

Can you make the changes necessary? Then we'll happily merge!

jmikrut avatar Dec 21 '23 20:12 jmikrut

Hey @r1tsuu - yes, @DanRibbens and I are talking now and have some direction for you.

First up, YOU ARE A BADASS - we have wanted to do this but then you came in and did it. Nice.

Let's have the new properties be called dbName and enumName - just put them right into Payload Core, no declare pattern necessary. In MongoDB, as Dan mentioned, dbName on blocks / arrays / etc. won't do anything but I don't care. That's fine.

Our goal with this feedback is to make the DX as simple as possible and I want to retain declare for cases where we absolutely, 100% need it. Here seems like it isn't necessary.

Can you make the changes necessary? Then we'll happily merge!

Of course, i will do it! Should i also add dbName support for MongoDB in this?

r1tsuu avatar Dec 21 '23 22:12 r1tsuu

Of course, i will do it! Should i also add dbName support for MongoDB in this?

If you can, that would be great! It should obviously be much simpler than Postgres. If you can't get to it, we'd probably add it to this PR before we merge just to make sure we have parity and not an unexpected lack of support for MongoDB.

We will keep an eye out and act accordingly!

jmikrut avatar Dec 21 '23 23:12 jmikrut

Hello, sorry for late, i can do it this weekend, wouldn't you mind to wait? Maybe earlier, i'll see

r1tsuu avatar Jan 11 '24 15:01 r1tsuu

Hello, sorry for late, i can do it this weekend, wouldn't you mind to wait? Maybe earlier, i'll see

I don't mean to call you out, whoever gets to it first. I'm just marking what work is ready or not out of the 80+ open PRs 😭

DanRibbens avatar Jan 11 '24 16:01 DanRibbens

I'm working on making this change happen. This is one of the things we need to finally get postgres out of beta which is a prerequisite for finishing SQLite.

DanRibbens avatar Jan 24 '24 15:01 DanRibbens

I'm working on making this change happen. This is one of the things we need to finally get postgres out of beta which is a prerequisite for finishing SQLite.

Okay, thank you, sorry, i can't find the time for that now

r1tsuu avatar Jan 25 '24 01:01 r1tsuu

Okay, thank you, sorry, i can't find the time for that now

It's all good. I'm making the change a little more involved after reviewing this again.

There are two use cases for needing to customize the table names and the original solution only partly solves the first of these:

  1. Condense names to fit below the max character limit of postgres names
  2. To map configurations to existing databases.

My approach for the changes I'm making is to allow the dbName to be the exact string to use instead of concatenating the prefix of the root table. This way you get full control.

Alternatively to this I'm letting you call a function that receives the root table name so that your config or plugins can do whatever they need for naming while respecting uniqueness of table names.

This level of configuration should be enough to fully support the two issues and also it would allow for more dynamic name shrinking strategies too.

DanRibbens avatar Jan 25 '24 01:01 DanRibbens

@r1tsuu, This PR is ready for review if you are interested in giving your feedback. I took it a lot further from where you started.

DanRibbens avatar Feb 08 '24 21:02 DanRibbens

@r1tsuu, This PR is ready for review if you are interested in giving your feedback. I took it a lot further from where you started.

Yeah, great work, i started with a simple idea to only fix cases where the length of the table name is exceeded because i didn't want to change structure too much, it's good that you can customize like that now

r1tsuu avatar Feb 09 '24 17:02 r1tsuu

Looking forward to having this feature! Auto-generated table names are the only thing that stops us from switching to Postgres

grundiss avatar Feb 16 '24 06:02 grundiss

@DanRibbens , any chance of having this merged/published this month?

grundiss avatar Mar 12 '24 13:03 grundiss

Suggestion to also expose a tableNamePrefix (or dbNamePrefix) in the adapter config which will allow for a multi project use case.

Normally with fully exposed drizzle you'd be able to do something like this:

export const createTable = pgTableCreator((name) => `projectX_${name}`);

Giving you the ability to host multiple project in the same db schema (also used for multi-tenant)

As it is, the dbName at the collection level won't cut it since it won't affect the metadata tables that payload automatically creates for you. e.g. image

MendyLanda avatar Apr 02 '24 12:04 MendyLanda

@grundiss, we've been so busy on the v3 work it got put on the back burner. I finally got this merged today and it will be in the next release to v2 nd the v3 alpha.

@MendyLanda, this is a great feature request: https://github.com/payloadcms/payload/pull/4525#issuecomment-2031923450

You should make a PR, it would be pretty straightforward given the changes we made to the postgres adapter with this change.

I'm closing this now that everything has been merged and soon to be released.

Thanks everyone for the help and discussion!

DanRibbens avatar Apr 05 '24 20:04 DanRibbens