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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add constraintName to JoinColumn #8524

Closed

Conversation

M-TGH
Copy link
Contributor

@M-TGH M-TGH commented Jan 12, 2022

Description of change

Change to add a constraintName to JoinColumn data.
Also adding givenName to ForeignKey types.
Passing it on through JoinColumns and JoinTables.

Personally at the current looking at picking up an existing database with TypeORM and keeping as much unchanged as possible. This is one of a couple of things I was missing for that. Whilst possible through naming strategy's, that didn't at current feel like as nice of a solution yet, especially for as big a database as I'm working with.

This implements some of the changes suggested in #1355. Hence I decided, whilst unsure about the decision, to add the tests under a folder with that issue number as well. With this in mind, I'm not fully linking it since not the whole issue will be fixed.

I tried my hand at adding some documentation for it, let me know if more is needed. And I lack the skills to update the Chinese documentation, I assume that can be forgiven? 馃槄

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000 N\A
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

Add a constraintName to JoinColumn decorators to allow specifying foreignKey name.
Use constraintName when building JoinTable entities as well.

Partially solves: typeorm#1355
@M-TGH M-TGH force-pushed the feat/join-column-constraint-name branch from 14727d1 to fece7e3 Compare January 17, 2022 13:32
@M-TGH M-TGH force-pushed the feat/join-column-constraint-name branch from fece7e3 to 81d0db7 Compare January 17, 2022 13:38
@M-TGH
Copy link
Contributor Author

M-TGH commented Jan 17, 2022

Rebased to add new changes from master. Also noticed I still had .only in the tests (oops 馃槄 ), fixed that. 馃敡
EDIT: Currently still failing test looks to be failing on master as well.

Add constraintName property with correct variable undefined to snapshot in tests for issue 5444.
@pleerock
Copy link
Member

I'm not sure how this is going to affect schema sync, and I don't believe it's as easy as it seems. We need to ask @AlexMesser to review this one.

@pleerock pleerock removed the request for review from AlexMesser January 31, 2022 12:11
@M-TGH
Copy link
Contributor Author

M-TGH commented Jan 31, 2022

@pleerock

I'm not sure how this is going to affect schema sync, and I don't believe it's as easy as it seems. We need to ask @AlexMesser to review this one.

Sounds good. Let me know if there's any extra changes needed, might need some pointers for them, but I've got the time to make them.
You removed the request for their review btw (since you had requested it earlier I think)馃憖

@AlexMesser
Copy link
Collaborator

AlexMesser commented Feb 15, 2022

Here we change FK names on table renaming and custom names will be replaced.

Btw, we have custom constraint name support for indices and uniques, but seems it is not working on table renaming 馃

I will join to this PR to fix this problem and to add support for custom PK names as well.

@AlexMesser AlexMesser self-assigned this Feb 15, 2022
@M-TGH
Copy link
Contributor Author

M-TGH commented Feb 16, 2022

I will join to this PR to fix this problem and to add support for custom PK names as well.

Sounds good! Let me know if there's anything I can help with, I've got time but I'm not experienced with the project so I'm not sure there's much of relevance that I can pick up.

@joaomarcos85
Copy link

How is the evolution of this item?

@AlexMesser
Copy link
Collaborator

I'll return to this after #8730 and #8806

@AlexMesser
Copy link
Collaborator

Superseded by #8900. Thank you for contribution!

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

Successfully merging this pull request may close these issues.

None yet

4 participants