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

Uniqueness validator succeeds on UPDATE with non-unique values if model has columnMap #13398

Closed
JetA2 opened this Issue Jun 4, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@JetA2
Copy link

JetA2 commented Jun 4, 2018

When a model uses a Uniqueness validator, the validation succeeds for non-unique values when an UPDATE is performed and the model uses a columnMap.

I believe this is due to the validator not using the mapped column name when creating the part of the query that excludes the current object from validation. See the line of code below:

let params["bind"][] = record->readAttribute(primaryField);

Sample code to reproduce:

<?php

$di = new Phalcon\DI\FactoryDefault();

$di->setShared('db', function () {
    return new \Phalcon\Db\Adapter\PDO\Sqlite(array(
        'dbname'   => ':memory:'
    ));
});

class Test extends \Phalcon\Mvc\Model
{
    public $mappedPrimaryKey;
    public $mappedData;

    public function columnMap()
    {
        // Keys are the real names in the table and
        // the values their names in the application
        //
        return array(
            "primaryKey"    => "mappedPrimaryKey",  
            "data"          => "mappedData"
        );
    }

    public function validation()
    {
        $validator = new Phalcon\Validation();          

            $validator->add( "mappedData",
                new Phalcon\Validation\Validator\Uniqueness( array(
                    "message" => "Data must be unique"
         )));

        return $this->validate( $validator );
    }
}

use \Phalcon\Db\Column as Column;

$di['db']->createTable(
    'test',
    null,
    [
       'columns' => [
            new Column(
                'primaryKey',
                [
                    'type'          => Column::TYPE_INTEGER,
                    'notNull'       => true,
                    'autoIncrement' => true,
                    'primary'       => true,
                ]
            ),
            new Column(
                'data',
                [
                    'type'    => Column::TYPE_TEXT,
                    'notNull' => true,
                ]
            )
        ]
    ]
);

$testObject1 = new Test;
$testObject1->mappedData = "Object 1";

$testObject1->save();

$testObject2 = new Test;
$testObject2->mappedData = "Object 2";

$testObject2->save();

$allRecords = Test::find();
foreach ( $allRecords as $record )
{
    echo $record->mappedPrimaryKey . ": " . $record->mappedData . "\n";
}
echo "\n";

$result = Test::findFirst( "mappedPrimaryKey = 1" );
$result->mappedData = "Object 2";   // Not unique
$result->save();

$allRecords = Test::find();
foreach ( $allRecords as $record )
{
    echo $record->mappedPrimaryKey . ": " . $record->mappedData . "\n";
}

echo "\nPhalcon Version: ", Phalcon\Version::get(), "\n";

My output:

1: Object 1
2: Object 2

1: Object 2
2: Object 2

Phalcon Version: 3.4.0
@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 7, 2018

The only way i see it it's somehow problem with sqlite adapter or something. Uniqueness validator cod sie fine, the problem is either with metadata or the sqlite adapter. What about mysql?

@JetA2

This comment has been minimized.

Copy link
Author

JetA2 commented Jun 8, 2018

From what I can see in the source code, there is an issue with the validator itself. The validator uses the readAttribute function of the Phalcon\Mvc\Model class to fetch the value of the primary key to bind in the query.

This is the readAttribute function:

	/**
	 * Reads an attribute value by its name
	 *
	 * <code>
	 * echo $robot->readAttribute("name");
	 * </code>
	 */
	public function readAttribute(string! attribute)
	{
		if !isset this->{attribute} {
			return null;
		}

		return this->{attribute};
	}

If understand Zephir correctly, the readAttribute function is looking for a member variable with the name supplied to the function in the "attribute" parameter. On the model in the sample code, this member variable would be named "mappedPrimaryKey" since the model uses a column map.

However, the uniqueness validator supplies the readAttribute function with the variable "primaryField":

let params["bind"][] = record->readAttribute(primaryField);

The variable "primaryField" is populated from the function "getPrimaryKeyAttributes" from the Phalcon\Mvc\Model\MetaData class. The function gets the primary key column name from the database schema. Thus the variable "primaryField" in the validator will contain the column name "primaryKey" for the sample code.

The "readAttribute" function will not find a member variable in the model named "primaryKey" since the member variable is named "mappedPrimaryKey" and will return null instead of the numeric primary key value. A null value will then be bound in the query, which can be seen in the query log.

If line 298 in the uniqueness validator is changed to the following I believe it should take care of the issue:

let params["bind"][] = record->readAttribute( this->getColumnNameReal(record, primaryField) );
@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 13, 2018

That's weird, in deed it returns null. Not sure why someone missed this.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 13, 2018

Well i guess this almost never happened because you almost always have primary key like id in table and in model $id as well and that's why no one had this issue.

Jurigag added a commit to Jurigag/cphalcon that referenced this issue Jun 13, 2018

Jurigag added a commit to Jurigag/cphalcon that referenced this issue Jun 13, 2018

@Jurigag Jurigag referenced this issue Jun 13, 2018

Merged

Fixes #13398 #13408

3 of 3 tasks complete

@sergeyklay sergeyklay added this to the 3.4.1 milestone Jun 14, 2018

sergeyklay added a commit that referenced this issue Jun 17, 2018

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 17, 2018

Fixed in the 3.4.x branch. Feel free to open a new issue if the problem appears again. Thank you for contributing.

@sergeyklay sergeyklay closed this Jun 17, 2018

sergeyklay added a commit that referenced this issue Jun 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.