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: nested property coercion and resolution #1702

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
4 participants
@b-admike
Copy link
Member

commented Mar 21, 2019

Description

Ensure that nested array properties maintain connector-specific data type metadata and also coerce string values for array of primitive properties into their type. Connect to strongloop/loopback-connector-mongodb#493.

Update:
This fix is for
a) coercing Date array properties from string representations to Date
b) making sure we use the coerced values from model instance on update operation (applies to all primitive data types like number, Date, etc)

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@b-admike b-admike requested review from raymondfeng, hacksparrow, bajtos, jannyHou and strongloop/sq-lb-apex Mar 21, 2019

// Skip the type property but don't delete it Model.extend() shares same instances of the properties from the base class
if (a !== 'type') {
if (a !== 'type' && !isNestedArrayProp) {

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Mar 22, 2019

Member

I have landed a PR handling a condition like this - #1699.

It makes me wonder if some architectural optimization can be done around this structure, instead of checking if a structure is an array within an array (including my implementation).

@bajtos @raymondfeng

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 22, 2019

Member

+1 to find a way how to address this problem in a wider context.

I am proposing the following:

  • Land this PR first, to fix the immediate problem
  • Create a follow-up spike story to investigate better solutions. Time-box it to 2-3 days only. I don't want to spend too much time on improving this legacy codebase, we are likely to throw most of it away as part of juggler refactor in the future.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 3, 2019

Author Member

Actually, I've talked to @raymondfeng and have concluded to revert this change. Right now the following two flavours of an array property work:

   const model = db.define('test', {
      randomReview: {
        type: [
          {
            type: 'string',
          },
        ],
        mongodb: {
          dataType: 'Decimal128',
        },
      },
    });

or

   const model = db.define('test', {
      randomReview: {
        type: [String ],
        mongodb: {
          dataType: 'Decimal128',
        },
      },
    });

Thoughts?

@bajtos
Copy link
Member

left a comment

I am not able to fully asses possible impacts of this change in a wider context. Having said that, your proposal looks reasonable to me.

I am not sure if we want to fix support for data structures with even deeper nesting than you have in our examples, PTAL at my comments below.

I am ok to leave such edge cases out of scope of this pull request if you prefer. I think your patch is a good improvement that's worth landing even if it does not fix everything.

const propDef = prop[a][0];
for (const a in propDef) {
if (a !== 'type') {
typeDef[a] = propDef[a];

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 22, 2019

Member

What if propDef[a] is another array? I think we need to recursively apply the same logic you are applying at line 243 above.

Please add a unit test to cover this scenario.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 3, 2019

Author Member

Reverting these changes as mentioned above. Let's discuss.

if (Array.isArray(type) && type[0].name) {
const actualType = type[0];
if (BASE_TYPES[actualType.name] && Array.isArray(self.__data[p])) {
self.__data[p].forEach((ele, idx) => self.__data[p][idx] = new actualType(ele));

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 22, 2019

Member

ele sounds weird to me, can we use elem instead? Or even it (standing for array item).

Also I think this can be more elegantly expressed using map function?

self._data[p] = self._data[p].map(item => new actualType(item));

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

I think this can be more elegantly expressed using map function?

self._data[p] = self._data[p].map(item => new actualType(item));

☝️

@@ -333,6 +333,13 @@ ModelBaseClass.prototype._initProperties = function(data, options) {
if (propVal === undefined && persistUndefinedAsNull) {
self.__data[p] = propVal = null;
}
// coerce array values of primitives from string
if (Array.isArray(type) && type[0].name) {

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 22, 2019

Member

What if a non-primitive type is nested deeper? I am thinking about something like this:

type = [
  // an array of anonymous objects with the following structure:
  {
    // name is a string
    name: String,
    // roles hold an array of Role instances
    roles: [Role]
  }
]

Do we need to handle this case?

If anonymous objects are converted into named models, and thus my example above will not trigger this particular line of code, then how about array of arrays?

// Cell is a model, let's say it has one property "color"

// 2D array of Cell instances
type = [[Cell]]; 

// Example value:
propValue = [
 [{color: 'black'}, {color: 'white'}],
 [{color: 'white'}, {color: 'black'}],
];

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 11, 2019

Author Member

I would like to tackle this in a follow-up PR. Will create an issue for it.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

@b-admike have you created that issue yet? Please post a link to it here.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 12, 2019

Author Member

Yes I've created it now :) #1716.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Author Member

I think this case should be fixed now since the actual bug was the fact that we didn't coerce array types of dates properly, but we do traverse through nested properties properly. As part of #1716, we should probably write tests first to verify before working on a fix.

@bajtos

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

have concluded to revert this change. Right now the following two flavours of an array property work

Sounds good to me 👍

@bajtos
Copy link
Member

left a comment

Some of my older comments are still relevant.

Please check why the CI builds are failing.

@@ -333,6 +333,13 @@ ModelBaseClass.prototype._initProperties = function(data, options) {
if (propVal === undefined && persistUndefinedAsNull) {
self.__data[p] = propVal = null;
}
// coerce array values of primitives from string
if (Array.isArray(type) && type[0].name) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

@b-admike have you created that issue yet? Please post a link to it here.

if (Array.isArray(type) && type[0].name) {
const actualType = type[0];
if (BASE_TYPES[actualType.name] && Array.isArray(self.__data[p])) {
self.__data[p].forEach((ele, idx) => self.__data[p][idx] = new actualType(ele));

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

I think this can be more elegantly expressed using map function?

self._data[p] = self._data[p].map(item => new actualType(item));

☝️

@bajtos
Copy link
Member

left a comment

This is tricky :(

});
created.bunchOfDates[0].should.be.an.instanceOf(Date);
created.bunchOfDates[0].should.equal(Date(dateVal));
});

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

I think it would be good to test coercion for array types defined using the approach shown in the first test too.

const dateArrayModel = db.define('dateArrayModel', {
  bunchOfDates: {
    type: [Date],
  },
});

// ...
created.bunchOfDates[0].should.be.an.instanceOf(Date);
created.bunchOfDates[0].should.equal(Date(dateVal));

Unless that's something we have already covered by other tests?

Similarly, test how connector-specific properties are preserved for the second flavor.

const model = db.define('model', {
   bunchOfDates: [
     {
       type: Date,
        // is this the right syntax?
        mongodb: {
           dataType: 'Decimal128',
        },  
     },
   ],
 });

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 12, 2019

Author Member

Okay I can try this out as well, I'll check if there is existing coverage.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 15, 2019

Author Member

I think it would be good to test coercion for array types defined using the approach shown in the first test too.

Agreed, will add the test case in my next commit.

Similarly, test how connector-specific properties are preserved for the second flavor.

Should we allow this? I don't think we handle this case and this was what I tried to fix with my first commit, but I feel like it can be addressed properly in a wider context and we should advocate our users use first flavour (prop: {type:[String], mongodb: ...}. FYI this is how we interpret that definition:
def:

     customMongoProp2: [{
        type: String,
        mongodb: {
          dataType: 'Decimal128',
        },
      },

result:

model.definition.properties.customMongoProp2
Object {0: Object, type: Array(1)}
should:Assertion
type:Array(1) []
0:Object {type: , mongodb: Object}
__proto__:Object {constructor: , __defineGetter__: , __defineSetter__: , …}

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 16, 2019

Contributor

Just followed the discussion above, I think the definition is missing an object property name:

const model = db.define('model', {
   bunchOfDates: [
     {
       // Please note the object should contain each property name and its definition as a key-value pair
       date: {
         type: Date,
         // is this the right syntax?
         mongodb: {
           dataType: 'Decimal128',
         },  
       }
     },
   ],
 });

This comment has been minimized.

Copy link
@jannyHou

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Author Member

Thanks Janny. I think the use cases you pointed out are covered in the PR I created for mongodb (see strongloop/loopback-connector-mongodb#501). We are looking at specifying an array type like {"emails": [{"type": "string", "length": 64}]} (see https://loopback.io/doc/en/lb3/LoopBack-types.html#array-types). I see the same results for even non-connector specific properties, so I think it's possibly a bug or we support it and maybe I need to change my assertions:

model.definition.properties.emails
Object {0: Object, type: Array(1)}
should:Assertion
type:Array(1) []
0:Object {type: "string", length: 64}
__proto__:Object {constructor: , __defineGetter__: , __defineSetter__: , …}

I'm going to leave it out of this PR since the fix in this PR is for a) making sure Date array properties are properly coerced, and b) making sure we coerce primitive types on update operation.
cc @bajtos

type: Date,
},
],
});

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Member

On the second thought, I am not sure if this is a valid syntax. I remember we were recently fixing a problem where properties called type were treated incorrectly.

What are the rules for deciding whether this model contains an array of Date instances ([dateVal, dateVal]), vs. an array of anonymous objects that have a type property containing a date instance ([{type: dateVal}, {type: dateVal}])?

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 12, 2019

Author Member

Good question. I think the fix we made recently allows users to use type name for properties IIRC. In my head, it'd be something like:

prop: {
 type: [Date]
}

versus

prop: {
 type: {
   type: [Date]
 }
}
@jannyHou
Copy link
Contributor

left a comment

@b-admike Great effort on the coercion fix!

I read though your chat with the customer in strongloop/loopback-connector-mongodb#493, just a clue: maybe your change is not picked up because both loopback-connector-mongodb and loopback-connector are still using juggler@3.x as devDependency. And your fix(at least this PR) is based on master, which is juggler@4.x.

And the PR seems fixes the coercion for Date type property, how would it affect the decimal coercion in the mongodb connector?

const updated = await numAndDateModel.updateAll({id: created.id}, updateData);
const found = await numAndDateModel.findById(created.id);
found.dateProp.should.deepEqual(new Date(updateDate));
found.dateArray[1].should.deepEqual(new Date(updateDate));

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 16, 2019

Contributor

nitpick: shall we assert found.dateArray[0] and found.numArray[0] as well?

@@ -2293,6 +2293,7 @@ DataAccessObject.updateAll = function(where, data, options, cb) {
if (!(data instanceof Model)) {
try {
inst = new Model(data, {applyDefaultValues: false});
ctx.data = inst.toObject(true);

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 16, 2019

Contributor

what does true mean here?

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Author Member

Good question. From the API documentation, it'll convert model instance to Plain Data Object for properties defined in the schema:

/**
 * Convert model instance to a plain JSON object.
 * Returns a canonical object representation (no getters and setters).
 *
 * @param {Boolean} onlySchema Restrict properties to dataSource only.  Default is false.  If true, the function returns only properties defined in the schema;  Otherwise it returns all enumerable properties.

I followed what was done for create (see https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L353) for this statement.

});
created.bunchOfDates[0].should.be.an.instanceOf(Date);
created.bunchOfDates[0].should.equal(Date(dateVal));
});

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 16, 2019

Contributor

Just followed the discussion above, I think the definition is missing an object property name:

const model = db.define('model', {
   bunchOfDates: [
     {
       // Please note the object should contain each property name and its definition as a key-value pair
       date: {
         type: Date,
         // is this the right syntax?
         mongodb: {
           dataType: 'Decimal128',
         },  
       }
     },
   ],
 });
});
created.bunchOfDates[0].should.be.an.instanceOf(Date);
created.bunchOfDates[0].should.equal(Date(dateVal));
});

This comment has been minimized.

Copy link
@jannyHou
@bajtos

bajtos approved these changes Apr 16, 2019

Copy link
Member

left a comment

I don't see any obvious problems and don't have bandwidth to build a full understanding of the proposed changes.

FWIW, LGTM.

@b-admike b-admike force-pushed the fix/nested-property-resolution branch from 0eaaff5 to 0948c1e Apr 16, 2019

@b-admike b-admike force-pushed the fix/nested-property-resolution branch from 0948c1e to 48af738 Apr 16, 2019

@@ -2366,7 +2366,7 @@ describe('manipulation', function() {
it('should not coerce invalid values provided in where conditions', function(done) {
Person.update({name: 'Brett Boe'}, {dob: 'notadate'}, function(err) {
should.exist(err);
err.message.should.equal('Invalid date: notadate');
err.message.should.equal('Invalid date: Invalid Date');

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Author Member

FYI this change is because we coerce date values in update now, and then validate them using Model._coerce in https://github.com/strongloop/loopback-datasource-juggler/pull/1702/files#diff-d717a01e40d8164976e4468af1bce663R2330. I was going to return the invalid string value for the date instead in the first pass (during model instantiation), but we have other tests which expect Invalid Date to be returned for those values (https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L2226), and I feel like that'd not be backwards compatible as opposed to a not so helpful message here.

if (Array.isArray(item)) {
return item;
} else if (this.itemType === Date) {
if (item === null) return null;

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Author Member

FYI keeping the same conversion we do in https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/model-builder.js#L704-L708 for non-array Date properties here.

@b-admike b-admike merged commit 47addd9 into master Apr 16, 2019

14 checks passed

Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (48af738)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (48af738)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 84.072%
Details
loopback-datasource-juggler Success! (48af738)
Details
loopback-datasource-juggler/node=4.x,os=windows Success! (48af738)
Details
loopback-datasource-juggler/node=6.x,os=windows Success! (48af738)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected

@b-admike b-admike deleted the fix/nested-property-resolution branch Apr 16, 2019

b-admike added a commit that referenced this pull request Apr 30, 2019

test: define models in before hooks
Re-work tests from #1702 based on the failures observed in #1719
and cherry-pick those commits back to master using ES6 syntax.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.