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: skip validation for undefined values while updating #9587

Merged
merged 9 commits into from Jun 24, 2018

Conversation

@jony89
Copy link
Contributor

@jony89 jony89 commented Jun 23, 2018

Pull Request check-list

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

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

Description of change

Fixes #7056, after #7167 has failed since it affects the .create method (and should have not).

As stated in #7056, while assigning undefined value into a non-nullable prop and calling .update a validation Error is raised, yet while assigning undefined to nullable prop is completely ignored and not shown in the update query, due to the mapValueFieldNames function which removes undefined props as desired.
This also aligning the behavior with #9548 .

BWC checks for non-nullable props :

  • assigning null : no implication
  • assigning value : no implicaation
  • assigning undefined : no implication in final query yet no exception is raised now.
@codecov
Copy link

@codecov codecov bot commented Jun 23, 2018

Codecov Report

Merging #9587 into master will increase coverage by <.01%.
The diff coverage is 100%.

jony89 added 2 commits Jun 23, 2018
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Why don't we just filter out undefined from values input? Please add a test to verify instance.update also ignores any undefined values

@jony89
Copy link
Contributor Author

@jony89 jony89 commented Jun 24, 2018

Why don't we just filter out undefined from values input?

@sushantdhiman since there is valuesUse and values I guessed they might be used for something else.

I can do even better and move valuesUse to upper scope and remove entirely the values, but again I think it was done for a reason. what do u think ?

@jony89
Copy link
Contributor Author

@jony89 jony89 commented Jun 24, 2018

@sushantdhiman yeah that what I thought at start, but there is no need for that because that's exactly what mapValueFieldNames does later (along with virtual attribtues). so we can just do it sooner ?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jun 24, 2018

mapValueFieldNames will convert attributes to field name which may fail in next segment as it is expected that fields to contain attributes (bad name :D)

So safest option is to filter out undefined at the top, probably use omitBy which will filter for undefined keys and create new object in one go, removing need for clone

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Need some work in tests and its good to go

@@ -35,6 +35,53 @@ describe(Support.getTestDialectTeaser('Model'), () => {
}));
});

it('should not check for notNull Violation for undefined values', function() {
const ownerId = 2;
let accountRowId;

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 24, 2018
Contributor

Get rid of accountRowId, if you do account.findOne you will directly get our target instance, as there is sync in before each.

name: Math.random().toString()
}).then(account => {
accountRowId = account.get('id');
const accountVal = {

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 24, 2018
Contributor

values from accountVal can be passed directly to this.Account.update, no need to keep values as constant.

});
});

it('should ignore undefined values', function() {

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 24, 2018
Contributor

Looks like duplicate of test above, Please add a test here https://github.com/sequelize/sequelize/blob/master/test/integration/instance/update.test.js so we can be sure instance method update also ignores undefined values

This comment has been minimized.

@jony89

jony89 Jun 24, 2018
Author Contributor

It was one for not null props and one for the nullable props (as it might behave different) but I guess the first one is enough

@@ -35,6 +35,53 @@ describe(Support.getTestDialectTeaser('Model'), () => {
}));
});

it('should not check for notNull Violation for undefined values', function() {

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 24, 2018
Contributor

should ignore undefined values without throwing not null validation

@jony89
Copy link
Contributor Author

@jony89 jony89 commented Jun 24, 2018

Please add a test here https://github.com/sequelize/sequelize/blob/master/test/integration/instance/update.test.js so we can be sure instance method update also ignores undefined values

So I had to edit the .update non-static method as well. otherwise the test in test/integration/instance/update.test.js would fail, the value in the DB was not updated with undefined but the instance in the RAM did. I'm not sure about this case.

See if it's fine by you. and whether u like to refactor that .omitBy line.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Thanks

@sushantdhiman sushantdhiman merged commit 7a49a65 into sequelize:master Jun 24, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 95.96%)
Details
codecov/project 95.96% (+<.01%) compared to abf4c73
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@peskic93
Copy link

@peskic93 peskic93 commented Jun 18, 2019

@sushantdhiman Can this be merged in v4?

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

Successfully merging this pull request may close these issues.

3 participants