[SEMVER-MAJOR] Strict mode cleanup by davidcheung · Pull Request #1084 · loopbackio/loopback-datasource-juggler · GitHub

davidcheung · 2016-09-07T18:59:37Z

connect to strongloop/loopback/issues/1299

davidcheung · 2016-09-07T21:09:23Z

    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) {


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

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?

agreed, having empty object might cause confusion.

Perhaps we should add the name property to the model definition instead?

bajtos · 2016-09-09T12:57:08Z

      if (isStrict) {
        // SQL-based connectors don't support dynamic properties
        delete EXPECTED.extra;
+        delete data.extra;


Why is it necessary to delete data.extra?

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)

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

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)?

it is related to this change, because sql database uses strict:true which silently remove before, but now returns err

Ah, of course! Makes a lot of sense.

bajtos · 2016-09-09T13:01:07Z

I did a quick partial review, didn't reviewed the added/removed test cases.

davidcheung · 2016-09-14T21:02:04Z

@bajtos PTAL

bajtos

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.

bajtos · 2016-09-16T10:58:57Z

      TestModel = db.define(
        'TestModel',
        {
+          name: {type: String, require: false},


type, require instead of required?

bajtos · 2016-09-16T11:02:13Z

-    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


Please preserve this comment.

moved it under it('should return error on unknown attributes when strict: true',

bajtos · 2016-09-16T11:03:31Z

-    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


This comment does not make sense to me when it's before it should allow unknown attributes when strict: false

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',

bajtos

LGTM.

I pointed out few details that can be improved, feel free to ignore them.

bajtos · 2016-09-19T12:18:14Z

+              should.not.exist(p.foo);
+              Person.findById(p.id, function(e, p) {
+                if (e) return done(e);
+                should.not.exist(p.foo);


p.should.not.have.property('foo')?

bajtos · 2016-09-19T12:18:23Z

+              should.exist(err);
+              err.name.should.equal('ValidationError');
+              err.message.should.containEql('`foo` is not defined in the model');
+              should.not.exist(p.foo);


p.should.not.have.property('foo')?

bajtos · 2016-09-19T12:18:37Z

+            function(err, p) {
+              if (err) return done(err);
+              should.not.exist(err);
+              should.exist(p.foo);


p.should.have.property('foo')?

bajtos · 2016-09-19T12:18:58Z

+          p.updateAttributes({name: 'John', foo: 'bar'},
+            function(err, p) {
+              if (err) return done(err);
+              should.not.exist(err);


this is redundant, if (err) has already handled this case.

bajtos · 2016-09-19T12:19:16Z

+              Person.findById(p.id, function(e, p) {
+                if (e) return done(e);
+                p.name.should.equal('John');
+                should.not.exist(p.unknownVar);


p.should.not.have.property('unknownVar')?

bajtos · 2016-09-19T12:19:41Z

  if (strict === undefined) {
    strict = ctor.definition.settings.strict;
  }
+  if (strict === 'throw') {


Perhaps join with the line above?

} else if (string === 'throw') {

bajtos · 2016-09-19T12:24:37Z

@slnode test please

- Deprecation of strict:validate and strict:throw - When strict mode is enabled, it will now always return validation error (previous strict:validate)

davidcheung · 2016-09-19T15:19:16Z

@slnode test please

davidcheung · 2016-09-19T18:24:21Z

@slnode test please

davidcheung added the breaking-change label Sep 7, 2016

davidcheung self-assigned this Sep 7, 2016

davidcheung added the #review label Sep 7, 2016

davidcheung force-pushed the strict-mode-cleanup branch from 1ed2972 to 60d0599 Compare September 7, 2016 21:06

davidcheung reviewed Sep 7, 2016
View reviewed changes

davidcheung force-pushed the strict-mode-cleanup branch from 11fa30a to edfde85 Compare September 8, 2016 18:14

bajtos reviewed Sep 9, 2016
View reviewed changes

davidcheung force-pushed the strict-mode-cleanup branch 2 times, most recently from 39da2dc to 98ac9a5 Compare September 9, 2016 19:55

bajtos suggested changes Sep 16, 2016

View reviewed changes

davidcheung force-pushed the strict-mode-cleanup branch 2 times, most recently from 749f973 to 2ec1c2c Compare September 16, 2016 15:45

bajtos approved these changes Sep 19, 2016

View reviewed changes

davidcheung force-pushed the strict-mode-cleanup branch 2 times, most recently from 47d6f73 to 805db78 Compare September 19, 2016 14:27

Strict mode now always return validationError

805db78

- Deprecation of strict:validate and strict:throw - When strict mode is enabled, it will now always return validation error (previous strict:validate)

davidcheung merged commit 373e038 into master Sep 20, 2016

davidcheung deleted the strict-mode-cleanup branch September 20, 2016 12:24

davidcheung removed the #review label Sep 20, 2016

Conversation

davidcheung commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Sep 9, 2016

Uh oh!

davidcheung commented Sep 14, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Sep 19, 2016

Uh oh!

davidcheung commented Sep 19, 2016

Uh oh!

davidcheung commented Sep 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bajtos Sep 19, 2016 •

edited

Loading