dao: should not convert the val type if object by yorkie · Pull Request #711 · loopbackio/loopback-datasource-juggler

yorkie · 2015-09-02T14:32:10Z

If the value is an object, maybe we should not convert it to corresponding type like supporting:

{ '$exists': true }

/cc @raymondfeng

slnode · 2015-09-02T14:32:11Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

raymondfeng · 2015-09-02T20:07:29Z

@yorkie Thank you for the patch. Can you provide a test case to demonstrate what issue you are trying to solve? We'll include the test when the PR is merged.

yorkie · 2015-09-03T03:34:35Z

Hey @raymondfeng I'm fixing the same issue at strongloop/loopback#646, because I was occurring as well, and this patch should just provide a ability to use MongoDB's any $ operator without supporting by loopback.

If you think this is valuable, I will add the test case :)

yorkie · 2016-05-03T04:07:45Z

Ping @raymondfeng :-)

slnode · 2016-05-03T04:07:47Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

superkhau · 2016-05-03T20:31:57Z

@yorkie Thank you for the patch. Can you provide a test case to demonstrate what issue you are trying to solve? We'll include the test when the PR is merged.

Can you add a test case please?

superkhau · 2016-05-03T20:32:57Z

        } else if (operator === 'regexp' && val instanceof RegExp) {
          // Do not coerce regex literals/objects
-        } else if (!((operator === 'like' || operator === 'nlike') && val instanceof RegExp)) {
+        } else if (!((operator === 'like' || operator === 'nlike') && val instanceof Object)) {


I think we should add another || for your case. Why is the RegExp check being changed?

As I have said in description, current loopback's where filter doesn't support complicated query which contains something mongoDB operators in an object.

That part I understand. But why not .. && (val instanceof RegExp || val instanceof Object)) {?

But I guess the RegExp is also an Object?

Yes it is, but you hide the fact that you're checking for RegExp explicitly when you combine them together.

Okay, that makes sense.

Do you want to make that change and then we can get it merged in?

Can you also write a unit test to verify your changes and prevent regressions in the future?

Ok, I will update nits and add test case ASAP :-)

TY, ping me again after and I will get it merged in ASAP. ;)

yorkie · 2016-05-07T10:00:43Z

@superkhau I have added the test case and fix nits, could you please have a review again? Thank you :-)

superkhau · 2016-05-08T23:52:11Z

Can you run the tests through the linter? I see some stylistic issues that will probably cause linting to fail.

npm test

superkhau · 2016-05-08T23:52:24Z

Can you also squash the commits?

yorkie · 2016-05-09T05:00:02Z

Hey @superkhau the npm test seems not include the lint check?

yorkie · 2016-05-09T05:17:17Z

Done the squash :-)

superkhau · 2016-05-09T19:21:29Z

I just looked, it does, but still uses makefiles (which we are in the process of removing). However the lint code is there, so you should be able to run npm run lint manually. ;) Please try that instead.

bajtos · 2016-05-10T12:11:08Z

ability to use MongoDB's any $ operator without supporting by loopback.

FWIW, we already support MongoDB's $ operators in DAO.updateAttributes, one has to enable that feature via model-level setting allowExtendedOperators. Perhaps we should do the same here and enable this feature only when the flag is enabled?

yorkie · 2016-05-10T12:16:01Z

@bajtos do you mean check allowExtendedOperators here?

bajtos · 2016-05-10T12:23:19Z

do you mean check allowExtendedOperators here?

Yes, that's correct, allow MongoDB-like object values in query only when allowExtendedOperators is turned on. Sorry for not being more clear!

However, I am not sure if it's the right thing to do. I'd rather leave it up to you, @raymondfeng and @superkhau to decide.

yorkie · 2016-05-10T12:24:43Z

Okay, I will fix the lint warnings pointed by @superkhau firstly :-)

yorkie · 2016-05-10T12:36:35Z

Fixed and did rebase @superkhau :-)

superkhau · 2016-05-10T18:51:57Z

@raymondfeng Input on the above comments?

superkhau · 2016-05-16T23:04:54Z

Note, I spoke to @raymondfeng regarding the flag, we want it implemented to work with allowExtendedOperators.

yorkie · 2016-05-17T05:13:52Z

Ok, got it, I will do it soon later :-)

yorkie · 2016-05-18T05:38:10Z

Hey @superkhau @raymondfeng Do you have something documentation about the config allowExtendedOperators?

superkhau · 2016-05-18T16:40:29Z

@crandmck ^

crandmck · 2016-05-18T18:32:31Z

Do you have something documentation about the config allowExtendedOperators?

What we have is just for Mongo: https://docs.strongloop.com/display/APIC/MongoDB+connector#MongoDBconnector-UsingMongoDBoperatorsinupdateoperations

IIUC, this change means you can use it with other data sources--right? If so, which ones?
And is the config the same in <modelname>.json ?

superkhau · 2016-05-18T19:01:58Z

IIUC, this change means you can use it with other data sources--right? If so, which ones?

Not sure about this one (I believe it is for all connectors since we are modifying DAO), but @bajtos or @raymondfeng can confirm this.

And is the config the same in .json ?

Yes, the config is the same.

crandmck · 2016-05-18T19:53:30Z

If it's only Mongo, then what does this add over what's already documented?

superkhau · 2016-05-18T22:13:31Z

-        u.order.should.equal(1);
-        u.name.should.equal('Paul McCartney');
+      User.findOne({ order: 'order' }, function(err, user) {
+        should.not.exist(err);


We should change this to if (err) return done(err) for better error output.

superkhau · 2016-05-18T23:50:22Z

@yorkie is adding a feature that will only turn on if allowExtendedOperators is set to true in the config, so it doesn't really depend on the connector.

If it's only Mongo, then what does this add over what's already documented?

It's a change to DAO so it will affect all connectors.

superkhau · 2016-05-20T00:32:17Z

@bajtos Reassigning this one to you as I'll be gone for a few weeks. It's pretty close to done.

bajtos · 2016-05-20T12:54:32Z

+        User.findOne(function(err, user) {
+          should.not.exist(err);
+          should.exist(user);
+          user.id.toString().should.equal(users[0].id.toString());


IIUC, this change is cleaning up the code by renaming e, u to err, user. While I see a lot of value in clean code, I see much more value in git commit history that makes code archaeology easy. Changes like these makes examination of history more difficult, as they add noise.

If you think this is still a change worth making, then please put it in a standalone commit, possibly in a new pull request.

I asked @yorkie to change it to err/user earlier because I didn't realize it was already named e/u. My bad! @yorkie Can you change it back to so these irrelevant lines aren't part of this commit?

bajtos · 2016-12-05T15:08:52Z

Closing in favour of #1180

slnode · 2016-12-05T15:08:53Z

Can one of the admins verify this patch?

slnode · 2016-12-05T15:08:54Z

Can one of the admins verify this patch?

slnode · 2016-12-05T15:08:54Z

Can one of the admins verify this patch?

altsang added the #community contribution label Sep 2, 2015

bajtos assigned raymondfeng Oct 9, 2015

superkhau reviewed May 3, 2016
View reviewed changes

superkhau assigned superkhau and unassigned raymondfeng May 6, 2016

yorkie force-pushed the patch-1 branch from e96cd3f to ecd618b Compare May 9, 2016 05:17

superkhau mentioned this pull request May 9, 2016

Use mocha instead of Makefile for testing #924

Merged

superkhau added #review and removed #community contribution labels May 9, 2016

yorkie force-pushed the patch-1 branch from ecd618b to ef6dabc Compare May 10, 2016 12:36

superkhau reviewed May 18, 2016
View reviewed changes

superkhau assigned bajtos and unassigned superkhau May 20, 2016

bajtos reviewed May 20, 2016
View reviewed changes

0candy added #community contribution and removed #wip labels Jun 8, 2016

Amir-61 added the juggler-DAO label Jul 29, 2016

Amir-61 added the bug label Aug 22, 2016

bajtos added feature and removed bug labels Sep 6, 2016

bajtos closed this Dec 5, 2016

bajtos removed the #community contribution label Dec 5, 2016

Uh oh!

Conversation

yorkie commented Sep 2, 2015 • edited by 0candy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slnode commented Sep 2, 2015

Uh oh!

raymondfeng commented Sep 2, 2015

Uh oh!

yorkie commented Sep 3, 2015

Uh oh!

yorkie commented May 3, 2016

Uh oh!

slnode commented May 3, 2016

Uh oh!

superkhau commented May 3, 2016

Uh oh!

superkhau May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorkie commented May 7, 2016

Uh oh!

superkhau commented May 8, 2016

Uh oh!

superkhau commented May 8, 2016

Uh oh!

yorkie commented May 9, 2016

Uh oh!

yorkie commented May 9, 2016

Uh oh!

superkhau commented May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented May 10, 2016

Uh oh!

yorkie commented May 10, 2016

Uh oh!

bajtos commented May 10, 2016

Uh oh!

yorkie commented May 10, 2016

Uh oh!

yorkie commented May 10, 2016

Uh oh!

superkhau commented May 10, 2016

Uh oh!

superkhau commented May 16, 2016

Uh oh!

yorkie commented May 17, 2016

Uh oh!

yorkie commented May 18, 2016

Uh oh!

superkhau commented May 18, 2016

Uh oh!

crandmck commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crandmck commented May 18, 2016

Uh oh!

yorkie commented Sep 2, 2015 •

edited by 0candy

Loading

superkhau May 3, 2016 •

edited

Loading

superkhau commented May 9, 2016 •

edited

Loading

crandmck commented May 18, 2016 •

edited

Loading

superkhau commented May 18, 2016 •

edited

Loading