Skip to content

feat: add constraintName to JoinColumn #8524

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.

Sign up for GitHub

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

Jump to bottom
Closed
M-TGH wants to merge 4 commits into from
Closed

feat: add constraintName to JoinColumn #8524

M-TGH wants to merge 4 commits into from

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

@pleerock pleerock requested a review from AlexMesser January 15, 2022 14:26
@M-TGH
feat: add constraintName to JoinColumn
6f623c2 
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 •
edited
Loading

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.

@M-TGH
test: update snapshot in 5444 test
ab8f0f3 
Add constraintName property with correct variable undefined to snapshot in tests for issue 5444.
@pleerock
Copy link
Member

pleerock commented Jan 31, 2022

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)👀

@pleerock pleerock requested a review from AlexMesser January 31, 2022 13:16
@AlexMesser
Copy link
Collaborator

AlexMesser commented Feb 15, 2022 •
edited
Loading

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

joaomarcos85 commented Mar 29, 2022

How is the evolution of this item?

@AlexMesser
Copy link
Collaborator

AlexMesser commented Mar 29, 2022

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

@AlexMesser AlexMesser mentioned this pull request Apr 29, 2022
Merged
7 tasks
@AlexMesser
Copy link
Collaborator

AlexMesser commented Apr 29, 2022

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

Reviewers

@AlexMesser AlexMesser Awaiting requested review from AlexMesser

Assignees

@AlexMesser AlexMesser

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@M-TGH @pleerock @AlexMesser @joaomarcos85