loopbackio / loopback-connector-elastic-search

Strongloop Loopback connector for Elasticsearch
MIT License
78 stars 56 forks source link

[BUG] when `and` was nested inside `or`, the ES query was built incorrectly #99

Closed AhsanAyaz closed 7 years ago

AhsanAyaz commented 7 years ago

Fixed the query issue of OR inside and AND for where filter. Wrote the filter test for above case since it was missing. Added support for querying phrases. Wrote a filter test for that too.

slnode commented 7 years ago

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

AhsanAyaz commented 7 years ago

Fixes #93

AhsanAyaz commented 7 years ago

Did anyone get a chance to review the PR?

AhsanAyaz commented 7 years ago

@pulkitsinghal I've rebased my PR with your repository so there shouldn't be any conflicts left.

Thanks.

pulkitsinghal commented 7 years ago

@pulkitsinghal I've rebased my PR with your repository so there shouldn't be any conflicts left.

Thanks @AhsanAyaz, I appreciate your help! I didn't notice your comment until just now so in the meantime I used the manual approach:

git checkout -b AhsanAyaz-develop develop
git pull git://github.com/AhsanAyaz/loopback-connector-elastic-search.git develop

to do the same locally.

Here are the changes I made:

  1. Removed the phrase search support because that's what native is for:
  2. Removed the mix of tabs and whitespaces then fixed the indentations
  3. Ran the test against ES version 1.x:
    docker-compose -f docker-compose-for-testing-v1.yml up
    npm run testv1
  4. Now planning to copy over the test for all versions, which ... yes, is a bit wasteful ... until I become clever about managing tests.
pulkitsinghal commented 7 years ago

https://github.com/strongloop-community/loopback-connector-elastic-search/pull/100 Has been merged into develop but hasn't been released.

@AhsanAyaz - do you want to take this chance to close this specific PR and then start over from develop for only the phrase search support (will require some discussion therefore delays) or are ok with using native?

I can plan to release with or without phrase search support as the tests are passing. Let me know.

AhsanAyaz commented 7 years ago

@pulkitsinghal I think you're right with the native approach. And since it is pretty dynamic, we won't have to put/plug too many of them. I'll modify the approach in my application with native. You can proceed without phrase search support.

Thanks.

AhsanAyaz commented 7 years ago

@pulkitsinghal I'm closing this PR as the one without phrase search support has already been merged. Do let me know your plan to have a release. Thanks 👍

slnode commented 7 years ago

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

pulkitsinghal commented 7 years ago

fixed and released in v1.4.1 - Jun 6, 2017