CUniqueValidator does not using table alias while creating query condition #114

Closed
qiangxue opened this Issue Feb 15, 2012 · 14 comments

Comments

5 participants
Owner

qiangxue commented Feb 15, 2012

What steps will reproduce the problem?

  1. I have created CActiveRecord model "User", which has the field "name"
  2. I have created relation to model "UserGroup", which also has field "name"
  3. I have made default scope, which always joins "UserGroup"
  4. I have set up validation rules:
    public function rules() {
    ...
    array('name','unique','className'=>'User','caseSensitive'=>false)
    }
  5. If I update the model "User" (NOT new record scenario), the exception raises:
    CDbCommand failed to execute the SQL statement: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'name' in where clause is ambiguous

What is the expected output? What do you see instead?

What version of the product are you using? On what operating system?

I am using framework version 1.1.6

Please provide any additional information below.

The source of the problem - method "CUniqueValidator::validateAttribute()". While creating search critea it does not use table alias:

$columnName=$column->rawName;
$criteria=new CDbCriteria(array(
'condition'=>$this->caseSensitive ? "$columnName=:value" : "LOWER($columnName)=LOWER(:value)",
'params'=>array(':value'=>$value),
));

code:
$object->getTableAlias(true)
is missing

Migrated from http://code.google.com/p/yii/issues/detail?id=2242


earlier comments

qiang.xue said, at 2011-03-26T17:58:17.000Z:

Out of time. Move to next milestone.

klimov.paul said, at 2011-04-29T09:10:55.000Z:

Here is the code of the method "CUniqueValidator::validateAttribute" that will fix this problem:

protected function validateAttribute($object,$attribute)
{
$value=$object->$attribute;
if($this->allowEmpty && $this->isEmpty($value))
return;

    $className=$this->className===null?get_class($object):Yii::import($this->className);
    $attributeName=$this->attributeName===null?$attribute:$this->attributeName;
    $finder=CActiveRecord::model($className);
    $table=$finder->getTableSchema();
    if(($column=$table->getColumn($attributeName))===null)
        throw new CException(Yii::t('yii','Table "{table}" does not have a column named "{column}".',
            array('{column}'=>$attributeName,'{table}'=>$table->name)));

    $columnName=$column->rawName;               
    $criteria=new CDbCriteria(array(
        // Modified by klimov.paul@gmail.com
        // Allow to use table alias in column check criteria:
        'condition'=>$this->caseSensitive ? $finder->getTableAlias(true).".{$columnName}=:value" : "LOWER(".$finder->getTableAlias(true).".{$columnName})=LOWER(:value)",
        'params'=>array(':value'=>$value),
    ));
    if($this->criteria!==array())
        $criteria->mergeWith($this->criteria);

    if(!$object instanceof CActiveRecord || $object->isNewRecord || $object->tableName()!==$finder->tableName())
        $exists=$finder->exists($criteria);
    else
    {
        $criteria->limit=2;
        $objects=$finder->findAll($criteria);
        $n=count($objects);
        if($n===1)
        {
            if($column->isPrimaryKey)  // primary key is modified and not unique
                $exists=$object->getOldPrimaryKey()!=$object->getPrimaryKey();
            else // non-primary key, need to exclude the current record based on PK
                $exists=$objects[0]->getPrimaryKey()!=$object->getOldPrimaryKey();
        }
        else
            $exists=$n>1;
    }

    if($exists)
    {
        $message=$this->message!==null?$this->message:Yii::t('yii','{attribute} "{value}" has already been taken.');
        $this->addError($object,$attribute,$message,array('{value}'=>$value));
    }
}

klimov.paul said, at 2011-06-29T08:11:32.000Z:

You have made a misjudgement merging this issue into the issue #2524. These ones report different problem, while it may look the same. I think issue #2488 also should not be merged into #2524, but can be merged with this issue.

You declare the issue #2524 is resolved with the revision r3267, which so framework version 1.1.8 includes. Well it seems the issue #2524 indeed has been resolved, but for the this issue (#2242) and issue #2488 I can see any changes.

