Unexpected ActiveQuery behavior - only one request to database #1545

Closed
SonicGD opened this Issue Dec 16, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@SonicGD
Contributor

SonicGD commented Dec 16, 2013

Hello. Today we faced unexpected AQ behavior. You can't call count() and then call all() or one(). If you do - Yii throws an exception.

$query = Model::find()->where(['param1'=>1]);
$query->count(); //ok
$query->all(); //exception, param not bounds

So, for example, in pagination, you should get query, clone it and use one copy for count and other for query data.

$query = Model::find()->where(['param1'=>1]);
$countQuery = clone $query;
$countQuery->count(); //ok
$query->all(); //ok

It's not expected at the first look. Is it a bug or it just need to be documented somewhere?

@ghost ghost assigned qiangxue Dec 16, 2013

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Dec 16, 2013

Member

Hm... not sure why it is not working imo it should be possible to run a once composed query in multiple variations.

Member

cebe commented Dec 16, 2013

Hm... not sure why it is not working imo it should be possible to run a once composed query in multiple variations.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Dec 17, 2013

Member

not sure why it is not working imo it should be possible to run a once composed query in multiple variations.

PDOStatement resets all bound params on the query execution, so on next attempt of query run you will see error “bound parameters missmatch”

Member

klimov-paul commented Dec 17, 2013

not sure why it is not working imo it should be possible to run a once composed query in multiple variations.

PDOStatement resets all bound params on the query execution, so on next attempt of query run you will see error “bound parameters missmatch”

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Dec 17, 2013

Member

yeah but we are creating a completely new Command when executing so I am wondering why this matters. Am I missing something?

Member

cebe commented Dec 17, 2013

yeah but we are creating a completely new Command when executing so I am wondering why this matters. Am I missing something?

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Dec 17, 2013

Member

but we are creating a completely new Command when executing so I am wondering why this matters.

Indeed, my mistake. Something else should be involved.

Member

klimov-paul commented Dec 17, 2013

but we are creating a completely new Command when executing so I am wondering why this matters.

Indeed, my mistake. Something else should be involved.

@SonicGD

This comment has been minimized.

Show comment
Hide comment
@SonicGD

SonicGD Dec 17, 2013

Contributor

I think problem is that ActiveQuery run QueryBuilder only if $query->sql is null

if ($this->sql === null) {
            if ($this->from === null) {
                $tableName = $modelClass::tableName();
                if ($this->select === null && !empty($this->join)) {
                    $this->select = ["$tableName.*"];
                }
                $this->from = [$tableName];
            }
            list ($this->sql, $params) = $db->getQueryBuilder()->build($this);
        }

But if we already run query, sql is not empty. And if params should be taken for $query->where - it never happens. And if we, for example, overload count() for ActiveQuery and set sql to null after command create - all works:

public function count($q = '*', $db = null)
    {
        $this->select = ["COUNT($q)"];
        $result = $this->createCommand($db)->queryScalar();
        $this->sql = null;
        return $result;
    }

But, it should be done every time we create command.

Contributor

SonicGD commented Dec 17, 2013

I think problem is that ActiveQuery run QueryBuilder only if $query->sql is null

if ($this->sql === null) {
            if ($this->from === null) {
                $tableName = $modelClass::tableName();
                if ($this->select === null && !empty($this->join)) {
                    $this->select = ["$tableName.*"];
                }
                $this->from = [$tableName];
            }
            list ($this->sql, $params) = $db->getQueryBuilder()->build($this);
        }

But if we already run query, sql is not empty. And if params should be taken for $query->where - it never happens. And if we, for example, overload count() for ActiveQuery and set sql to null after command create - all works:

public function count($q = '*', $db = null)
    {
        $this->select = ["COUNT($q)"];
        $result = $this->createCommand($db)->queryScalar();
        $this->sql = null;
        return $result;
    }

But, it should be done every time we create command.

@SonicGD

This comment has been minimized.

Show comment
Hide comment
@SonicGD

SonicGD Dec 17, 2013

Contributor

Or maybe we can simply unset it in createCommand:

$command= $db->createCommand($this->sql, $params);
        $this->sql = null;
return $command;

But it need to be tested.

Contributor

SonicGD commented Dec 17, 2013

Or maybe we can simply unset it in createCommand:

$command= $db->createCommand($this->sql, $params);
        $this->sql = null;
return $command;

But it need to be tested.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Dec 17, 2013

Member

Good catch! :-) will fix it.

Member

cebe commented Dec 17, 2013

Good catch! :-) will fix it.

@ghost ghost assigned cebe Dec 17, 2013

@cebe cebe closed this in 2febbeb Dec 17, 2013

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 17, 2013

Member

I think it's better we don't modify $this->params and $this->sql with the following code:

$params = $this->params;
$sql = $this->sql;
if ($sql === null) {
    ...
    list ($sql, $params) = $db->getQueryBuilder()->build($this);
}
return $db->createCommand($sql, $params);

Also, methods count() etc. need to be modified so that it restores select after the query. Otherwise, if you call count() first and then all(), you won't get expected results.

Member

qiangxue commented Dec 17, 2013

I think it's better we don't modify $this->params and $this->sql with the following code:

$params = $this->params;
$sql = $this->sql;
if ($sql === null) {
    ...
    list ($sql, $params) = $db->getQueryBuilder()->build($this);
}
return $db->createCommand($sql, $params);

Also, methods count() etc. need to be modified so that it restores select after the query. Otherwise, if you call count() first and then all(), you won't get expected results.

@cebe cebe reopened this Dec 17, 2013

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Dec 17, 2013

Member

yep. query is already reuseable in this way except that count changes it. Are you already working on it? If not I can do it.

Member

cebe commented Dec 17, 2013

yep. query is already reuseable in this way except that count changes it. Are you already working on it? If not I can do it.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 17, 2013

Member

Nope. You can work on this.

So the main idea is after calling createCommand(), we should keep sql and params unchanged.

Member

qiangxue commented Dec 17, 2013

Nope. You can work on this.

So the main idea is after calling createCommand(), we should keep sql and params unchanged.

cebe added a commit to cebe/yii2 that referenced this issue Dec 17, 2013

@qiangxue qiangxue closed this in #1560 Dec 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment