Skip to content

Honour allowExtendedOperators in "DAO.find"#1180

Merged
bajtos merged 1 commit into
masterfrom
feature/extended-operators-in-query
Dec 8, 2016
Merged

Honour allowExtendedOperators in "DAO.find"#1180
bajtos merged 1 commit into
masterfrom
feature/extended-operators-in-query

Conversation

@bajtos

@bajtos bajtos commented Dec 5, 2016

Copy link
Copy Markdown
Member

Modify the coercion of filter.where to hounour "allowExtendedOperators" and don't coerce property values of type object (extended operators). This allows MongoDB users to query using MongoDBs operators like { $exists: true, $ne: null }.

See strongloop/loopback#646

@raymondfeng @superkhau or @Amir-61 please review

@fabien IIRC, you are using MongoDB quite a lot. Could you please take a look too?

@bajtos

bajtos commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

The first commit is from #1179, I'll rebase it away.

@bajtos bajtos force-pushed the feature/extended-operators-in-query branch from f0f199b to d861b04 Compare December 5, 2016 15:08

@superkhau superkhau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks, LGTM.

Comment thread test/allow-extended-operators.test.js Outdated
// we have to build a new object for each test
return {where: {value: {$exists: true}}};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty space

Comment thread test/allow-extended-operators.test.js Outdated
};
callback(null, [instanceFound]);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these declared outside describe and extendedQuery is inside? Put all inside or put all outside for consistency?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree... It seems reasonable to have consistency...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extendedQuery is specific to DAO.find test cases only.

The rest is more generic and can be used in the future e.g. for DAO.updateAttributes tests that are missing now (the implementation is already in place though).

I moved them from the root scope into the describe block for consistency.

@superkhau superkhau assigned bajtos and unassigned superkhau Dec 5, 2016

@Amir-61 Amir-61 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor fixes.

Comment thread test/allow-extended-operators.test.js Outdated
};
callback(null, [instanceFound]);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree... It seems reasonable to have consistency...

@Amir-61

Amir-61 commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

@bajtos
Nice...Happy ES6 coding 🎊 Could you please let us know what is the plan regarding ES6? Are we going to write tests in ES6 from now on?

Modify the coercion of filter.where to hounour "allowExtendedOperators"
and don't coerce property values of type object (extended operators).
@bajtos bajtos force-pushed the feature/extended-operators-in-query branch from d861b04 to 779cf6a Compare December 6, 2016 09:28
@bajtos

bajtos commented Dec 6, 2016

Copy link
Copy Markdown
Member Author

Nice...Happy ES6 coding 🎊 Could you please let us know what is the plan regarding ES6? Are we going to write tests in ES6 from now on?

@Amir-61 Yes, we can and should use ES6 in master branches now that we no longer support Node v0.10/v0.12. See loopbackio/loopback.io#187 and http://loopback.io/doc/en/contrib/style-guide.html

Note that when back-porting changes to older versions (typically 1.x and 2.x), we should stick to ES5, to make sure we don't break LoopBack for people still running on pre-v4 versions of Node.

@bajtos bajtos merged commit 29c60c7 into master Dec 8, 2016
@bajtos bajtos deleted the feature/extended-operators-in-query branch December 8, 2016 08:56
@bajtos bajtos removed the #review label Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants