-
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
Conversation
1ed2972
to
60d0599
Compare
@@ -206,7 +206,7 @@ describe('datatypes', function() { | |||
|
|||
it('should set missing optional properties to null', function(done) { | |||
var EXPECTED = {desc: null, stars: null}; | |||
TestModel.create({name: 'a-test-name'}, function(err, created) { | |||
TestModel.create({}, function(err, created) { |
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.
i believe the name has nothing to do with this test case, can someone confirm @Amir-61 @bajtos
and under strict mode it will return validationError, or perhaps we can define it on top https://github.com/strongloop/loopback-datasource-juggler/blob/60d059928d2ef919135efa7bf5425e10a5c83be0/test/datatype.test.js#L192-L200
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.
This test checks what happens when you have persistUndefinedAsNull
configured and create a model instance that does not have values for all properties.
Your conclusion seems reasonable to me. I guess my only concern is whether it's ok to pass an empty object to create
. Perhaps we should add the name
property to the model definition instead?
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.
agreed, having empty object might cause confusion.
Perhaps we should add the name property to the model definition instead?
11fa30a
to
edfde85
Compare
if (isStrict) { | ||
// SQL-based connectors don't support dynamic properties | ||
delete EXPECTED.extra; | ||
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.
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 to
1) mysql imported features datatypes model option persistUndefinedAsNull should convert property value undefined to null:
ValidationError: The `TestModel` instance is not valid. Details: `extra` is not defined in the model (value: undefined); `haha` is not defined in the model (value: undefined).
at /Users/davidcheung/node-web/loopback-datasource-juggler/lib/dao.js:327:12
at ModelConstructor.<anonymous> (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/validations.js:495:11)
at ModelConstructor.next (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/hooks.js:83:12)
at ModelConstructor.<anonymous> (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/validations.js:492:23)
at ModelConstructor.trigger (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/hooks.js:73:12)
at ModelConstructor.Validatable.isValid (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/validations.js:458:8)
at /Users/davidcheung/node-web/loopback-datasource-juggler/lib/dao.js:323:9
at doNotify (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:99:49)
at doNotify (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:99:49)
at Function.ObserverMixin._notifyBaseObservers (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:122:5)
at Function.ObserverMixin.notifyObserversOf (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:97:8)
at Function.ObserverMixin._notifyBaseObservers (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:120:15)
at Function.ObserverMixin.notifyObserversOf (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/observer.js:97:8)
at Function.DataAccessObject.create (/Users/davidcheung/node-web/loopback-datasource-juggler/lib/dao.js:307:9)
at Context.<anonymous> (/Users/davidcheung/node-web/loopback-datasource-juggler/test/datatype.test.js:229:17)
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.
I'm not sure what to do with this test case, so the option of { persistUndefinedAsNull: true }
disables the discarding of undefined property, while strict 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 test
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.
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 returns err
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.
I did a quick partial review, didn't reviewed the added/removed test cases. |
39da2dc
to
98ac9a5
Compare
@bajtos PTAL |
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.
I left few minor comments for the changes in tests, the implementation looks mostly good to me.
Please clean up the history while addressing the comments, I think the patch will be ok to land after the next iteration.
@@ -192,6 +192,7 @@ describe('datatypes', function() { | |||
TestModel = db.define( | |||
'TestModel', | |||
{ | |||
name: {type: String, require: false}, |
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.
type, require
instead of required
?
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 |
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.
Please preserve this comment.
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.
moved it under it('should return error on unknown attributes when strict: true',
it('should throw error on unknown attributes when strict: throw', function(done) { | ||
// `strict: true` used to silently remove unknown properties, | ||
// now return validationError upon unknown properties, | ||
// same as previous validate behavior |
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.
This comment does not make sense to me when it's before it should allow unknown attributes when strict: false
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.
ure right, going to remove it
going to leave the test below with (which i think is alittle easier to understand)
// `strict: true` used to silently remove unknown properties,
// now return validationError upon unknown properties
it('should return error on unknown attributes when strict: true',
749f973
to
2ec1c2c
Compare
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.
LGTM.
I pointed out few details that can be improved, feel free to ignore them.
should.not.exist(p.foo); | ||
Person.findById(p.id, function(e, p) { | ||
if (e) return done(e); | ||
should.not.exist(p.foo); |
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.
p.should.not.have.property('foo')
?
should.exist(err); | ||
err.name.should.equal('ValidationError'); | ||
err.message.should.containEql('`foo` is not defined in the model'); | ||
should.not.exist(p.foo); |
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.
p.should.not.have.property('foo')
?
function(err, p) { | ||
if (err) return done(err); | ||
should.not.exist(err); | ||
should.exist(p.foo); |
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.
p.should.have.property('foo')
?
p.updateAttributes({name: 'John', foo: 'bar'}, | ||
function(err, p) { | ||
if (err) return done(err); | ||
should.not.exist(err); |
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.
this is redundant, if (err)
has already handled this case.
Person.findById(p.id, function(e, p) { | ||
if (e) return done(e); | ||
p.name.should.equal('John'); | ||
should.not.exist(p.unknownVar); |
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.
p.should.not.have.property('unknownVar')
?
@@ -95,6 +95,11 @@ ModelBaseClass.prototype._initProperties = function(data, options) { | |||
if (strict === undefined) { | |||
strict = ctor.definition.settings.strict; | |||
} | |||
if (strict === 'throw') { |
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.
Perhaps join with the line above?
} else if (string === 'throw') {
@slnode test please |
47d6f73
to
805db78
Compare
- Deprecation of strict:validate and strict:throw - When strict mode is enabled, it will now always return validation error (previous strict:validate)
@slnode test please |
@slnode test please |
connect to strongloop/loopback/issues/1299