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

feat: return null if no defaultValue is specified #14682

Closed
wants to merge 2 commits into from

Conversation

WikiRik
Copy link
Member

@WikiRik WikiRik commented Jun 25, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This PR adds a defaultValue of null if there is no defaultValue specified when creating a model and the attribute allows null.
This fixes #14671.

@ephys I'm not so sure about this change. As you can see the unit tests fail because it also adds DEFAULT NULL to the SQL query. Is that wanted behaviour? This might even qualify as a breaking change

Todos

  • See what to do about the failing tests

src/sequelize.js Outdated
@@ -1271,6 +1271,10 @@ Use Sequelize#query if you wish to use replacements.`);
}
}

if (attribute.allowNull !== false && !attribute.defaultValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultValue could be 0, or '', a better check would be:

Suggested change
if (attribute.allowNull !== false && !attribute.defaultValue) {
if (attribute.allowNull !== false && attribute.defaultValue === undefined) {

It's also a good opportunity to normalize attribute.allowNull just before this so the test can be reduced to

Suggested change
if (attribute.allowNull !== false && !attribute.defaultValue) {
if (attribute.allowNull && attribute.defaultValue === undefined) {

@ephys
Copy link
Member

ephys commented Jun 25, 2022

As you can see the unit tests fail because it also adds DEFAULT NULL to the SQL query. Is that wanted behaviour? This might even qualify as a breaking change

The only element I was worried about was https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html, but after checking DEFAULT NULL is the default behavior for nullable columns there too

I think this is a safe change (but the tests need to be updated)

@WikiRik
Copy link
Member Author

WikiRik commented Jun 26, 2022

@ephys looking at the failed integration tests, I think it would be better to do this change after #14260 so that the impact of this on the tests is smaller. Do you agree?

@ephys
Copy link
Member

ephys commented Jun 27, 2022

Do we want to do #14260 in v7? I was thinking of postponing it to v8 to reduce how many impactful breaking changes we have, and maybe add the flag to change the default nullability in v7?

@ephys
Copy link
Member

ephys commented Jul 9, 2022

I think this PR can be undrafted once #14715 is complete, that other PR should implement the missing pieces

@WikiRik
Copy link
Member Author

WikiRik commented Apr 12, 2024

We still have an open issue connected to this, I'll close this for now and we can revisit with that issue later

@WikiRik WikiRik closed this Apr 12, 2024
@WikiRik WikiRik deleted the WikiRik/null-defaultValue branch April 12, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model.create() in MySQL does not return null value when defaultValue hasn't defined
2 participants