[Pulp-dev] Required fields for models at the DB level

[David Davis]

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?

David


On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse <bmbouter at redhat.com> wrote:

> 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:
>
> * 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.
> * 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.
> * If we cannot include the serializer, those are cases where we would
> specifically benefit from calling full_clean manually.
>
> Other thoughts and concerns are welcome. If nothing major comes up then we
> can close https://pulp.plan.io/issues/5828 as WONTFIX.
>
>
>
> On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz <gmbnomis at gmail.com> wrote:
>
>> On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
>> >    If anyone has concerns with us enabling Model validation by default
>> on
>> >    all models please let us know soon.
>>
>> I don't know (yet) if I have concerns, but DRF seems to have, see
>>
>> https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform
>>
>> According to DRF's design, all validation logic should be at one
>> place, which is the serializer.  This seems to be a controversial
>> issue, see e.g.
>> https://github.com/encode/django-rest-framework/issues/3144.  From
>> that issue:
>>
>> What happens in the case where in your models you are forcing a
>> full_clean (perhaps by including it in the save method)?  Will
>> serializers know how to handle an exception from an explicit
>> full_clean?
>>
>> And Tom Christie's answer:
>>
>> I'd recommend that application level validation should generally
>> happen prior to save, not during it.  Personally I'd avoid
>> full_clean, and instead ensure that state changing operations on
>> model instances are only ever made via method calls that can provide
>> a boundary that ensures that only valid state changes may ever be
>> made by the rest of the application.
>>
>>
>>
>> We need to be aware that we are leaving the path recommended by DRF
>> if we implement this proposal and mix Django validation and DRF
>> validation.  Unfortunately, I don't know what the alternative is.
>> Using DRF serializers to construct all model instances looks clumsy
>> when it comes to relations.
>>
>> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191205/f19691e6/attachment.htm>