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

hacksparrow · 2019-05-09T04:41:01Z

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

Related issues

loopbackio/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

test/id.test.js

lib/mongodb.js

test/id.test.js

lib/mongodb.js

bajtos · 2019-05-27T13:05:22Z

lib/mongodb.js

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


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.

Given:

const ObjectIdTypeRegex = /objectid/i;

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

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

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 got your point, I was thinking of something else.

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).

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

Let's create a follow up story.

@b-admike @hacksparrow @bajtos
Created #545 as I believe it relates to this conversation. This commit and its related code appears to fix the issue related to string-properties-stored-as-ObjectId but does not account for a property that is an Array of ObjectID or Array of ObjectID-like-strings.

lib/mongodb.js

jannyHou · 2019-05-27T13:35:07Z

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.

lib/mongodb.js

b-admike

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

b-admike · 2019-05-29T15:59:48Z

lib/mongodb.js

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


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

test/objectid.test.js

b-admike · 2019-05-29T16:19:58Z

test/id.test.js

+
+    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/);


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/); });

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.

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.

lib/mongodb.js

bajtos

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.

bajtos · 2019-05-31T09:16:50Z

lib/mongodb.js

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


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?

lib/mongodb.js

b-admike

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

hacksparrow · 2019-06-03T10:17:38Z

@b-admike any idea why this is failing? https://travis-ci.org/strongloop/loopback-connector-mongodb/jobs/540672910

hacksparrow · 2019-06-03T10:32:48Z

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

Adds mongodb.dataType to model property definition Feedback

b-admike

LGTM 🎉

jannyHou

👏 LGTM great effort!

hacksparrow requested review from b-admike, dhmlau, emonddr and jannyHou as code owners May 9, 2019 04:41

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

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

hacksparrow commented May 9, 2019

View reviewed changes

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

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

bajtos reviewed May 9, 2019

View reviewed changes

lib/mongodb.js Outdated Show resolved Hide resolved

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

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

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

dhmlau mentioned this pull request May 13, 2019

Mongo fails to find user by id (hasMany relation not working) loopbackio/loopback-next#2085

Closed

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

bajtos suggested changes May 14, 2019

View reviewed changes

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 Show resolved Hide resolved

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

dhmlau mentioned this pull request May 15, 2019

Type ObjectID for model property loopbackio/loopback-next#1875

Closed

hacksparrow force-pushed the feat/isObjectID branch 5 times, most recently from 62965ce to 305e0e0 Compare May 16, 2019 16:32

b-admike reviewed May 17, 2019

View reviewed changes

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

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

lib/mongodb.js Outdated Show resolved Hide resolved

hacksparrow force-pushed the feat/isObjectID branch 4 times, most recently from adc266a to 3b73d23 Compare May 27, 2019 12:45

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 force-pushed the feat/isObjectID branch from 3b73d23 to e6d504b Compare May 27, 2019 12:57

bajtos reviewed May 27, 2019

View reviewed changes

jannyHou reviewed May 28, 2019

View reviewed changes

lib/mongodb.js Show resolved Hide resolved

b-admike reviewed May 29, 2019

View reviewed changes

hacksparrow force-pushed the feat/isObjectID branch 3 times, most recently from 6fb8e55 to 9114a18 Compare May 31, 2019 08:53

bajtos approved these changes May 31, 2019

View reviewed changes

b-admike approved these changes May 31, 2019

View reviewed changes

hacksparrow force-pushed the feat/isObjectID branch from 9114a18 to ab4d3f7 Compare June 3, 2019 10:09

hacksparrow force-pushed the feat/isObjectID branch from ab4d3f7 to a909ff1 Compare June 3, 2019 10:20

hacksparrow force-pushed the feat/isObjectID branch from a909ff1 to 3503be6 Compare June 3, 2019 14:32

feat: add mongodb.dataType to property definition

8d86bdc

Adds mongodb.dataType to model property definition Feedback

hacksparrow force-pushed the feat/isObjectID branch from 3503be6 to 8d86bdc Compare June 3, 2019 14:33

b-admike approved these changes Jun 3, 2019

View reviewed changes

jannyHou approved these changes Jun 3, 2019

View reviewed changes

hacksparrow merged commit 8d86bdc into master Jun 3, 2019

delete-merged-branch bot deleted the feat/isObjectID branch June 3, 2019 15:56

hacksparrow mentioned this pull request Jun 14, 2019

fix: ObjectID data type preservation #525

Merged

2 tasks

bajtos mentioned this pull request Jun 14, 2019

Possible bug inside findOne with MongoDB loopbackio/loopback-next#2674

Closed

bajtos mentioned this pull request Sep 12, 2019

Robust handling of ObjectID type for MongoDB loopbackio/loopback-next#3720

Open

dhmlau mentioned this pull request Oct 15, 2019

coercePropertyValue Does Not Account For Array of ObjectID or Array of ObjectID strings #545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hacksparrow commented May 9, 2019 •

edited

Loading

bajtos May 27, 2019

hacksparrow May 27, 2019

b-admike May 29, 2019

bajtos May 31, 2019

hacksparrow May 31, 2019

b-admike May 31, 2019

b-admike May 31, 2019

hacksparrow Jun 3, 2019

taicho Oct 11, 2019

jannyHou commented May 27, 2019

b-admike left a comment

b-admike May 29, 2019

b-admike May 29, 2019

hacksparrow May 30, 2019 •

edited

Loading

b-admike May 30, 2019

bajtos left a comment

bajtos May 31, 2019

b-admike left a comment

hacksparrow commented Jun 3, 2019

hacksparrow commented Jun 3, 2019

b-admike left a comment

jannyHou left a comment

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

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

Conversation

hacksparrow commented May 9, 2019 • edited Loading

Related issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented May 27, 2019

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow May 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

hacksparrow commented Jun 3, 2019

hacksparrow commented Jun 3, 2019

b-admike left a comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

hacksparrow commented May 9, 2019 •

edited

Loading

hacksparrow May 30, 2019 •

edited

Loading