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 decimal props #501

Merged
merged 1 commit into from Apr 12, 2019

Conversation

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

commented Apr 2, 2019

Description

Fixes #493; Coerce decimal property values from string to decimal128 at every level of the model definition tree. The decimal properties can be nested in an Array, Object, and any combination of those. Depends on strongloop/loopback-datasource-juggler#1702 for model with property randomReviews (Array of decimal properties) to work.

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 self-assigned this Apr 2, 2019

@b-admike b-admike requested a review from strongloop/sq-lb-apex Apr 2, 2019

@b-admike b-admike requested review from dhmlau, jannyHou and virkt25 as code owners Apr 2, 2019

@b-admike b-admike force-pushed the fix/nested-decimal-props branch 2 times, most recently from 691e52e to 418bd2a Apr 2, 2019

@jannyHou
Copy link
Contributor

left a comment

@b-admike Great effort!

I left a few comments for the tests. And could you make sure the tests pass? CI has failures.

'rating': '4.5',
},
}, function(err, inst) {
// inst.toObject().awards.prizeMoney.should.equal(Decimal128.fromString('25000.00'));

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 3, 2019

Contributor

any reason to comment this assertion?

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 3, 2019

Author Member

There is a bug in juggler right now where these assertions will fail because juggler doesn't return the updated model instance on create/update :(. Even though they are stored as decimal128 in mongo, we get back string values. I aim to fix that in juggler, but I was manually able to verify that they are stored correctly.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

because juggler doesn't return the updated model instance on create/update

I think that's an intentional behavior, see https://loopback.io/doc/en/lb3/Operation-hooks.html

By default, create and updateAttributes do not apply database updates to the model instance returned to the callback, therefore any changes made by “loaded” hooks are discarded. To change this behavior, set a per-model option updateOnLoad: true.

I think it's more important to verify what has been actually stored by the database. You can start by calling findById to fetch the actual data. IDK if decimal128 type is preserved on that path. If not, then you may need to execute custom MongoDB command to access raw data.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 5, 2019

Author Member

findById did not work, but yeah perhaps using custom MongoDB command would work here 👍

};
modelWithDeepNestedDecimalProps.updateAll({id: inst.id}, updateData, function(err, inst) {
inst.toObject().imdb.rating.should.equal(Decimal128.fromString('7.5'));
});

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 3, 2019

Contributor

do you want to test 'testDecimal' in the super nested innerObj property as well(since you defined it in the model)?

* @param {*} visitor A callback function which takes a property value and
* definition to apply custom property coercion
*/
function visitAllProperties(data, propDefMap, visitor) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

When I see the name visitAllProperties, I expect the function to be read-only, i.e. only visit the properties but do not change their values (unless the visitor does that).

Let's come up with a better name please, one that will clearly communicate the fact that this is changing property values. For example, editAllPropertyValues

Alternatively, and I think this make this new function useful in more situations, we can change the signature of the visitor callback to allow visitors to change property values (if they want). For example:

