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

feat: adds mongodb: {dataType: 'ObjectID'} to model properties #517

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
4 participants
@hacksparrow
Copy link
Member

commented May 9, 2019

Adds mongodb: {dataType: 'ObjectID'} to model properties. Enforces strictObjectIDCoercion settings on all model properties.

Related issues

strongloop/loopback-next#2085

  • connect to <link_to_referenced_issue>

Checklist

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

@hacksparrow hacksparrow changed the title feat: add isObjectID to property options [WIP]feat: add isObjectID to property options May 9, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 4 times, most recently from 954f159 to 35af7d6 May 9, 2019

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

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 2 times, most recently from ba18b44 to 3bc1a45 May 9, 2019

Show resolved Hide resolved lib/mongodb.js Outdated

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 3 times, most recently from c6a2db5 to 2e4c2af May 9, 2019

@hacksparrow hacksparrow changed the title [WIP]feat: add isObjectID to property options feat: add isObjectID to property options May 9, 2019

@hacksparrow hacksparrow changed the title feat: add isObjectID to property options [WIP]feat: add isObjectID to property options May 9, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 2 times, most recently from 693ccf4 to 9836d99 May 14, 2019

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

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from 9836d99 to 9858fb0 May 14, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 5 times, most recently from 62965ce to 305e0e0 May 15, 2019

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

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from fe9e78a to 622d7ac May 24, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 4 times, most recently from adc266a to 3b73d23 May 24, 2019

@hacksparrow hacksparrow changed the title [WIP]feat: adds mongodb: {dataType: 'ObjectID'} to model properties feat: adds mongodb: {dataType: 'ObjectID'} to model properties May 27, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from 3b73d23 to e6d504b May 27, 2019

Show resolved Hide resolved lib/mongodb.js Outdated
if (isDecimal) {
cond = Decimal128.fromString(cond);
debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name);
}
}

This comment has been minimized.

Copy link
@bajtos

bajtos May 27, 2019

Member

I think you need to handle prop.mongodb.dataType.toLowerCase() === 'objectid' too, so that string properties stored as ObjectIDs are handled in where queries too. Please start with a unit test that fails now.

I am ok to leave this part out of scope of this pull request if you prefer, but I think it should be implemented before we can call the user story done.

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow May 27, 2019

Author Member

Given:

const ObjectIdTypeRegex = /objectid/i;

Shouldn't ObjectIdTypeRegex.test() take care of things?

This comment has been minimized.

Copy link
@b-admike

b-admike May 29, 2019

Member

That should do it. Can we add a test where we specify the type as dataType: 'objectid'?

This comment has been minimized.

Copy link
@bajtos

bajtos May 31, 2019

Member

Shouldn't ObjectIdTypeRegex.test() take care of things?

What will happen when the value is not an ObjectID string? IIUC, in your proposal the query will return no records, because the non-ObjectID string will not match any of the ObjectID values stored in the database.

Are we fine with that behavior?

I was thinking that we should reject such requests with 400 Bad Request error instead.

Also, what happens when strictObjectIDCoercion is enabled and the property is defined as {type: 'string', mongodb: {dataType: 'ObjectID'}}. Will we correctly coerce the string value to ObjectID, so that it can match any existing values in the database?

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow May 31, 2019

Author Member

I got your point, I was thinking of something else.

This comment has been minimized.

Copy link
@b-admike

b-admike May 31, 2019

Member

Shouldn't ObjectIdTypeRegex.test() take care of things?

What will happen when the value is not an ObjectID string? IIUC, in your proposal the query will return no records, because the non-ObjectID string will not match any of the ObjectID values stored in the database.

Are we fine with that behavior?

I was thinking that we should reject such requests with 400 Bad Request error instead.

Also, what happens when strictObjectIDCoercion is enabled and the property is defined as {type: 'string', mongodb: {dataType: 'ObjectID'}}. Will we correctly coerce the string value to ObjectID, so that it can match any existing values in the database?

I misunderstood this, sorry. Rejecting non-objectID string makes sense to me (I thought this was the behaviour). Also, for the case when strict coercion flag is enabled, we should coerce string values to ObjectID, and also reject/throw an error if we can't (invalid values).

This comment has been minimized.

Copy link
@b-admike

b-admike May 31, 2019

Member

@bajtos @hacksparrow is this something we can create a follow-up story on?

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Jun 3, 2019

Author Member

Let's create a follow up story.

Show resolved Hide resolved lib/mongodb.js Outdated
Show resolved Hide resolved lib/mongodb.js Outdated
Show resolved Hide resolved lib/mongodb.js
Show resolved Hide resolved lib/mongodb.js
@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

