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

fix: reject nullable primary columns #7001

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

carlpaten
Copy link
Contributor

@carlpaten carlpaten commented Nov 2, 2020

Every supported back-end requires that primary key columns be non-nullable.
As of this PR, this mistake is caught at compile-time rather than as part of execution.

BREAKING CHANGE: passing ColumnOptions to @PrimaryKeyColumn does not function anymore. One must use PrimaryKeyColumnOptions instead.

Description of change

Forbid primary columns from being marked as nullable. This condition is expected by all currently-supported back-ends and is generally understood to be standard.

As of the time this PR was opened, some of the tests aren't passing on my machine, and I've not touched documentation. I'd like to validate that this change is desired and likely to be merged before spending more time on it.

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 (N/A)
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

Every supported back-end requires that primary key columns be non-nullable.
As of this PR, this mistake is caught at compile-time rather than as part of execution.

BREAKING CHANGE: passing `ColumnOptions` to `@PrimaryColumn` does not function anymore. One must use `PrimaryColumnOptions` instead.
@pleerock
Copy link
Member

pleerock commented Feb 8, 2021

Looks correct. I don't think that much of a breaking change since there should be only few people (if not less) who could do it.

@pleerock pleerock merged commit cdace6e into typeorm:master Feb 8, 2021
@pleerock
Copy link
Member

pleerock commented Feb 8, 2021

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

2 participants