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

Unique validation rule needs to check if target attribute has previous validation errors #19407

Closed
xcopy opened this issue May 23, 2022 · 8 comments
Labels
Milestone

Comments

@xcopy
Copy link

xcopy commented May 23, 2022

What steps will reproduce the problem?

  1. Create a claim table
$this->createTable('{{%claim}}', [
    'id' => $this->primaryKey(),
    'event_date' => $this->date()->notNull(),
]);
  1. Create a medication table
$this->createTable('{{%medication}}', [
    'id' => $this->primaryKey(),
    'name' => $this->string()->notNull()->unique(),
]);
  1. Create a junction table claim_medication
$this->createTable('{{%claim_medication}}', [
    'id' => $this->primaryKey(),
    'claim_id' => $this->integer()->notNull(),
    'medication_id' => $this->integer()->notNull(),
]);

$this->addForeignKey(
    'fk-claim_medication-claim_id',
    'claim_medication',
    'claim_id',
    'claim',
    'id'
);

$this->addForeignKey(
    'fk-claim_medication-medication_id',
    'claim_medication',
    'medication_id',
    'medication',
    'id'
);

// IMPORTANT: unique index on two columns
$this->createIndex(
    'idx-claim_medication-unique',
    'claim_medication',
    ['claim_id', 'medication_id'],
    true
);

Generated rules in the ClaimMedication model:

public function rules()
{
    return [
        [['claim_id', 'medication_id'], 'required'],
        [['claim_id', 'medication_id'], 'default', 'value' => null],
        [['claim_id', 'medication_id'], 'integer'],
        [['strength', 'quantity', 'duration'], 'string', 'max' => 255],
        [['claim_id', 'medication_id'], 'unique', 'targetAttribute' => ['claim_id', 'medication_id']],
        [['claim_id'], 'exist', 'skipOnError' => true, 'targetClass' => Claim::className(), 'targetAttribute' => ['claim_id' => 'id']],
        [['medication_id'], 'exist', 'skipOnError' => true, 'targetClass' => Medication::className(), 'targetAttribute' => ['medication_id' => 'id']],
    ];
}
  1. Try to create a new ClaimMedication model
$model = new app\models\ClaimMedication();

// case 1
$model->claim_id = 1;
$model->medication_id = null;
$model->save(); // all good, just one error: "Medication ID cannot be blank."

// case 2
$model->claim_id = 1;
$model->medication_id = 1;
$model->save(); // all good, no errors

// case 3
$model->claim_id = 1;
$model->medication_id = ''; // an empty string
$model->save(); // not good

What is the expected result?

Still expecting a single error on case 3: Medication ID cannot be blank.

What do you get instead?

Database exception on case 3 (i.e. unique rule in action):

yii\db\Exception with message 'SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for type integer: ""
The SQL being executed was: SELECT EXISTS(SELECT * FROM "claim_medication" WHERE ("claim_medication"."claim_id"=1) AND ("claim_medication"."medication_id"=''))'

Additional info

Q A
Yii version 2.0.46-dev
PHP version 7.4.29
Operating system Ubuntu 20.04

IMPORTANT: If I swap the order of the rules required & default, then everything works as expected. But this change may affect of other models. The problem occurs precisely when there is more than one attribute in the rule unique.

@bizley
Copy link
Member

bizley commented May 23, 2022

There is nothing wrong with the rules themselves. You need to modify the integer rule config though:

[['claim_id', 'medication_id'], 'integer', 'skipOnEmpty' => false],

Without that addition empty strings will not be validated and you will pass not allowed value to your DB.

@bizley bizley closed this as completed May 23, 2022
@xcopy
Copy link
Author

xcopy commented May 23, 2022

There is nothing wrong with the rules themselves. You need to modify the integer rule config though:

[['claim_id', 'medication_id'], 'integer', 'skipOnEmpty' => false],

Without that addition empty strings will not be validated and you will pass not allowed value to your DB.

It didn't help. Still can see that exception.

Yes, there is nothing wrong with the rules themselves, I agree. But, can anyone has/had the same issue? Any thoughts?

@bizley
Copy link
Member

bizley commented May 23, 2022

Hmm, I can see a slight problem indeed here, we need to correct this.

When unique rule is about to be fired and it uses one of the target attributes that has some error already, it should not be fired when skipOnError is set to true (default).

@bizley bizley reopened this May 23, 2022
@bizley bizley changed the title Database exception on combination of required, default and unique rules Unique validation rule needs to check if target attribute has previous validation errors May 23, 2022
@bizley bizley added the type:bug Bug label May 23, 2022
@bizley bizley added this to the 2.0.46 milestone May 23, 2022
@xcopy
Copy link
Author

xcopy commented May 23, 2022

...one of the target attributes that has some error already

@bizley yeah, that's what I wanted to say. Thanks.

@WinterSilence
Copy link
Contributor

hm...I'm check this case with MySQL8 and SELECT EXISTS(SELECT * FROM order_item WHERE (order_item.order_id=1) AND (order_item.item_id='')) executed without errors. What's database(and version) you use?

@xcopy
Copy link
Author

xcopy commented May 30, 2022

What's database(and version) you use?

PostgreSQL 12.x

@particleflux
Copy link
Contributor

@samdark
This fix has an undocumented side effect: If the validators target multiple attributes, only one of them is reported as an attribute with errors.
Previously, it would report both of them.

Example:

[['mailAddress', 'objectId'], 'unique', 'targetAttribute' => ['mailAddress', 'objectId'],],

Would previously create these errors:

'mailAddress' => [
    'The combination "verified@test.local"-"1" of Mail Address and Object Id has already been taken.',
],
'objectId'     => [
    'The combination "verified@test.local"-"1" of Mail Address and Object Id has already been taken.',
],

But now it only creates one of them:

'objectId'     => [
    'The combination "verified@test.local"-"1" of Mail Address and Object Id has already been taken.',
],

Not sure if worth fixing, but maybe might be worth noting in the CHANGELOG?

@bizley
Copy link
Member

bizley commented Aug 23, 2023

Hm, yes, probably you are right, although it was 1 year ago. Do you time to add PR with that changelog mention for this version?

particleflux added a commit to particleflux/yii2 that referenced this issue Aug 25, 2023
The `unique` and `exists` validators behave slightly different since
2.0.46 when used on multiple/combined attributes.

See: yiisoft#19407 (comment)
particleflux added a commit to particleflux/yii2 that referenced this issue Aug 25, 2023
The `unique` and `exists` validators behave slightly different since
2.0.46 (issue yiisoft#19407) when used on multiple/combined attributes.

See: yiisoft#19407 (comment)
samdark pushed a commit that referenced this issue Aug 26, 2023
The `unique` and `exists` validators behave slightly different since
2.0.46 (issue #19407) when used on multiple/combined attributes.

See: #19407 (comment)
yii-bot pushed a commit to yiisoft/yii2-framework that referenced this issue Aug 26, 2023
The `unique` and `exists` validators behave slightly different since
2.0.46 (issue #19407) when used on multiple/combined attributes.

See: yiisoft/yii2#19407 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants