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

Unable to provide name for PK through naming strategy #2762

Closed
oliversalzburg opened this issue Feb 15, 2022 · 3 comments
Closed

Unable to provide name for PK through naming strategy #2762

oliversalzburg opened this issue Feb 15, 2022 · 3 comments

Comments

@oliversalzburg
Copy link
Contributor

Describe the bug

It is not actually possible to control the name of primary keys through a naming strategy, even in database drivers where it should be supported (postgres).

To Reproduce
Steps to reproduce the behavior:

  1. Provide alternative naming for the "primary" index type in the naming strategy.

Expected behavior

Names of primary keys can be controlled when this should be supported by the database driver.

Additional context

The current naming behavior seems to be hardcoded at:

const keyName = tableName.includes('.') ? tableName.split('.').pop()! + '_pkey' : undefined;

I wasn't able to understand where the naming strategy would be referenced here.

Versions

Dependency Version
node 16
typescript 4.4
mikro-orm 5.0.1
postgresql 5.0.1
@B4nan
Copy link
Member

B4nan commented Feb 15, 2022

That link you mentioned is using undefined unless you have explicit schema name, not really a hardcoded name (not sure if the first branch is even used, seeing the coverage ignore comment).

The reason here is that we don't usually need to name a PK, and doing so would require additional query. PKs are not compared by name when diffing.

So I would rather keep this as is, will check it later today to be sure, but I would rather not have this configurable and save one query for each table. The first branch could use the naming strategy, but as said, I am not sure if it's even valid, might be a dead code.

edit: looking at the schema diffing snapshots, and we probably use separate query for anything that is not autoincrement, so it would be probably ok to use the PK name at least there. not sure how other drivers behave, I remember I was testing this and decided to keep default PK names

@oliversalzburg
Copy link
Contributor Author

Hmm okay, I can see it's more complex than what I thought. I might just rename the PKs in our existing data model then to reach some alignment. I'll give a moment, until your final call.

@B4nan
Copy link
Member

B4nan commented Feb 15, 2022

Looks like the actual reason for this was mysql and sqlite, where the PK key name is forced to be PRIMARY, and I didn't want to have that in the generated SQL. Will try to find a way how to make this work for postgres.

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

No branches or pull requests

2 participants