-
Notifications
You must be signed in to change notification settings - Fork 236
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
Conversation
691e52e
to
418bd2a
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.
@b-admike Great effort!
I left a few comments for the tests. And could you make sure the tests pass? CI has failures.
test/mongodb.test.js
Outdated
'rating': '4.5', | ||
}, | ||
}, function(err, inst) { | ||
// inst.toObject().awards.prizeMoney.should.equal(Decimal128.fromString('25000.00')); |
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.
any reason to comment this assertion?
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.
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.
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.
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.
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.
findById
did not work, but yeah perhaps using custom MongoDB command would work here 👍
test/mongodb.test.js
Outdated
}; | ||
modelWithDeepNestedDecimalProps.updateAll({id: inst.id}, updateData, function(err, inst) { | ||
inst.toObject().imdb.rating.should.equal(Decimal128.fromString('7.5')); | ||
}); |
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.
do you want to test 'testDecimal' in the super nested innerObj
property as well(since you defined it in the model)?
lib/mongodb.js
Outdated
* @param {*} visitor A callback function which takes a property value and | ||
* definition to apply custom property coercion | ||
*/ | ||
function visitAllProperties(data, propDefMap, visitor) { |
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.
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));
}
}
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 like the proposal for the latter where we can preserve visitAllProperties
but call visitor
as setValue
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.
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.
lib/mongodb.js
Outdated
/** | ||
* | ||
* @param {*} data Plain Data Object for the matching property definition(s) | ||
* @param {*} propDefMap Property definition(s) which include information about property type |
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.
Have you considered accepting the entire model definition object, or even the model constructor? I think it will make this function easier to use.
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 have not but can take a stab at it.
lib/mongodb.js
Outdated
* @param {*} propDefinition Property definition which contains metadata for the | ||
* property type | ||
*/ | ||
function checkNested(propDefinition) { |
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.
function checkNested(propDefinition) { | |
function isNestedModel(propDefinition) { |
lib/mongodb.js
Outdated
* decimal128 type | ||
* @param {*} propertyDef A property definition containing metadata about property type | ||
*/ | ||
function checkDecimalProp(propertyDef) { |
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.
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
}
lib/mongodb.js
Outdated
if (!propDefinition.type.definition) { | ||
return false; | ||
} else { | ||
if (propDefinition.type.definition.properties) return true; |
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 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;
}
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.
That's an elegant way to put it, thank you I agree.
test/mongodb.test.js
Outdated
@@ -308,6 +312,142 @@ describe('mongodb connector', function() { | |||
} | |||
); | |||
|
|||
modelWithDecimalArray = db.define('modelWithDecimalArray', { |
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.
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.
lib/mongodb.js
Outdated
* @param {*} visitor A callback function which takes a property value and | ||
* definition to apply custom property coercion | ||
*/ | ||
function visitAllProperties(data, propDefMap, visitor) { |
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.
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.
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.
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.
lib/mongodb.js
Outdated
* @param {*} def Property definition to check if property is MongoDB | ||
* decima type | ||
*/ | ||
function coerceDecimalProperties(value, def) { |
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.
Ditto, move this function out of convertDecimalProps
.
lib/mongodb.js
Outdated
* @param {*} propDefinition Property definition which contains metadata for the | ||
* property type | ||
*/ | ||
function checkNested(propDefinition) { |
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.
Ditto, move this function out of convertDecimalProps
.
lib/mongodb.js
Outdated
* decimal128 type | ||
* @param {*} propertyDef A property definition containing metadata about property type | ||
*/ | ||
function checkDecimalProp(propertyDef) { |
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.
Ditto, move this function out of convertDecimalProps
.
9c3fb73
to
12ed873
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.
The patch looks better now.
I am still not happy with the design of visitAllProperties
and also have few more minor comments, see below.
lib/mongodb.js
Outdated
if (isDecimal) { | ||
data[p] = Decimal128.fromString(data[p]); | ||
debug('convertDecimalProps decimal value: ', data[p]); | ||
function visitAllProperties(data, modelCtorOrDef, setValue) { |
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.
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.
function visitAllProperties(data, modelCtorOrDef, setValue) { | |
function visitAllProperties(data, modelCtorOrDef, visitor) { |
lib/mongodb.js
Outdated
visitAllProperties(value, def.type.definition, setValue); | ||
} | ||
} else { | ||
data[p] = setValue(value, def); |
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.
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.
data[p] = setValue(value, def); | |
visitor(value, def, (newValue) => { data[p] = newValue; }); |
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, this had unintended behaviour (backwards-incompatible) that was setting undefined values in the data object when we should skip them. Thank you!
lib/mongodb.js
Outdated
* @param {*} propDef Property definition to check if property is MongoDB | ||
* decima type | ||
*/ | ||
function coerceDecimalProperties(propValue, propDef) { |
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.
Since this is coercing a single property, I think a singular form would be better?
function coerceDecimalProperties(propValue, propDef) { | |
function coerceDecimalProperty(propValue, propDef) { |
lib/mongodb.js
Outdated
* | ||
* @param {*} propValue Property value to coerce into a Decimal128 value | ||
* @param {*} propDef Property definition to check if property is MongoDB | ||
* decima type |
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.
Typo: decima -> decimal.
Personally, I would use the exact type name as used by MongoDB, i.e. decimal128
.
* decima type | |
* decimal128 type |
lib/mongodb.js
Outdated
*/ | ||
function hasDataType(dataType, propertyDef) { | ||
return propertyDef && propertyDef.mongodb && | ||
propertyDef.mongodb.dataType && |
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.
propertyDef.mongodb.dataType && | |
propertyDef.mongodb.dataType && |
lib/mongodb.js
Outdated
function hasDataType(dataType, propertyDef) { | ||
return propertyDef && propertyDef.mongodb && | ||
propertyDef.mongodb.dataType && | ||
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase(); |
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.
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase(); | |
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase(); |
test/decimal.test.js
Outdated
.then(function(updatedInstance) { | ||
updatedInstance.randomReview[0].should.be.instanceOf(Decimal128); | ||
updatedInstance.randomReview[0].should.deepEqual(Decimal128.fromString('5.5')); | ||
done(); |
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.
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.
test/decimal.test.js
Outdated
}); | ||
}); | ||
|
||
function findModelInstance(modelName, id, cb) { |
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.
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:
function findModelInstance(modelName, id, cb) { | |
function findRawModelData(modelName, id, cb) { |
Similarly for findModelInstanceAsync
below.
test/decimal.test.js
Outdated
function findModelInstance(modelName, id, cb) { | ||
db.connector.execute(modelName, 'findOne', {_id: {$eq: id}}, {safe: true}, cb); | ||
} | ||
var findModelInstanceAsync = promisify(findModelInstance); |
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.
Use const
, not var
- see https://loopback.io/doc/en/contrib/style-guide-es6.html#variable-declarations
4878803
to
ba4faa6
Compare
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. |
Thanks for all the feedback @bajtos. I've applied them all. LGTY now? |
a429071
to
1b62f33
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.
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.
lib/mongodb.js
Outdated
* Decimal128 type | ||
*/ | ||
function coerceDecimalProperty(propValue, propDef, setValue) { | ||
var updatedValue; |
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.
var updatedValue; | |
let updatedValue; |
test/decimal.test.js
Outdated
modelWithDecimalArray, | ||
modelWithDecimalNestedArray, | ||
modelWithDecimalNestedObject, | ||
modelWithDeepNestedDecimalProps; |
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 move the declaration of modelWith*
properties to the test cases that are initializing them.
test/decimal.test.js
Outdated
}; | ||
let instanceId; | ||
|
||
modelWithDecimalNestedArray.create(createData) |
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.
❗️ Make sure to return the promise to Mocha, otherwise Mocha treats the test as if it finishes synchronously with success.
modelWithDecimalNestedArray.create(createData) | |
return modelWithDecimalNestedArray.create(createData) |
}, | ||
}, | ||
}, | ||
}, |
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 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.
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.
That sounds good. I will make a follow-up PR for these improvements 👍
test/mongodb.test.js
Outdated
@@ -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'); |
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 move ECONNREFUSED
assertion changes to a standalone commit, or even to a standalone pull request (to get them landed quickly).
6dc076d
to
d3e0863
Compare
Co-authored-by: Miroslav Bajtoš <mbajtoss@gmail.com>
d3e0863
to
fb41e40
Compare
The test failures below are what #505 aims to fix and are not introduced by this PR (also fail on master):
Therefore, I'm going to merge this PR and hopefully we can fix them in #505. |
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 loopbackio/loopback-datasource-juggler#1702 for model with property
randomReviews
(Array of decimal properties) to work.Related issues
Checklist
guide