"error validating BELONGS_TO relation" new approach of fix discussion

[grikdotnet]

I have models A BELONGS_TO B (a.b_id foreign key references b.id),
writing by the manual:
$a = new A;
$a->b = new B();
$a->withRelated(true,array('b'))->save();

If I have a validator in A like array('b_id', 'required'), I get an error 'B Id cannot be blank.'

If I disable this validator it works fine. Looks like validator is executed before filling up the relation.

[creocoder]

Validator is executed before filling up the relation. This is by design. Give an example of a case where it is not convenient for real-world example.

[grikdotnet]

Импорт товара для каталога:

$product = new Product;
$product->manufacturer = Manufacturer::model->find('name=:name',array(':name'=>$data['manufacturer_name'])) ?: Manufacturer::factory($data['manufacturer_name']);
$product->name = $data['product_name'];
$product->withRelated->save(array('manufacturer'));

У товара должен быть производитель, это поле в таблице - not null и FK на таблицу manufacturers.
В модели product это поле должно валидироваться на обязательность заполнения, иначе это нарушает логику приложения. Модель Product должна использоваться и без расширения withRelated, и выдавать ошибку на отсутствие manufacturer_id при валидации (при редактировании описаний товаров), а не доводить до sql exception.

[creocoder]

Хорошо, сделаем для BELONGS_TO исключение.

[cnanders]

I ran in to this same issue today. I can't read the Russian in the above two comments, but it seems to me this is a problem in many real-world situations. Using the example from above:

$a = new A;
$a->b = new B();
$a->withRelated->save(true,array('b'));

It seems to me like the validator should first run on object B, then save/update object B, then pull the primary_key from B and fill in the foreign_key property of A, then run the validator on A, then save A. This way you can set force the foreign_key on A to be not null and still run all of the validators. The idea would be to run the validators starting at lowest child model upwards to the highest parent model, saving and filling in foreign keys after each validation. If there ends up being a failed validator somewhere in the chain, you would need to go backwards through the children, undoing each save. For this, you would need to keep a cache of every model that is updated through the relation chain and discard only when you get through everything without a validation error.

By the way, this is a great extension! Thanks for building and maintaining it

[grikdotnet]

creocoder said he'll fix it

[grikdotnet]

actually, cnanders is right - the related model should be checked if it's new and saved before validating the core one for MANY-MANY as well, not just for BELONGS_TO

[creocoder]

The problem can be fixed a much more elegant way. I'm working on it.

[creocoder]

Fixed

[cnanders]

creocoder: Thanks! That was a fast turnaround. Looking forward to putting my "not null" properties back on my FKs. I know I already said it, but you did an excellent job with this extension.

[creocoder]

Thanks ;-)

[creocoder]

@grikdotnet @cnanders Guys sorry, I decide rollback this fix + add some new features. Problems you posted can be solved other way. After this fix behavior perfomance down because there is no more prevalidation. We can discuss before rollback and i can show how to solve problems and tell why this problems must be solved other way.

[cnanders]

No problem. Thanks for contacting us. You said they can be solved another way - does this mean the updated version will have a fix, or will I have to get rid of the "not null" properties on FKs in the database?

[creocoder]

First. Why i want rollback? Because fix changed behavior approach from prevalidate all/save all to validate/save/validate/save. The last approach not good in perfomance terms + not work on DBs without transaction support. This is why this fix will be rollbacked.

@cnanders Can you show example when you have problems with this extension before fix? I just want to show how to solve this problem without this fix.

[cnanders]

@creocoder The example I describe above (I'll include it here again) is the simplest scenario where I ran into problems. The fix was to remove the "not null" property in the database model.

**************** Example of when I had problems with the extension before fix *******

$a = new A;
$a->b = new B();
$a->withRelated->save(true,array('b'));

It seems to me like the validator should first run on object B, then save/update object B, then pull the primary_key from B and fill in the foreign_key property of A, then run the validator on A, then save A. This way you can set force the foreign_key on A to be not null and still run all of the validators. The idea would be to run the validators starting at lowest child model upwards to the highest parent model, saving and filling in foreign keys after each validation. If there ends up being a failed validator somewhere in the chain, you would need to go backwards through the children, undoing each save. For this, you would need to keep a cache of every model that is updated through the relation chain and discard only when you get through everything without a validation error.

*********** END *************

[creocoder]

The fix was to remove the "not null" property in the database model.
No, this is bad solution :)

@cnanders Ok. I will tell you how you example could work with old approach. Validator first run on model B, then validator run on model A. Then if all valid we save model B, than pull PK from B and fill in FK property of A, than save A. Example will work fine. Where is an problem?

[cnanders]

When you use Gii to auto-generate models, if the database has a "not null" attribute, it adds a validator criteria to the model like this:

public function rules(){
return array(
array('your_foreign_key_property','required');
)
}

