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.

Sign up for GitHub

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

Jump to bottom

Handle Empty Array for IN and NOT IN queries. #8035

Closed
wants to merge 1 commit into from
Closed

Handle Empty Array for IN and NOT IN queries. #8035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/Doctrine/ORM/Query/Expr/Func.php
View file
Open in desktop
Expand Up @@ -19,6 +19,8 @@

namespace Doctrine\ORM\Query\Expr;

use function count;

/**
* Expression class for generating DQL functions.
*
Expand Down Expand Up @@ -73,6 +75,10 @@ public function getArguments()
*/
public function __toString()
{
if (count($this->arguments) === 0) {
return $this->name . '(NULL)';
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this return all elements where the value is null for nullable columns? This seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@beberlei No,

Select foo from bar where foo IN (NULL)

Return always 0 row. I tried it multiple times.

foo IN (A, B) mean foo = A or foo = B
foo IN (NULL) mean foo = NULL
foo = NULL always returns no result, instead of foo IS NULL.

And as explained here: #8033 (comment), this is already the query created when using

$query->andWhere("$alias.foo IN (:bar)");
$query->setParameter('bar', []);

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK NULL is handled that way in all commonly known databases like e.g. PostgreSQL, MySQL, SQLite...

https://dbfiddle.uk/?rdbms=sqlite_3.8&fiddle=ac349cc3ca597516e307d690e7f19d90

Copy link
Member

@greg0ire greg0ire Feb 29, 2020 •
edited

Choose a reason for hiding this comment

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

I saw this nice comic on Twitter about this the other day :)

use x IS NULL, not x = NULL
https://t.co/bIJIHAJkGa

— 🔎Julia Evans🔍 (@b0rk) October 17, 2019

Copy link
Member

Choose a reason for hiding this comment

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

Wait so IN is considered a function 🤔 🤔 🤔 ? I mean it has a left operand…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I discovered 😅

Copy link
Member

Choose a reason for hiding this comment

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

This makes it a bit scary… touching such a generic method looks like it could have unforeseen repercussions.

}

return $this->name . '(' . implode(', ', $this->arguments) . ')';
}
}
10 changes: 10 additions & 0 deletions tests/Doctrine/Tests/ORM/Query/ExprTest.php
View file
Open in desktop
Expand Up @@ -282,6 +282,11 @@ public function testInLiteralExpr()
$this->assertEquals("u.type IN('foo', 'bar')", (string) $this->_expr->in('u.type', ['foo', 'bar']));
}

public function testInExprForEmptyArray()
{
self::assertEquals('u.id IN(NULL)', (string) $this->_expr->in('u.id', []));
}

public function testNotInExpr()
{
$this->assertEquals('u.id NOT IN(1, 2, 3)', (string) $this->_expr->notIn('u.id', [1, 2, 3]));
Expand All @@ -292,6 +297,11 @@ public function testNotInLiteralExpr()
$this->assertEquals("u.type NOT IN('foo', 'bar')", (string) $this->_expr->notIn('u.type', ['foo', 'bar']));
}

public function testNotInExprForEmptyArray()
{
self::assertEquals('u.id NOT IN(NULL)', (string) $this->_expr->notIn('u.id', []));
Copy link
Member

Choose a reason for hiding this comment

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

u.id NOT IN(NULL) would result in an empty result too.

https://dbfiddle.uk/?rdbms=sqlite_3.8&fiddle=0097a9450b4e56c142e64a480b745614

this contradicts with your expectations described in your issue #8033 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know.

It's still an improvement since

  • IN (NULL) works correclty
  • NOT IN (NULL) wont return an error, when NOT IN () does.

And in the next comment #8033 (comment), I explained that

$query->andWhere("foo.bar NOT IN (:baz)")->setParameter('baz', []);

Is actually translated as foo.bar NOT IN (NULL).

So my PR make the behaviour of $query->expr()->notIn() more consistent with $query->andWhere("foo.bar NOT IN (:baz)")->setParameter('baz', []);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can improve the behaviour of the NOT IN query with an empty array, but It'll need a lot a more code changes and some discussions, so I recommend to split this from this PR, which was only looking for more consistency.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to split it, then you'll have to remove the NOT IN changes. What do you want to change for supporting this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to split it, then you'll have to remove the NOT IN changes.

I disagree. Actually if you write

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', ':baz'))
$query->setParameter('baz', []);

There is no exception and this is translated as foo.bar NOT IN (NULL).

But if you write directly

$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', []))

You get an exception. But there is absolutely no reason to behave differently from the first case.

So it's still an improvement to have a consistent behaviour for notIn('foo.bar', ':baz')) and notIn('foo.bar', [])).

I improve both the IN case and the NOT IN case.
So I don't think I should remove the NOT IN changes.

I don't know what I have to change to completely supports the NOT IN case and if it's possible. It should handle correctly the case

$query->andWhere('foo.bar NOT IN :baz')
$query->setParameter('baz', []);

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', ':baz'))
$query->setParameter('baz', []);

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', []))

And this seems for me like a lot of work/rework.

Is this PR perfect and fixing everything ? No
Is this PR better than the actual situation ? Yes

}

public function testAndxOrxExpr()
{
$andExpr = $this->_expr->andX();
Expand Down