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: allow coercion of nested properties [3.x] #1726

Merged
merged 1 commit into from May 3, 2019

Conversation

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

commented Apr 25, 2019

Description

Fixes #1723. Make sure we coerce nested property values for DAO methods like updateAll.

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 25, 2019

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

Merged

test: strict model update with mongo operators #510

2 of 2 tasks complete
@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@jannyHou
Copy link
Contributor

left a comment

👍 LGTM when CI passes.

@b-admike b-admike changed the title fix: allow coercion of nested properties fix: allow coercion of nested properties [3.x] Apr 25, 2019

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

Merged

fix: allow coercion of nested properties #1727

2 of 2 tasks complete
@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@slnode test please

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
@mitsos1os

mitsos1os Apr 25, 2019

Contributor

Here an actual boolean could be returned to keep function signature consistent. ex:
return !!(propType.definition && propType.definition.properties; or
return propType.hasOwnProperty('definition') && propType.definition.hasOwnProperty('properties');

} else {
self._coerce(value, options, DataType.definition);
}
}

This comment has been minimized.

Copy link
@mitsos1os

mitsos1os Apr 25, 2019

Contributor

I might be missing something here, but by taking a quick look, if nested model is found, couldn't it just continue on to next iteration instead of following the rest of the logic?

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 26, 2019

Author Member

If the nested model is not an array, then we are recursively sending its definition to use with the _coerce function here.

This comment has been minimized.

Copy link
@mitsos1os

mitsos1os Apr 26, 2019

Contributor

Yes I understand that. I was talking about using a continue before closing the NestedModel is between line 409 and 410

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Thanks for your feedback @mitsos1os, I've addressed them. LGTY now?

@mitsos1os

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@b-admike LGTM! Thanks for reviewing the feedback!

@b-admike b-admike force-pushed the fix/nested-property-coercion branch from ad9402b to 54100b3 May 2, 2019

@bajtos

bajtos approved these changes May 3, 2019

Copy link
Member

left a comment

I don't see any obvious problems. FWIW, LGTM.

@b-admike

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@slnode test please

@b-admike b-admike force-pushed the fix/nested-property-coercion branch 5 times, most recently from 6181d96 to 4a590c8 May 3, 2019

fix: allow coercion of nested properties
Handle model definitions with nested property
definitions for coercion of primitive values.

@b-admike b-admike force-pushed the fix/nested-property-coercion branch from 4a590c8 to 4924241 May 3, 2019

@b-admike b-admike merged commit 4f31db1 into 3.x May 3, 2019

28 of 35 checks passed

[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] downstream: loopback-connector-dashdb@master Failed! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-db2@master Failed! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-mongodb@3.x Failed! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-oracle@master Failed! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-rest@master Failed! (4924241)
Details
[cis-jenkins] downstream: loopback-ibmdb@master Failed! (4924241)
Details
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] downstream: angular-live-set@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-cassandra@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-cloudant@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-couchdb2@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-grpc@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-kv-redis@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-mongodb@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-mssql@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-mysql@4.x Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-mysql@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-postgresql@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector-remote@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-connector@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback-sdk-angular@master Success! (4924241)
Details
[cis-jenkins] downstream: loopback@master Success! (4924241)
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (4924241)
Details
[cis-jenkins] x64 && linux && nvm,6 Success! (4924241)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (4924241)
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
loopback-datasource-juggler Success! (4924241)
Details
loopback-datasource-juggler/node=4.x,os=windows Success! (4924241)
Details
loopback-datasource-juggler/node=6.x,os=windows Success! (4924241)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected

@b-admike b-admike deleted the fix/nested-property-coercion branch May 3, 2019

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.