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 <bmbou...@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 <gmbno...@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@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
