DDC-284: Function 'in' of class Expr does not work with an array of strings as parameter #3593

Closed
doctrinebot opened this Issue Jan 28, 2010 · 18 comments

1 participant

@doctrinebot

Jira issue originally created by user ubaltzer:

Function in of Expr seems not to work properly.
I tried to fetch a result using a query with the function in() of Expr as described in the doctirne 2.0 documentation (querying an object Offer that contains a column named 'state' of type string)):

$qb->select('ofr')
->from('Offer', 'ofr')
->where($qb->expr()->in('ofr.state', array('test','second value','third value')));
$query = $qb->getQuery();
$result = $query->getResult();

Then the following error occurs:
[Syntax Error] line 0, col 139: Error: Expected Literal, got 'test'
in ...Doctrine/ORM/Query/QueryException.php, on line 41

Stack:
Doctrine\ORM\Query\QueryException::syntaxError( string ) in ...Doctrine/ORM/Query/Parser.php on line 346
Doctrine\ORM\Query\Parser->syntaxError( string ) in ...Doctrine/ORM/Query/Parser.php on line 2032
Doctrine\ORM\Query\Parser->Literal( ) in ...Doctrine/ORM/Query/Parser.php on line 2047
Doctrine\ORM\Query\Parser->InParameter( ) in ...Doctrine/ORM/Query/Parser.php on line 2423
Doctrine\ORM\Query\Parser->InExpression( ) in ...Doctrine/ORM/Query/Parser.php on line 1928
Doctrine\ORM\Query\Parser->SimpleConditionalExpression( ) in ...Doctrine/ORM/Query/Parser.php on line 1858
Doctrine\ORM\Query\Parser->ConditionalPrimary( ) in ...Doctrine/ORM/Query/Parser.php on line 1815
Doctrine\ORM\Query\Parser->ConditionalFactor( ) in ...Doctrine/ORM/Query/Parser.php on line 1791
Doctrine\ORM\Query\Parser->ConditionalTerm( ) in ...Doctrine/ORM/Query/Parser.php on line 1773
Doctrine\ORM\Query\Parser->ConditionalExpression( ) in ...Doctrine/ORM/Query/Parser.php on line 1854
Doctrine\ORM\Query\Parser->ConditionalPrimary( ) in ...Doctrine/ORM/Query/Parser.php on line 1815
Doctrine\ORM\Query\Parser->ConditionalFactor( ) in ...Doctrine/ORM/Query/Parser.php on line 1795
Doctrine\ORM\Query\Parser->ConditionalTerm( ) in ...Doctrine/ORM/Query/Parser.php on line 1773
Doctrine\ORM\Query\Parser->ConditionalExpression( ) in ...Doctrine/ORM/Query/Parser.php on line 1273
Doctrine\ORM\Query\Parser->WhereClause( ) in ...Doctrine/ORM/Query/Parser.php on line 810
Doctrine\ORM\Query\Parser->SelectStatement( ) in ...Doctrine/ORM/Query/Parser.php on line 780
Doctrine\ORM\Query\Parser->QueryLanguage( ) in ...Doctrine/ORM/Query/Parser.php on line 275
Doctrine\ORM\Query\Parser->parse( ) in ...Doctrine/ORM/Query.php on line 159
Doctrine\ORM\Query->_parse( ) in ...Doctrine/ORM/Query.php on line 193
Doctrine\ORM\Query->_doExecute( array ) in ...Doctrine/ORM/AbstractQuery.php on line 511
Doctrine\ORM\AbstractQuery->execute( array, integer ) in ...Doctrine/ORM/AbstractQuery.php on line 349
Doctrine\ORM\AbstractQuery->getResult( )

@doctrinebot

Comment created by @guilhermeblanco:

You have to use literal values.

Example:

$qb->where($qb>expr()->in('ofr.state', array(
    $qb->expr()->literal('test'),
    $qb->expr()->literal('second value'),
    $qb->expr()->literal('third value')
)));

Marking ticket as invalid.

@doctrinebot

Comment created by romanb:

It doesnt make sense to me that ->where($qb->expr()->in('u.id', array(1, 2, 3))) works and with strings it doesnt. Both are literals.

@doctrinebot

Comment created by romanb:

I guess you can use:

->where($qb>expr()->in('ofr.state', array("'test'","'second value'", "'third value'")));

Note the single quotes around the values. in DQL string literals must be quoted in single quotes.

@doctrinebot

Comment created by @beberlei:

doesn't the dql parser know what type of value this is and how it should be quoted? I am all for convenience for IN(), its a very important construct and often used

@doctrinebot

Comment created by romanb:

@Benjamin: If it's not in single-quotes, its not a string literal per definition of the grammar :) The DQL you get for:

->where($qb>expr()->in('ofr.state', array('test','second value','third value')));

is:

... WHERE ofr.state IN(test, second value, third value)

Thus the error:

Expected Literal, got 'test'

when it encounters test where a literal (a string in single quotes, an integer, ...) is expected.

@doctrinebot

Comment created by @guilhermeblanco:

I'm looking into a possible fix for this without affecting too much in our structure, since IN should also supports subqueries.

I think the best solution is to implement a Literal class, since it won't affect if user passes the construction as I sent.
A simple class check would solve it smoothly.

Working on a patch for it. Should be committed soon.

@doctrinebot

Comment created by romanb:

@Guilherme:

->where($qb>expr()->in('ofr.state', array("'test'","'second value'", "'third value'")));

is fine, dont you think? Doesnt that work? Dont make something complex if this works! :)

@doctrinebot

Comment created by @guilhermeblanco:

@romanb: It works, but it should NOT be done like that.

It should either be done automatically or make it strict as it's currently (called $qb->expr()->literal('foo')).

Since it's SO common this usage (array of values), I'm thinking to support auto-conversion natively. I always thought Literals SHOULD be a class... I even spoke with you about it when building the DQL parser. =\

@doctrinebot

Comment created by romanb:

Think carefully about that. I didnt do that in the parser for performance reasons.

Do you really want to wrap every single literal value in an object? ->in('a.bla', $arrayWith50Integers) = 50 objects?

I am seeing overkill.

So please, please consider this.

@doctrinebot

Comment created by romanb:

What you want to do is providing some minor convenience at the cost of an order of magnitude higher memory consumption and slower performance.

Just compare 50 ints vs 50 objects holding ints in a microbenchmark performance & memory-wise.

I am already regretting reopening this!

@doctrinebot

Comment created by @guilhermeblanco:

@romanb: Ok... so let's consider a quote comparison.

If it's already wrapped in quotes, skip the literal call. Otherwise, apply individually.
So.. what would perform better? What do you think? By doing this, it would require to iterate through all items and do a string check (something like str[0] == "'" && str[strlen(str) - 1] == "'" OR via a regex (which I think would not perform better, need to check)) and if it doesn't contain, apply the literal call.

If you have any better idea, feel free. =)

@doctrinebot

Comment created by romanb:

Ok, how about this:

    public function in($x, $y)
    {
        if (is_array($y)) {
            foreach ($y as &$literal) {
                $literal = $this->literal($literal);
            }
        }
        return new Expr\Func($x . ' IN', (array) $y);
    }


    public function literal($literal)
    {
        if (is_numeric($literal)) {
            return (string) $literal;
        } else {
            return "'" . str_replace("'", "''", $literal) . "'"; // quote ' in the literal
        }
    }

The literal fix is needed either way, I think, otherwise single quotes in strings are misinterpreted.

@doctrinebot

Comment created by romanb:

This works perfect for me in my local copy. Here is a test I wrote:

    public function testWhereInWithStringLiterals()
    {
        $qb = $this->_em->createQueryBuilder();
        $qb->select('u')
           ->from('Doctrine\Tests\Models\CMS\CmsUser', 'u')
           ->where($qb->expr()->in('u.name', array('one', 'two', 'three')));

        $this->assertValidQueryBuilder($qb, "SELECT u FROM Doctrine\Tests\Models\CMS\CmsUser u WHERE u.name IN('one', 'two', 'three')");

        $qb->where($qb->expr()->in('u.name', array("O'Reilly", "O'Neil", 'Smith')));

        $this->assertValidQueryBuilder($qb, "SELECT u FROM Doctrine\Tests\Models\CMS\CmsUser u WHERE u.name IN('O''Reilly', 'O''Neil', 'Smith')");
    }

And subqueries still work, too.

@doctrinebot

Comment created by @guilhermeblanco:

@romanb that would work, of course.

BUT... It would break BC in the case I do something like:

$qb->where($qb>expr()->in('ofr.state', array(
    $qb->expr()->literal('test'),
    $qb->expr()->literal('second value'),
    $qb->expr()->literal('third value')
)));

That would be the same as array("'test'", "'second value'", "'third value'"), which would double the quote and give even more headaches.
I think we can consider this BC break now... what do you think?

@doctrinebot

Comment created by romanb:

@Guilherme: Good point. That is a problem, because it is not correct.

After all, I start to think what we have right now is still best (literals should always be passed through ->literal()). Literals in queries are not that frequent anyway, since you mostly use bound parameters.

Need to sleep over this.

@doctrinebot

Comment created by @guilhermeblanco:

@romanb I think since we're in the last alpha, it's still acceptable that we break BC...

I disagree with you that it's not used frequently. I think the opposite! While we still do not support IN ?, we're still struggled in the expr IN support, which is the exactly issue we're facing here.

SO... We need to agree if we break BC or of we add overhead by adding a Literal class.
Which one you think it'll be the best one to be merged?

I'll merge one of them... just need some thoughts from you. =)

Cheers,

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by julienferment:

Thank you all for your advices.
This works for me :

$string = "'string1','string2','string3'";

->add('where',$qb->expr()->in('d.reference', $string))

As you said, each string should be surrounded by single quotes.

Julien

@doctrinebot doctrinebot added this to the 2.0-BETA1 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
@doctrinebot doctrinebot added the Bug label Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment