Uh oh!
There was an error while loading. Please reload this page.
array_type_field_filter: Add new contains filter to search on Array type field - #460
Conversation
5a5d62f to
7566064Comparec3f9a69 to
1fcf79aCompare| }); | ||
| it('should support where filter for array type field', function(done) { | ||
| Post.dataSource.settings.allowExtendedOperators = true; |
There was a problem hiding this comment.
After adding this we do not need juggler related PR.
PFB PR Link:
loopbackio/loopback-datasource-juggler#1865
1fcf79a to
17f56b7Compare
bajtos
left a comment
There was a problem hiding this comment.
I like how it's possible to implement your desired feature inside the connector only 👍
I have few comments on implementation details, PTAL above.
Please add a new section to README to explain what extended operators are supported (e.g. @>) and what is needed to allow the app to use them (set allowExtendedOperators in model and/or datasource settings).
Uh oh!
There was an error while loading. Please reload this page.
| categories: {'@>': ['AA']}, | ||
| }, | ||
| ]}}, function(err, post) { | ||
| should.not.exist(err); |
There was a problem hiding this comment.
| should.not.exist(err); | |
| if(err)returndone(err); |
That way the original error details and the stack trace is preserved.
There was a problem hiding this comment.
How we would be handling this with async await type test case? @bajtos
I was thinking to write test case this way for handling failure of testcase with proper details.
it('should support where filter for array type field',async()=>{Post.dataSource.settings.allowExtendedOperators=true;try{constpost=awaitPost.find({where: {categories: {'contains': ['AA']},}});should.exist(post);post.length.should.equal(1);}catch(err){should.not.exist(err);}});Please tell me if it is correct, I will update current test case in this way only.
There was a problem hiding this comment.
As a rule of thumb, the tests should pass any unexpected error to the test runner:
- Use
done(err)when using callback - Use regular error propagation when using async functions
it('should support where filter for array type field',async()=>{Post.dataSource.settings.allowExtendedOperators=true;constpost=awaitPost.find({where: {categories: {'contains': ['AA']},}});should.exist(post);post.length.should.equal(1);});Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
17f56b7 to
ecf676cCompareThere was a problem hiding this comment.
The new version looks pretty good.
Please change the commit message to use the new operator name
Please add a new section to README to explain what extended operators are supported (e.g.
@>) and what is needed to allow the app to use them (setallowExtendedOperatorsin model and/or datasource settings). Put yourself in the shoes of other LoopBack users - where would you look for information aboutcontainsoperator and what info would you want to see to help you understand how to use this feature in your app?
ecf676c to
5fc5d1eCompare@bajtos Thanks for review.. For Point#1 I have done the update.
|
bajtos
commented
Sep 21, 2020
That's a good question. I believe the postgres connector so far supports only built-in LoopBack operators supported by all (or most?) connectors, so there was no need to explicitly list extended operators. Now that you are implementing the first extended operator, I think you should simply add a new section. You can draw inspiration from MongoDB connector README:
In the case of PostgreSQL and your new |
@bajtos Added the readme section for the new contains operator, Can you review it... |
Implement support for an extended operator `contains` that can be used to filter records that have an array property containing the selected items. Signed-off-by: shubhisood <shubhi.sood@sourcefuse.com>
d8be8fc to
55c1003Comparebajtos
commented
Oct 6, 2020
Thank you @shubhisood for the update. I pushed few more commits to fix the failing builds and improve README content. Let's wait for CI results before landing. |
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Array properties are represented as juggler `List` instance, we need to modify test assertions to convert them via `toArray()` before applying `.should.eql` check. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
55c1003 to
8f2da81Comparebajtos
commented
Oct 6, 2020
@slnode test please |
bajtos
commented
Oct 6, 2020
@slnode test please |
bajtos
commented
Oct 6, 2020
Landed, thank you @shubhisood for the contribution ❤️ |
shubhisood
commented
Oct 6, 2020
Thanks for merging @bajtos |
Include references to all related GitHub issues and other pull requests, for example:
Fixes#342
Checklist
npm testpasses on your machine