-
Notifications
You must be signed in to change notification settings - Fork 363
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
[SEMVER-MAJOR] Strict mode cleanup #1084
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -544,41 +544,83 @@ describe('manipulation', function() { | |
}); | ||
}); | ||
|
||
it('should ignore unknown attributes when strict: true', function(done) { | ||
// Using {foo: 'bar'} only causes dependent test failures due to the | ||
// stripping of object properties when in strict mode (ie. {foo: 'bar'} | ||
// changes to '{}' and breaks other tests | ||
person.updateAttributes({name: 'John', foo: 'bar'}, | ||
function(err, p) { | ||
if (err) return done(err); | ||
should.not.exist(p.foo); | ||
Person.findById(p.id, function(e, p) { | ||
if (e) return done(e); | ||
should.not.exist(p.foo); | ||
done(); | ||
}); | ||
it('should discard undefined values before strict validation', | ||
function(done) { | ||
Person.definition.settings.strict = true; | ||
Person.findById(person.id, function(err, p) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please preserve this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved it under |
||
p.updateAttributes({name: 'John', unknownVar: undefined}, | ||
function(err, p) { | ||
// if uknownVar was defined, it would return validationError | ||
if (err) return done(err); | ||
Person.findById(p.id, function(e, p) { | ||
if (e) return done(e); | ||
p.name.should.equal('John'); | ||
p.should.not.have.property('unknownVar'); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
it('should allow unknown attributes when strict: false', | ||
function(done) { | ||
Person.definition.settings.strict = false; | ||
Person.findById(person.id, function(err, p) { | ||
p.updateAttributes({name: 'John', foo: 'bar'}, | ||
function(err, p) { | ||
if (err) return done(err); | ||
p.should.have.property('foo'); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
|
||
it('should throw error on unknown attributes when strict: throw', function(done) { | ||
// Prior to version 3.0 `strict: true` used to silently remove unknown properties, | ||
// now return validationError upon unknown properties | ||
it('should return error on unknown attributes when strict: true', | ||
function(done) { | ||
// Using {foo: 'bar'} only causes dependent test failures due to the | ||
// stripping of object properties when in strict mode (ie. {foo: 'bar'} | ||
// changes to '{}' and breaks other tests | ||
Person.definition.settings.strict = true; | ||
Person.findById(person.id, function(err, p) { | ||
p.updateAttributes({name: 'John', foo: 'bar'}, | ||
function(err, p) { | ||
should.exist(err); | ||
err.name.should.equal('ValidationError'); | ||
err.message.should.containEql('`foo` is not defined in the model'); | ||
p.should.not.have.property('foo'); | ||
Person.findById(p.id, function(e, p) { | ||
if (e) return done(e); | ||
p.should.not.have.property('foo'); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
// strict: throw is deprecated, use strict: true instead | ||
// which returns Validation Error for unknown properties | ||
it('should fallback to strict:true when using strict: throw', function(done) { | ||
Person.definition.settings.strict = 'throw'; | ||
Person.findById(person.id, function(err, p) { | ||
p.updateAttributes({foo: 'bar'}, | ||
function(err, p) { | ||
should.exist(err); | ||
err.name.should.equal('Error'); | ||
err.message.should.equal('Unknown property: foo'); | ||
should.not.exist(p); | ||
err.name.should.equal('ValidationError'); | ||
err.message.should.containEql('`foo` is not defined in the model'); | ||
Person.findById(person.id, function(e, p) { | ||
if (e) return done(e); | ||
should.not.exist(p.foo); | ||
p.should.not.have.property('foo'); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
it('should throw error on unknown attributes when strict: throw', function(done) { | ||
// strict: validate is deprecated, use strict: true instead | ||
// behavior remains the same as before, because validate is now default behavior | ||
it('should fallback to strict:true when using strict:validate', function(done) { | ||
Person.definition.settings.strict = 'validate'; | ||
Person.findById(person.id, function(err, p) { | ||
p.updateAttributes({foo: 'bar'}, | ||
|
@@ -588,7 +630,7 @@ describe('manipulation', function() { | |
err.message.should.containEql('`foo` is not defined in the model'); | ||
Person.findById(person.id, function(e, p) { | ||
if (e) return done(e); | ||
should.not.exist(p.foo); | ||
p.should.not.have.property('foo'); | ||
done(); | ||
}); | ||
}); | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to delete
data.extra
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql
fails with the following otherwise, seems it does not discard undefined properties which leads toThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to do with this test case, so the option of
{ persistUndefinedAsNull: true }
disables the discarding of undefined property, whilestrict mode
will return validation error.in this case deleting
data.extra
is needed ( and i think is correct behavior? )while the test will still check for the defined property (
desc
) which fulfills the original intent of the testThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised the problem was not discovered earlier. Is it really related to the changes made in this pull request? If it isn't, then could you perhaps isolate it in a standalone commit (if not send it as a new PR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is related to this change, because sql database uses
strict:true
which silently remove before, but now returnserr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, of course! Makes a lot of sense.