Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParseQuery::equalTo with strange behavior #476

Closed
igor-mobapps opened this issue May 29, 2021 · 5 comments · Fixed by #510
Closed

ParseQuery::equalTo with strange behavior #476

igor-mobapps opened this issue May 29, 2021 · 5 comments · Fixed by #510

Comments

@igor-mobapps
Copy link

Issue Description

Hello, I currently have a use case that requires making 2 conditions in the same field but with different values.

However, when using equalTo(field, value) and then notEqualTo(field, value) it returns a warning

Message: Warning: Illegal string offset '$ne'

This happens because when trying to add the $ne condition to the $this->where in the ParseQuery::addCondition($key, $condition, $value) method, the $this->where[$key] is a string, due to the ParseQuery::equalTo($key, $value) method adding the value directly to the $this->where array

equalto01

Steps to reproduce

$query = new ParseQuery('SomeClass');
$query->equalTo('docs.situation', 'pending');
$query->notEqualTo('docs.situation', 'rejected'); -> throw an warning and query break

Note: If the equalTo query is the last one to be called, it overrides the previous condition

$query = new ParseQuery('SomeClass');
$query->notEqualTo('docs.situation', 'rejected'); // is not added to the where stack
$query->lessThan('docs.situation', 'rejected'); // is not added to the where stack
$query->greaterThan('docs.situation', 'rejected'); // is not added to the where stack
$query->equalTo('docs.situation', 'pending');

Note: the code above is just a use case, of course the query itself doesn’t make much sense

Result of the query structure above

Parse\ParseQuery Object
(
    [className:Parse\ParseQuery:private] => Taxista
    [where:Parse\ParseQuery:private] => Array
        (
            [docs.situation] => pending
        )

    [orderBy:Parse\ParseQuery:private] => Array
        (
        )

    [includes:Parse\ParseQuery:private] => Array
        (
        )

    [excludes:Parse\ParseQuery:private] => Array
        (
        )

    [selectedKeys:Parse\ParseQuery:private] => Array
        (
        )

    [skip:Parse\ParseQuery:private] => 0
    [count:Parse\ParseQuery:private] => 
    [limit:Parse\ParseQuery:private] => -1
    [readPreference:Parse\ParseQuery:private] => 
    [includeReadPreference:Parse\ParseQuery:private] => 
    [subqueryReadPreference:Parse\ParseQuery:private] => 
)

Solution

Instead of using $this->where[$key] inside the ParseQuery::equalTo() method, use the $this->addCondition($key, '$eq', $value) method.

equalto02

I don't know if there is any special reason for using $this->where directly in the ParseQuery::equalTo() method instead of using $this->addCondition($key, '$eq', $value), but with aggregation the query listed above works normally.

With aggregate

// it works
$query = new ParseQuery('SomeClass');
$query->aggregate([
    'match' => [
        'docs.situation' => [
            '$eq' => 'pending',
            '$ne' => 'rejected'
        ]
    ]
]);

Environment Details

  • Your PHP Version: 7.4
  • Your Parse PHP SDK Version: 1.6
@dplewis
Copy link
Member

dplewis commented May 29, 2021

Can you write a failing test case?

The javascript SDK has similar behavior https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseQuery.js#L1216 (my Apple M1 chip is preventing me from running the tests, im looking into it)

@igor-mobapps
Copy link
Author

Hello @dplewis , I have no experience with open source contribution, in fact this is the first I do a PR for an open source project, anything just to report.

Below is the PR with the tests I did.

#477

I did a test that queries the database and is returning all 3, but it should have returned only 2.

And the other test is about what I related about equalTo overwriting the previous queries that use the same key.

Note: I can't do the test where the equalTo() is called before the notEqualTo, because it launches a Warning: Illegal string offset '$ne' and I can't find a solution for expect in phpunit 7

Note: I made the same query on the mongodb compass and the result is expected to be only 2

testObjectQuery01

@dplewis
Copy link
Member

dplewis commented Jun 8, 2021

A developer just posted a similar issue with the JS SDK. Does this exist there as well?

parse-community/Parse-SDK-JS#1373

@igor-mobapps
Copy link
Author

igor-mobapps commented Jun 8, 2021

yes, my friend from work when trying to do the same query I did in parse-php-sdk the query didn't work correctly, I don't know exactly the error due to lack of js expertise, but I know it didn't work properly.

JS SDK
https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseQuery.js#L1225

PHP SDK
https://github.com/parse-community/parse-php-sdk/blob/master/src/Parse/ParseQuery.php#L143

When looking at this piece of code, it looks a lot like the php SDK.

In PHP I know the error is because equalTo is assigning the value directly to the $where[$key] array instead of adding it to a $where[$key][$condition].

The main question is to understand why the equalTo method is not using the addCondition($key, $condition, $value) method, because when looking in other analytic SDKs, they are implementing equalTo in da likewise, without considering the use of the addCondition method.

@sadakchap
Copy link
Member

The main question is to understand why the equalTo method is not using the addCondition($key, $condition, $value) method, because when looking in other analytic SDKs, they are implementing equalTo in da likewise, without considering the use of the addCondition method.

Maybe because we want equalTo method to override previous. See this testcase.

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 a pull request may close this issue.

3 participants