I have updated framework to the version 1.1.8 and I can see this issue (#2242) is still unresolved. The same error appears at the same conditions.

Please read the original issue report and my comments carefully(!) once again. I have provided you all information you may need, even the source code to solve the problem.

qiang.xue said, at 2011-06-29T14:01:37.000Z:

Will check it again.

rupert@rabbitstyle.com said, at 2011-11-22T14:06:45.000Z:

I came across this problem too with the CExistValidator - I had a clash with ambiguity between id columns in different tables when I added my own criteria. Simply creating an extension of CExistValidator with '$finder->getTableAlias(true).".' before the column name in the condition solved it perfectly. Surely this should be pretty easy to add in to the validators - I can't see any issues that could arise from simply using the table alias to disambiguate (do correct me if I'm wrong).

qiang.xue said, at 2012-01-01T03:36:54.000Z:

set for 1.1.10 milestone

Owner

samdark commented May 22, 2012

@klimov-paul That is a bug. Can you work on the pull request for it? Thanks.

klimov-paul referenced this issue May 25, 2012

Closed

Issue #114 #750

Member

klimov-paul commented May 25, 2012

I have created pull request for this issue:

#750

Member

klimov-paul commented May 28, 2012

Pull request has been recreated:

#758

@klimov-paul klimov-paul added a commit to klimov-paul/yii that referenced this issue May 28, 2012

@klimov-paul klimov-paul CUniqueValidator and CExistValidator have been updated to use table a…
…lias while creating db query condition (fixes bug #114)
caa5148
Member

klimov-paul commented May 28, 2012

In my homeland we say "the lazy one do things twice"...
This one should be just fine:

#759

Once again I apologize

Owner

samdark commented May 28, 2012

@klimov-paul Thanks for not giving it up and doing proper pull request :)

samdark closed this May 28, 2012

Member

mdomba commented May 28, 2012

Yes, thanks @klimov-paul for this fix... You don't have to apologize for anything, we are all learning here :D

mdomba was assigned May 28, 2012

Contributor

resurtm commented Jun 21, 2012

Note that @klimov-paul's fix does not works properly if we're setting custom table alias in CExistValidator::$criteria.

Member

klimov-paul commented Jun 21, 2012

Indeed, there is a problem, if criteria, which is passed to validator, specifies its own table alias.
As much as there is a problem, when this criteria specifies query param named “:value”.
I am not sure if it is critical, however the following code can fix it:

$criteria=new CDbCriteria();
        if($this->criteria!==array())
            $criteria->mergeWith($this->criteria);

        $tableAlias = empty($criteria->alias) ? $finder->getTableAlias(true) : $criteria->alias;
        $valueParamName = ':value'.get_class($this);
        $criteria->addCondition($this->caseSensitive ? "{$tableAlias}.{$columnName}={$valueParamName}" : "LOWER({$tableAlias}.{$columnName})=LOWER({$valueParamName})");
        $criteria->params[$valueParamName] = $value;

mdomba reopened this Jun 21, 2012

Member

mdomba commented Jun 21, 2012

The same should be done for both validators not only CExistValidator.

How about something along this lines

if($this->criteria!==array())
{
    if empty($this->criteria->alias)
         $this->criteria->alias = $finder->getTableAlias(true);
}
Member

klimov-paul commented Jun 21, 2012

@mdomba, do not forget that “CExistValidator::criteria” can be either an array or an object.
I can not see other options except to create criteria instance and merge it with the “CExistValidator::criteria”.

Member

mdomba commented Jun 21, 2012

Right.

Just noticed that by description this can be only an array but it would work even as an object... we should update that in the description...

Member

klimov-paul commented Jun 22, 2012

I can create another pull request for this issue.
In this case should I use the same branch or a new one and should I update the changlog somehow?
No new issue has been created after all.

Member

mdomba commented Jun 22, 2012

No need to update the changelog it's always the same issue... not sure about branches... try to do it on the same one.

Member

klimov-paul commented Jun 25, 2012

This one should solve all problems here:

#877

mdomba closed this Jun 26, 2012

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