visitAllProperties(data, def, (propVal, propDef, setValue) => {
  if (checkDecimalProp(propDef)) {
    setValue(Decimal128.fromString(propValue));
  }
}

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 5, 2019

Author Member

I like the proposal for the latter where we can preserve visitAllProperties but call visitor as setValue instead 👍

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

I am afraid you misunderstood my second proposal. I was not saying to rename visitor to setValue, that would be confusing.

I'll post a new comment in the updated code.

/**
*
* @param {*} data Plain Data Object for the matching property definition(s)
* @param {*} propDefMap Property definition(s) which include information about property type

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Have you considered accepting the entire model definition object, or even the model constructor? I think it will make this function easier to use.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 5, 2019

Author Member

I have not but can take a stab at it.

* @param {*} propDefinition Property definition which contains metadata for the
* property type
*/
function checkNested(propDefinition) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member
Suggested change
function checkNested(propDefinition) {
function isNestedModel(propDefinition) {
* decimal128 type
* @param {*} propertyDef A property definition containing metadata about property type
*/
function checkDecimalProp(propertyDef) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member
Suggested change
function checkDecimalProp(propertyDef) {
function isDecimal128(propertyDef) {

Personally, I'd use a more generic variant.

function hasDataType(dataType, propertyDef) {
  return propertyDef && propertyDef.mongodb &&
    propertyDef.mongodb.dataType &&
    propertyDef.mongodb.dataType.toLowerCase() == dataType.toLowerCase();
}

// usage:
if (hasDataType('decimal128', propertyDef) {
  // coerce the properties
}
if (!propDefinition.type.definition) {
return false;
} else {
if (propDefinition.type.definition.properties) return true;

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

I think this function can be simplified by accepting the type instead of full property definition.

function isNestedModel(propType) {
  if (!propType) return false;
  if (Array.isArray(propType)) return isNestedModel(propType[0]);
  return propType.definition && propType.definition.properties;
}

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 5, 2019

Author Member

That's an elegant way to put it, thank you I agree.

@@ -308,6 +312,142 @@ describe('mongodb connector', function() {
}
);

modelWithDecimalArray = db.define('modelWithDecimalArray', {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

AFAICT, each of these models are used by a single test only. Please move their definitions to the tests that are using them, it will make the tests easier to read and also will avoid temptation to add unrelated properties to these models in the future.

* @param {*} visitor A callback function which takes a property value and
* definition to apply custom property coercion
*/
function visitAllProperties(data, propDefMap, visitor) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

IMPORTANT❗️

IIUC, these new functions are defined inside convertDecimalProps. Since they are not (or at least should not) require anything from convertDecimalProps closure, they should be defined at the top-level, so that there is only one ("singleton") instance used by all invocations of convertDecimalProps. Such solution allows V8 to optimize performance.

Personally, I'd prefer to move this particular helper to loopback-connector, so that connectors for other databases can use it too. To land this pull request sooner, I think it will be best to make that change (move) after this patch is landed.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 5, 2019

Author Member

IIUC, these new functions are defined inside convertDecimalProps. Since they are not (or at least should not) require anything from convertDecimalProps closure, they should be defined at the top-level, so that there is only one ("singleton") instance used by all invocations of convertDecimalProps. Such solution allows V8 to optimize performance.

Good to know, thanks! That makes a lot of sense. Yea we can think of moving this function out to loopback-connector as discussed after this patch is landed.

* @param {*} def Property definition to check if property is MongoDB
* decima type
*/
function coerceDecimalProperties(value, def) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Ditto, move this function out of convertDecimalProps.

* @param {*} propDefinition Property definition which contains metadata for the
* property type
*/
function checkNested(propDefinition) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Ditto, move this function out of convertDecimalProps.

* decimal128 type
* @param {*} propertyDef A property definition containing metadata about property type
*/
function checkDecimalProp(propertyDef) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Ditto, move this function out of convertDecimalProps.

Show resolved Hide resolved test/mongodb.test.js Outdated

@b-admike b-admike force-pushed the fix/nested-decimal-props branch from 9c3fb73 to 12ed873 Apr 8, 2019

@bajtos
Copy link
Member

left a comment

The patch looks better now.

I am still not happy with the design of visitAllProperties and also have few more minor comments, see below.

if (isDecimal) {
data[p] = Decimal128.fromString(data[p]);
debug('convertDecimalProps decimal value: ', data[p]);
function visitAllProperties(data, modelCtorOrDef, setValue) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

Continuing discussion started earlier.

A function called visitAllProperties should not be modifying property values, it should only call the supplied visiter function for every property.

Suggested change
function visitAllProperties(data, modelCtorOrDef, setValue) {
function visitAllProperties(data, modelCtorOrDef, visitor) {
visitAllProperties(value, def.type.definition, setValue);
}
} else {
data[p] = setValue(value, def);

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

Continuing discussion started earlier. Instead of using the value returned by the visitor to update the property, we should provide the visitor function with a tool allowing the visitor to decide if the property value should be updated and then let the visitor to update it.

Suggested change
data[p] = setValue(value, def);
visitor(value, def, (newValue) => { data[p] = newValue; });

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 9, 2019

Author Member

Agreed, this had unintended behaviour (backwards-incompatible) that was setting undefined values in the data object when we should skip them. Thank you!

* @param {*} propDef Property definition to check if property is MongoDB
* decima type
*/
function coerceDecimalProperties(propValue, propDef) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

Since this is coercing a single property, I think a singular form would be better?

Suggested change
function coerceDecimalProperties(propValue, propDef) {
function coerceDecimalProperty(propValue, propDef) {
*
* @param {*} propValue Property value to coerce into a Decimal128 value
* @param {*} propDef Property definition to check if property is MongoDB
* decima type

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

Typo: decima -> decimal.

Personally, I would use the exact type name as used by MongoDB, i.e. decimal128.

Suggested change
* decima type
* decimal128 type
*/
function hasDataType(dataType, propertyDef) {
return propertyDef && propertyDef.mongodb &&
propertyDef.mongodb.dataType &&

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member
Suggested change
propertyDef.mongodb.dataType &&
propertyDef.mongodb.dataType &&
function hasDataType(dataType, propertyDef) {
return propertyDef && propertyDef.mongodb &&
propertyDef.mongodb.dataType &&
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase();

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member
Suggested change
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase();
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase();
.then(function(updatedInstance) {
updatedInstance.randomReview[0].should.be.instanceOf(Decimal128);
updatedInstance.randomReview[0].should.deepEqual(Decimal128.fromString('5.5'));
done();

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

Don't mix callbacks with promises. Mocha supports tests written with promises, use that feature please.

it('should create/update instance for array of decimal props', function() {
  // ...
  return modelWithDecimalArray.create(createData)
    // ...
   .then(function(updatedInstance) {
      updatedInstance.randomReview[0].should.be.instanceOf(Decimal128);
      updatedInstance.randomReview[0].should.deepEqual(Decimal128.fromString('5.5'));
    });
});

The same comment applies to other tests too.

});
});

function findModelInstance(modelName, id, cb) {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 9, 2019

Member

IIUC, this method is not returning an instance of the model class, just the raw model data. In which case I am proposing a slightly different name:

Suggested change
function findModelInstance(modelName, id, cb) {
function findRawModelData(modelName, id, cb) {

Similarly for findModelInstanceAsync below.

function findModelInstance(modelName, id, cb) {
db.connector.execute(modelName, 'findOne', {_id: {$eq: id}}, {safe: true}, cb);
}
var findModelInstanceAsync = promisify(findModelInstance);

This comment has been minimized.

@b-admike b-admike force-pushed the fix/nested-decimal-props branch from 4878803 to ba4faa6 Apr 9, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

For the last commit, I used https://github.com/strongloop/loopback-connector-mongodb/pull/483/files#diff-f44d7a6206dcaa5ddd05b31118b5c2be as inspiration to fix one of the failing tests on master.

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Thanks for all the feedback @bajtos. I've applied them all. LGTY now?

@b-admike b-admike force-pushed the fix/nested-decimal-props branch 2 times, most recently from a429071 to 1b62f33 Apr 9, 2019

@bajtos

bajtos approved these changes Apr 11, 2019

Copy link
Member

left a comment

Almost there! Please make sure all new promise-based tests are returning a promise to Mocha (see one of the comments below).

Other comments are minor and can be ignored. No further review is necessary as far as I am concerned.

It would be great if you could get one more person to approve the changes, e.g. @jannyHou.

* Decimal128 type
*/
function coerceDecimalProperty(propValue, propDef, setValue) {
var updatedValue;

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member
Suggested change
var updatedValue;
let updatedValue;
modelWithDecimalArray,
modelWithDecimalNestedArray,
modelWithDecimalNestedObject,
modelWithDeepNestedDecimalProps;

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member

Please move the declaration of modelWith* properties to the test cases that are initializing them.

};
let instanceId;

modelWithDecimalNestedArray.create(createData)

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member

❗️ Make sure to return the promise to Mocha, otherwise Mocha treats the test as if it finishes synchronously with success.

Suggested change
modelWithDecimalNestedArray.create(createData)
return modelWithDecimalNestedArray.create(createData)
},
},
},
},

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member

This model has way too many different properties, it's difficult to see what's going on. I would prefer to split this model & test into multiple smaller and focused tests.

Few example cases I see:

  • decimal property in object in array in object
  • decimal property in object in object in array in object in array in object
  • decimal property in object in array in object in array in object
  • decimal property in object in object in object in object in object
  • etc.

Do we really need to go this deep in our test cases? Isn't it enough to verify the few transitions (object -> array, array -> object) that the rest of behavior is composed from?

Feel free to address this comment out of scope of this pull request.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 12, 2019

Author Member

That sounds good. I will make a follow-up PR for these improvements 👍

@@ -345,7 +345,7 @@ describe('mongodb connector', function() {
});
ds.ping(function(err) {
(!!err).should.be.True();
err.message.should.match(/failed to connect to server/);
err.code.should.eql('ECONNREFUSED');

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member

Please move ECONNREFUSED assertion changes to a standalone commit, or even to a standalone pull request (to get them landed quickly).

@b-admike b-admike added the customer label Apr 11, 2019

@b-admike b-admike referenced this pull request Apr 12, 2019

Merged

ci: fix previously failing tests #505

2 of 2 tasks complete

@b-admike b-admike force-pushed the fix/nested-decimal-props branch from 6dc076d to d3e0863 Apr 12, 2019

fix: coerce deep nested decimal properties
Co-authored-by: Miroslav Bajtoš <mbajtoss@gmail.com>

@b-admike b-admike force-pushed the fix/nested-decimal-props branch from d3e0863 to fb41e40 Apr 12, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

The test failures below are what #505 aims to fix and are not introduced by this PR (also fail on master):

10:08:59   1128 passing (17s)
10:08:59   100 pending
10:08:59 
  3 failing
10:08:59 
10:08:59   1) lazyConnect
10:08:59        should report connection error (lazyConnect = false):
10:08:59      Uncaught AssertionError: expected 'connect ECONNREFUSED 127.0.0.1:4' to match /failed to connect to server/
10:08:59       at Assertion.fail (node_modules/should/cjs/should.js:275:17)
10:08:59       at Assertion.value [as match] (node_modules/should/cjs/should.js:356:19)
10:08:59       at DataSource.<anonymous> (test/mongodb.test.js:59:26)
10:08:59       at DataSource.postInit (node_modules/loopback-datasource-juggler/lib/datasource.js:480:16)
10:08:59       at /home/jenkins/workspace/nb/loopback-connector-mongodb~master/3ed5ebba/lib/mongodb.js:320:23
10:08:59       at err (node_modules/mongodb/lib/utils.js:411:14)
10:08:59       at executeCallback (node_modules/mongodb/lib/utils.js:401:25)
10:08:59       at err (node_modules/mongodb/lib/operations/mongo_client_ops.js:285:21)
10:08:59       at connectCallback (node_modules/mongodb/lib/operations/mongo_client_ops.js:265:5)
10:08:59       at server.connect (node_modules/mongodb/lib/operations/mongo_client_ops.js:353:14)
10:08:59       at Server.<anonymous> (node_modules/mongodb/lib/topologies/server.js:237:11)
10:08:59       at Pool.<anonymous> (node_modules/mongodb-core/lib/topologies/server.js:452:12)
10:08:59       at connect (node_modules/mongodb-core/lib/connection/pool.js:557:14)
10:08:59       at makeConnection (node_modules/mongodb-core/lib/connection/connect.js:38:11)
10:08:59       at callback (node_modules/mongodb-core/lib/connection/connect.js:260:5)
10:08:59       at Socket.err (node_modules/mongodb-core/lib/connection/connect.js:286:7)
10:08:59       at emitErrorNT (internal/streams/destroy.js:66:8)
10:08:59       at _combinedTickCallback (internal/process/next_tick.js:139:11)
10:08:59       at process._tickCallback (internal/process/next_tick.js:181:9)
10:08:59 
10:08:59   2) mongodb connector
10:08:59        does not execute a nested `$where`:
10:08:59      Uncaught AssertionError: expected Error {
10:08:59   code: 'OPERATOR_NOT_ALLOWED_IN_QUERY',
10:08:59   statusCode: 400,
10:08:59   details: Object {
10:08:59     operators: Array [ '$where' ],
10:08:59     where: Object {
10:08:59       content: Object {
10:08:59         $where: 'function() {return this.content.contains("content")}'
10:08:59       }
10:08:59     }
10:08:59   },
10:08:59   message: 'Operators "$where" are not allowed in query'
10:08:59 } to not exist
10:08:59       at Post.find (test/mongodb.test.js:830:24)
10:08:59       at /home/jenkins/workspace/nb/loopback-connector-mongodb~master/3ed5ebba/node_modules/loopback-datasource-juggler/lib/dao.js:1532:7
10:08:59       at _combinedTickCallback (internal/process/next_tick.js:132:7)
10:08:59       at process._tickCallback (internal/process/next_tick.js:181:9)
10:08:59 
10:08:59   3) mongodb connector
10:08:59        .ping(cb)
10:08:59          should report connection errors with invalid config:
10:08:59      Uncaught AssertionError: expected 'connect ECONNREFUSED 127.0.0.1:4' to match /failed to connect to server/
10:08:59       at Assertion.fail (node_modules/should/cjs/should.js:275:17)
10:08:59       at Assertion.value [as match] (node_modules/should/cjs/should.js:356:19)
10:08:59       at /home/jenkins/workspace/nb/loopback-connector-mongodb~master/3ed5ebba/test/mongodb.test.js:348:28
10:08:59       at DataSource.<anonymous> (lib/mongodb.js:1863:7)
10:08:59       at DataSource.postInit (node_modules/loopback-datasource-juggler/lib/datasource.js:480:16)
10:08:59       at /home/jenkins/workspace/nb/loopback-connector-mongodb~master/3ed5ebba/lib/mongodb.js:320:23
10:08:59       at err (node_modules/mongodb/lib/utils.js:411:14)
10:08:59       at executeCallback (node_modules/mongodb/lib/utils.js:401:25)
10:08:59       at err (node_modules/mongodb/lib/operations/mongo_client_ops.js:285:21)
10:08:59       at connectCallback (node_modules/mongodb/lib/operations/mongo_client_ops.js:265:5)
10:08:59       at server.connect (node_modules/mongodb/lib/operations/mongo_client_ops.js:353:14)
10:08:59       at Server.<anonymous> (node_modules/mongodb/lib/topologies/server.js:237:11)
10:08:59       at Pool.<anonymous> (node_modules/mongodb-core/lib/topologies/server.js:452:12)
10:08:59       at connect (node_modules/mongodb-core/lib/connection/pool.js:557:14)
10:08:59       at makeConnection (node_modules/mongodb-core/lib/connection/connect.js:38:11)
10:08:59       at callback (node_modules/mongodb-core/lib/connection/connect.js:260:5)
10:08:59       at Socket.err (node_modules/mongodb-core/lib/connection/connect.js:286:7)
10:08:59       at emitErrorNT (internal/streams/destroy.js:66:8)
10:08:59       at _combinedTickCallback (internal/process/next_tick.js:139:11)
10:08:59       at process._tickCallback (internal/process/next_tick.js:181:9)
10:08:59 

Therefore, I'm going to merge this PR and hopefully we can fix them in #505.

@b-admike b-admike merged commit 36f8980 into master Apr 12, 2019

5 of 14 checks passed

[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] x64 && linux && nvm && dbs,10 Failed! (fb41e40)
Details
[cis-jenkins] x64 && linux && nvm && dbs,6 Failed! (fb41e40)
Details
[cis-jenkins] x64 && linux && nvm && dbs,8 Failed! (fb41e40)
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
loopback-connector-mongodb Failed! (fb41e40)
Details
loopback-connector-mongodb/node=6.x,os=windows Failed! (fb41e40)
Details
pr-builder ${BUILD_FAILURE_ANALYZER}
Details
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
clahub All contributors have signed the Contributor License Agreement.
Details
loopback-connector-mongodb/node=4.x,os=windows Success! (fb41e40)
Details
security/snyk - package.json (StrongLoop) No new issues
Details

@b-admike b-admike deleted the fix/nested-decimal-props branch Apr 12, 2019

@b-admike b-admike referenced this pull request Apr 16, 2019

Merged

fix: nested property coercion and resolution #1702

0 of 2 tasks complete

@b-admike b-admike referenced this pull request Apr 30, 2019

Merged

Fix/nested decimal coercion logic #513

2 of 2 tasks complete
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.