If both models are backed by MongoDB, I think we need to ensure that categoryId property is always set to ObjectID value, even if the caller provided a string only (e.g. because categoryId was obtained from HTTP request path like /api/categories/{some-objectid-string}/products.

@bajtos Thank you for the detailed explanation 👍 I forgot the relation constraint. Fair enough.

Show resolved Hide resolved lib/mongodb.js
@b-admike
Copy link
Member

left a comment

Looking great. Thanks for adding some docs in the README. I've put my thoughts on the ongoing discussions.

if (isDecimal) {
cond = Decimal128.fromString(cond);
debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name);
}
}

This comment has been minimized.

Copy link
@b-admike

b-admike May 29, 2019

Member

That should do it. Can we add a test where we specify the type as dataType: 'objectid'?

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

it('should throw if id is not a ObjectID-like string', async function() {
// Why is this not working?
// await User.create({id: 'abc', name: 'John'}).should.be.rejectedWith(/not an ObjectID string/);

This comment has been minimized.

Copy link
@b-admike

b-admike May 29, 2019

Member

Here are two ways of doing it (one is for expect/chai, one for mocha):
expect:
https://github.com/strongloop/loopback-next/blob/75939a4a144b3068ff2890394c178faad58d7458/packages/cli/test/integration/generators/relation.integration.js#L39-L48

Mocha:
https://github.com/strongloop/loopback-datasource-juggler/blob/91ab3e916281e56e8675889ab7c5a31dc223c22c/test/datasource.test.js#L446-L451

I suggest trying to have a promise based test:

it('should throw if id is not an ObjectID-like string', () => {

  return user.create({id: 'abc', name: 'John'}).should.be.rejectedWith(/not an ObjectID string/);
});

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow May 30, 2019

Author Member

return user.create({id: 'abc', name: 'John'}).should.be.rejectedWith(/not an ObjectID string/);

Doesn't help.

Would it be ok to replace should with expect in this repo? Will do so in a separate PR after this one lands, if ok.

This comment has been minimized.

Copy link
@b-admike

b-admike May 30, 2019

Member

Maybe my example is wrong. I didn't get to try it out. I'll try it out and let you know. In terms of replacing should with expect, it is ok, but not sure how much effort that is and how much beneficial it is.

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

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch 3 times, most recently from 6fb8e55 to 9114a18 May 30, 2019

@bajtos

bajtos approved these changes May 31, 2019

Copy link
Member

left a comment

I think the most important problems were addressed by now, but I don't have enough bandwidth to review the changes in full detail.

Please work with @b-admike and @jannyHou to get their approval, they have much better understanding of this codebase and MongoDB features than I do.

if (isDecimal) {
cond = Decimal128.fromString(cond);
debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name);
}
}

This comment has been minimized.

Copy link
@bajtos

bajtos May 31, 2019

Member

Shouldn't ObjectIdTypeRegex.test() take care of things?

What will happen when the value is not an ObjectID string? IIUC, in your proposal the query will return no records, because the non-ObjectID string will not match any of the ObjectID values stored in the database.

Are we fine with that behavior?

I was thinking that we should reject such requests with 400 Bad Request error instead.

Also, what happens when strictObjectIDCoercion is enabled and the property is defined as {type: 'string', mongodb: {dataType: 'ObjectID'}}. Will we correctly coerce the string value to ObjectID, so that it can match any existing values in the database?

Show resolved Hide resolved lib/mongodb.js
Show resolved Hide resolved lib/mongodb.js
@b-admike
Copy link
Member

left a comment

Changes LGTM 👍. I think there are two discussions where we can create follow-up tasks/PRs on.

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from 9114a18 to ab4d3f7 Jun 3, 2019

@hacksparrow

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from ab4d3f7 to a909ff1 Jun 3, 2019

@hacksparrow

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Ok, all green now. Let's land this today.

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from a909ff1 to 3503be6 Jun 3, 2019

feat: add mongodb.dataType to property definition
Adds mongodb.dataType to model property definition

Feedback

@hacksparrow hacksparrow force-pushed the feat/isObjectID branch from 3503be6 to 8d86bdc Jun 3, 2019

@b-admike
Copy link
Member

left a comment

LGTM 🎉

@jannyHou
Copy link
Contributor

left a comment

👏 LGTM great effort!

@hacksparrow hacksparrow merged commit 8d86bdc into master Jun 3, 2019

13 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 && dbs,10 Success! (8d86bdc)
Details
[cis-jenkins] x64 && linux && nvm && dbs,8 Success! (8d86bdc)
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-connector-mongodb Success! (8d86bdc)
Details
loopback-connector-mongodb/node=4.x,os=windows Success! (8d86bdc)
Details
loopback-connector-mongodb/node=6.x,os=windows Success! (8d86bdc)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected

@delete-merged-branch delete-merged-branch bot deleted the feat/isObjectID branch Jun 3, 2019

@hacksparrow hacksparrow referenced this pull request Jun 14, 2019

Merged

fix: ObjectID data type preservation #525

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.