+1 to have additional serializer for validation content fields and use current serializers for upload case +1 to validate during sync
On Fri, Dec 6, 2019 at 8:31 PM David Davis <davidda...@redhat.com> wrote: > I talked to @ttereshc this afternoon and a couple questions came up. > > First, in pulp_rpm we have a PackageSerializer[0] that handles creating > and displaying packages. The problem is that the package field values are > derived from the artifact and not the user so most fields are readonly and > not required. I'm imagining we'll have to split this serializer in two (ie > a PackageSerializer and a PackageUploadSerializer) in order to use it to > validate package data. Does that make sense or is there a better way? > > Secondly, I don't think we're validating data during sync. What if a user > syncs data from a remote that has bad data? I think we'll need to have the > stages somehow use serializers as well? If others agree, I can file an > issue. > > [0] > https://github.com/pulp/pulp_rpm/blob/326d189bbae9267d59282d62ada1fa36467a2d8f/pulp_rpm/app/serializers.py#L69 > > David > > > On Thu, Dec 5, 2019 at 6:32 AM David Davis <davidda...@redhat.com> wrote: > >> 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 >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