and this will fail validation when you validate model A if the property 'your_foreign_key_property' is not first filled before validation

[creocoder]

@cnanders You should not validate data which you can trust (which input not from user). This is meaningless and bad to perfomance. You can just remove this rule. Even not can, must remove :)

[cnanders]

@creocoder That is a good point. It is easy enough to comment out those validators that are auto-generated by Gii. I guess people should just be aware of it though, because if they generate models with Gii (I imagine most people do) then they will fail validation until the 'required' validator on the FK is commented out

(added) Perhaps Gii should be updated with a flag if(FK) && not-null, don't add 'required' validator

[creocoder]

@cnanders They must remove useless and meaningless rules after model generation or we can talk about bad programming practice if they don't. I think we do not need encourage people laziness especially when its cost behavior perfomance, lost non transactional DBs and prevalidation feature.

[cnanders]

Definitely. Thanks for taking the time to explain the rollback. You are making the right choice.

[creocoder]

@cnanders One solution to the "lazy people problem" is the dynamic elimination of FKs validation rules. I think about it. Behavior can do it automatically.

[grikdotnet]

I can't agree with a statement "You should not validate data input not from user".
AR Model is a database table mapped to an object. If a database table has a constraint, it is mapped to a validator.
If we partially map the table - a structure without constraints, we have a chance to shoot a foot, especially in a team development.

An .11 feature of excluding validators can be used as a hack to solve this chicken-egg problem. In this case a developer should place efforts to document this unobvious logic as carefully and clear as possible, cause nobody will guess it.

[creocoder]

If a database table has a constraint, it is mapped to a validator.

As i say you should not validate data which you can trust (which input not from user). Behavior will set this FKs in any case. Just use another scenarios if you real need FK validation. This will resolve problem too.

[grikdotnet]

well, I guess it is something that needs to be documented

[creocoder]

Ok. I will document it and all new features. I'm always use constraints in DBMS and never have problems with this extension even if FK sometimes need validation (when data come from user for example or from resource we cant trust), Example in you 3rd post just need use another scenario for import. It is advisable in all respects, including performance.

[grikdotnet]

Creo, your personal experience is not a common case due to your qualification. I manage a team of ordinary coders and can't just remove validation from a model cause they rely on the model API. I need to keep validators and scenarios understandable for people who did not read this conversation.
That's the issue with the solution.

[creocoder]

Im not propose remove all FKs validation from model, im tell only about scenarios where FKs validation meaningless.

[choojoy-work]

После последнего обновления перестало корректно работать расширение. У меня есть модель A и зависимые модели B, связь HAS_MANY. При попытке сохранить новую модель A с моделями B пишет ошибку валидации, что в моделях B не определен a_id, который у меня required. Я так понимаю, то же самое, что и в [url=https://github.com//issues/12]багтрекере[/url], что помечено как исправленное. В версии 0.63 такой проблемы не было, но коммит с ней удалили с гитхаба, а в новой 0.64 снова такая же проблема.

[creocoder]

@choojoy-work Для сохранения моделей B вместе с моделями A валидация по внешнему ключю a_id не требуется, т.к. расширение после валидации в любом случае самостоятельно проставляет значения внешних ключей. Таким образом валидация по этим ключам не имеет никакого смысла. В вашем случае нужно, либо устранить валидатор внешнего ключа, либо вынести валидацию внешнего ключа в сценарий, в котором такая валидация целесообразна.

[choojoy-work]

@creocoder, да, но в 0.63 у меня валидатор прекрасно уживался с поведением. Насколько я понимаю, ошибки внешних ключей игнорировались за счёт clearErrors(), в новой версии специально от этого отказались, есть причина? Я понимаю, что в общем-то и так нормально, но немного неприятна зависимость правил моделей от наличия или отсутствия поведения и вносимая этим нестабильность.
P. S. Спасибо за отличное расширение.

[creocoder]

@choojoy-work Наличие правил валидации для внешний ключей в случае сценария, в котором сохраняется основная и связанные модели — это ошибка проектирования модели. Такого правила там просто не должно быть, оно бессмысленно, это просто лишний вредный код. Версия 0.63 эту халатность со стороны разработчика прощала, при этом пришлось изменить принцип работы поведения. Версия 0.64 вернулась к своей первоначальной архитектуре и принципам и не будет прощать «абсолютно бесполезные и даже вредные правила валидации» для конкретной ситуации. Этого правила не должно быть как в случае использования поведения, так и без него, если это не случай, когда пользователь вносит данные о внешнем ключе через форму, либо если это процедура импорта данных из недоверенного источника.

Кстати, откуда и с какой целью у вас появилось данное правило валидации в моделях?

[choojoy-work]

@creocoder Спасибо за подробное разъяснение, вы правы. Так что данное правило появилось по глупости для лишней перестраховки.