<div dir="ltr"><div>Thanks @smyers and asmacdo. This convinces me that we should just use the DRF serializer for all validation and expect that full_clean() is not the authority on what it means to be validated. I was really holding out on full_clean, but I can see now that we should go wholesale into DRF serializers.<br><br></div><div>Regarding vocabulary, @asmacdo suggested using "validation" and "business logic" as two separate terms. Those make sense to me. Here is a quick definition, maybe others could refine:<br><br>* Validation:  Ensuring that all data accepted is correct as defined by the serializer. This is solely the responsibility of a DRF serializer and not other Django facilities like full_clean() or clean().<br></div><div>* Business Logic: Logic that determines how data can be created, stored, or changed.<br></div><div><br>== questions ==<br></div><div>* Where should ^ terms be documented?<br><br></div><div>* Take the case of a sync which has overrides provided? This isn't in the MVP, but in the future it could be. In that case, does the serializer associated with the importer validate the database data with the overrides "added" on top of it?<br><br></div><div>* For plugin writers writing a serializer for a subclassed Importer, do they also need to express validations for fields defined on the base Importer?<br><br>* The database still rejects data that doesn't adhere to the data layer 
definition right? That occurs even without the DRF serializer correct?<br><br></div><div>* In cases where data is created in the backend, do we need to validate that as a general practice? If we do, do we call the DRF serializer regularly in the backend or just let the database reject "bad" data at the db level?<br><br></div><div>Thanks for all the discussion on this topic.<br><br></div><div>-Brian<br></div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 7, 2017 at 11:08 AM, Sean Myers <span dir="ltr"><<a href="mailto:sean.myers@redhat.com" target="_blank">sean.myers@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 04/06/2017 06:40 PM, Austin Macdonald wrote:<br>
> tldr;<br>
>     1. Serializer errors won't be caught by serializer validation.<br>
>     2. DRF does not use full_clean(), so if data passes serializer<br>
> validation, it gets into the db.<br>
>     3. We can use full_clean for process level validation.<br>
><br>
> During a lengthy discussion on IRC, I incorrectly asserted that full_clean<br>
> did not work with DRF.<br>
><br>
> My thinking was partly based on the incorrect assumption that the data was<br>
> valid. This seemed reasonable given that I created the data from the REST<br>
> API, so the data was validated by the serializers. After a closer look, I<br>
> fixed an issue with the serializer and was able to use full_clean() as<br>
> expected.<br>
><br>
> The serializer formatted the REST data and then validated the data against<br>
> its own expectations. Of course it thought it was right!<br>
<br>
</div></div>Here's what I was getting at yesterday:<br>
<br>
<a href="https://paste.fedoraproject.org/paste/T2rjzc4RXrvD8DtXc5y3MF5M1UNdIGYhyRLivL9gydE=" rel="noreferrer" target="_blank">https://paste.fedoraproject.<wbr>org/paste/<wbr>T2rjzc4RXrvD8DtXc5y3MF5M1UNdIG<wbr>YhyRLivL9gydE=</a><br>
<br>
full_clean is an interface that Model instances provide to work with ModelForm.<br>
<br>
>From <a href="https://docs.djangoproject.com/en/1.10/ref/models/instances/#validating-objects" rel="noreferrer" target="_blank">https://docs.djangoproject.<wbr>com/en/1.10/ref/models/<wbr>instances/#validating-objects</a>:<br>
<br>
"""<br>
<br>
Model.full_clean(exclude=None, validate_unique=True)[source]¶<br>
<br>
This method calls Model.clean_fields(), Model.clean(), and Model.validate_unique()<br>
(if validate_unique is True), in that order and raises a ValidationError that has a<br>
message_dict attribute containing errors from all three stages.<br>
"""<br>
<br>
The interface is well-defined, and consists of more than just "full_clean". Reusing<br>
it for our purposes, with the claim of "supporting the Django way of doing things"<br>
doesn't really work, since we're so heavily using Django REST Framework, and this is<br>
explicitly *not* the way to do things in DRF. Not only does DRF not use this interface,<br>
they consciously made the choice to discontinue its use:<br>
<br>
<a href="http://www.django-rest-framework.org/topics/3.0-announcement/#differences-between-modelserializer-validation-and-modelform" rel="noreferrer" target="_blank">http://www.django-rest-<wbr>framework.org/topics/3.0-<wbr>announcement/#differences-<wbr>between-modelserializer-<wbr>validation-and-modelform</a><br>
<br>
Some of their justification is documented here:<br>
<br>
<a href="http://www.django-rest-framework.org/api-guide/validators/#validation-in-rest-framework" rel="noreferrer" target="_blank">http://www.django-rest-<wbr>framework.org/api-guide/<wbr>validators/#validation-in-<wbr>rest-framework</a><br>
<br>
I agree with their major points, particularly as they relate to a separation of<br>
concerns. I think that for input validation, either using a model's full_clean<br>
interface or using its related ModelSerializer should be done, not both. Since<br>
we need Model Serializers for the API, it makes sense to use the serializers<br>
for this, and to never use full_clean, so that we don't provide or implement<br>
two interfaces for the same job.<br>
<br>
Any "process validation" (I term I made up to try to distinguish what we're<br>
talking about from input validation, which is data validation done "at the<br>
edges") should be done by whatever process needs "valid" data. As far as I know,<br>
the interface for it does not exist, since it's for validation for a very pulp-<br>
specific bit of process.<br>
<br>
An interface should be implemented or created to address the "process validation"<br>
concern, but reusing either the DRF serializer or Django's full_clean interface to<br>
provide this behavior is a recoupling of concerns that should be avoided.<br>
<br>
Finally:<br>
Discussing this has been very difficult as a result of our overloading of the term<br>
"validation", so I recommend getting clearly defined use-cases and settling on a<br>
shared vocabulary before spending more time thinking and talking about this.<br>
<br>
<br>______________________________<wbr>_________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a><br>
<br></blockquote></div><br></div>