<div dir="ltr">This makes sense to me. I wonder if we should document what you've outlined in the plugin writers guide? If so, then maybe we should repurpose 5828?<br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><br></div><div>David<br></div></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse <<a href="mailto:bmbouter@redhat.com">bmbouter@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>After some IRC discussion during open floor, I think the consensus is that we should not have BaseModel call full_clean(). Plugin writers doing a lot of object creation without involving DRF serializers should call full_clean() either in application code or their specific model's save() method. Here's some recap about the motivations for this:</div><div><br></div><div>* By having full_clean() called with all model save it gives Pulp "two validation layers" when there only needs to be one. DRF's validation layer is the serializer, and it isn't prepared to do error handling from a "second" layer. This is partly an echo of the concerns from Tom Christie's writing.</div><div>* DRF is primarily designed to have data flow through its serializers. This issue is more problematic in cases where data is not flowing through the serializer. Thus we should try to include the serializer whenever possible.</div><div>* If we cannot include the serializer, those are cases where we would specifically benefit from calling full_clean manually.</div><div><br></div><div>Other thoughts and concerns are welcome. If nothing major comes up then we can close <a href="https://pulp.plan.io/issues/5828" target="_blank">https://pulp.plan.io/issues/5828</a> as WONTFIX.</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz <<a href="mailto:gmbnomis@gmail.com" target="_blank">gmbnomis@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:<br>
> If anyone has concerns with us enabling Model validation by default on<br>
> all models please let us know soon.<br>
<br>
I don't know (yet) if I have concerns, but DRF seems to have, see<br>
<a href="https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform" rel="noreferrer" target="_blank">https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform</a><br>
<br>
According to DRF's design, all validation logic should be at one<br>
place, which is the serializer. This seems to be a controversial<br>
issue, see e.g. <br>
<a href="https://github.com/encode/django-rest-framework/issues/3144" rel="noreferrer" target="_blank">https://github.com/encode/django-rest-framework/issues/3144</a>. From<br>
that issue:<br>
<br>
What happens in the case where in your models you are forcing a<br>
full_clean (perhaps by including it in the save method)? Will<br>
serializers know how to handle an exception from an explicit<br>
full_clean?<br>
<br>
And Tom Christie's answer:<br>
<br>
I'd recommend that application level validation should generally<br>
happen prior to save, not during it. Personally I'd avoid<br>
full_clean, and instead ensure that state changing operations on<br>
model instances are only ever made via method calls that can provide<br>
a boundary that ensures that only valid state changes may ever be<br>
made by the rest of the application.<br>
<br>
<br>
<br>
We need to be aware that we are leaving the path recommended by DRF<br>
if we implement this proposal and mix Django validation and DRF<br>
validation. Unfortunately, I don't know what the alternative is. <br>
Using DRF serializers to construct all model instances looks clumsy<br>
when it comes to relations.<br>
<br>
</blockquote></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>