loopbackio / loopback-connector-elastic-search

Strongloop Loopback connector for Elasticsearch
MIT License
79 stars 55 forks source link

OR inside AND not working correctly. #93

Closed AhsanAyaz closed 7 years ago

AhsanAyaz commented 7 years ago

I have a loopback query like:

{
    "where": {
        "and": [{
            "type": "email",
            "enterprise_id": "897cxzasdjhjhaz897",
            "or": [{
                "fromUserId": "12321312n3nkb"
            }, {
                "toUserIds": "89789z7xc8z7xc87sad"
            }]
        }]
    }
}

This actually creates a query :

"query": {
      "bool": {
        "must": [
          {
            "match": {
              "type": "email"
            }
          },
          {
            "match": {
              "enterprise_id": "897cxzasdjhjhaz897"
            }
          },
          [      //THIS seems wrong in this case
            {
              "match": {
                "fromUserId": "12321312n3nkb"
              }
            },
            {
              "match": {
                   "toUserIds": "89789z7xc8z7xc87sad"
              }
            }
          ]
        ]
      }

Since it is an AND and inside that there's an OR which is not at root level so what the following code of library does is that it appends the array of OR items inside the AND without bool and should and therefore returns an empty array:

else if (key === 'or' && Array.isArray(value)) {
            var orPath;
            if (root) {
                orPath = path.bool.should;
            }
            else {
                var orObject = {bool: {should: []}};
                orPath = orObject.bool.should;
                path.push(orPath);
            }
            cond.map(function (c) {
                log('ESConnector.prototype.buildDeepNestedQueries', 'mapped', 'body', JSON.stringify(body, null, 0));
                self.buildDeepNestedQueries(false, idName, c, body, orPath);
            });
        }

So i've changed that inside my code to

else {
                var orObject = {bool: {should: []}};
                orPath = orObject.bool.should;
                path.push(orObject);
            }

Which generates correct query :

"query": {
      "bool": {
        "must": [
          {
            "match": {
              "type": "email"
            }
          },
          {
            "match": {
              "enterprise_id": "897cxzasdjhjhaz897"
            }
          },
          {
            "bool": {
              "should": [
                {
                  "match": {
                    "fromUserId": "12321312n3nkb"
                  }
                },
                {
                  "match": {
                    "toUserIds": "89789z7xc8z7xc87sad"
                  }
                }
              ]
            }
          }
        ]
      }
    }

The above query returns the data perfectly.

My question is Is this the right way to resolve this issue? I'm assuming that if orPath was used, it would've been used for some reason. But in the case of AND, the andObject is used and not the andPath.

Any suggestions? If that is the right way of doing it what I did, i'll create a PR then.

pulkitsinghal commented 7 years ago

wow totally missed this until now at least now i know there's something to do maybe we'll look at it within next 30 days cc @aquid

aquid commented 7 years ago

@AhsanAyaz While this is really a bug and thank you for reporting this issue, but your query seems a bit confusing to me, can you improve it with something like this(just suggesting where section) ?

"where": {
    "and": [
      {
        "type": "email"
      }
      {
        "enterprise_id": "897cxzasdjhjhaz897"
      },
      {
        "or": [
          {
            "fromUserId": "12321312n3nkb"
          },
          {
            "toUserIds": "89789z7xc8z7xc87sad"
          }
        ]
      }
    ]
  }

OR maybe like this

"where": {
    "or": [
      {
        "type": "email",
        "enterprise_id": "897cxzasdjhjhaz897",
        "fromUserId": "12321312n3nkb"
      },
      {
        "type": "email",
        "enterprise_id": "897cxzasdjhjhaz897",
        "toUserIds": "89789z7xc8z7xc87sad"
      }
    ]
  }

Maybe this is not useful to you, I just wanted to display other possibilities or understand any specific reason for this query format.

I will soon look into your PR but before that can you add some test cases too? That will make sure that we don't mess it again with coming updates.

pulkitsinghal commented 7 years ago

It is day 30, i think ... doing a shallow review.

AhsanAyaz commented 7 years ago

@pulkitsinghal Thank you for reviewing. Here's the updated latest PR with test cases required. I had to use the plugin with the fixes for a complex webapp so I published a temporary package under my namespace @ahsanayaz and using it for now.

It'll be really helpful if this can be reviewed and merged :)

Thanks 👍

pulkitsinghal commented 7 years ago

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