I'm on it. David
On Mon, Dec 2, 2019 at 4:26 PM Brian Bouterse <bmbou...@redhat.com> wrote: > > > On Mon, Dec 2, 2019 at 3:53 PM David Davis <davidda...@redhat.com> wrote: > >> I think calling full_clean() in BaseModel's save() makes sense and is a >> more consistent experience for plugin writers. Maybe I am missing the >> downsides? >> >> +1 to before GA. >> > I created this issue https://pulp.plan.io/issues/5828 Can someone groom > it and post a PR to see what the tests do? > >> >> David >> >> >> On Mon, Dec 2, 2019 at 3:41 PM Tatiana Tereshchenko <ttere...@redhat.com> >> wrote: >> >>> Thanks for finding the cause of the lack of validation and for sharing >>> the link to the docs. >>> Adding full_clean() solves the problem with UpdateRecord which I brought >>> up. >>> >>> I'm concerned that if we don't introduce validation now, before GA, >>> users might have bad/missing data in the DB and later if we decide to add >>> validation, it can fail for an update on the existing model instance. >>> In addition, it can potentially affect data migration, if some required >>> fields are missing. >>> >>> To add it everywhere, we can override the `save` method for our base >>> model >>> https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/base.py#L9 >>> >>> I believe that RPM plugin is affected (see my initial email) and needs >>> to incorporate the change. It's hard to say without inspecting the models >>> how much and how critically the core is affected. >>> I'm working under the assumption that it's easier from the DB >>> perspective to go stricter and later loosen validation if needed, than the >>> other way around. >>> >>> What do you think? >>> >>> 1. Should we add validation for all the models or just selectively? >>> 2. Before or after GA? >>> >>> >>> >>> On Mon, Dec 2, 2019 at 8:24 PM Brian Bouterse <bmbou...@redhat.com> >>> wrote: >>> >>>> Thank you for bumping this thread. >>>> >>>> The problem you illustrate with UpdateRecord makes sense and is >>>> problematic. Django doesn't have Model.save() call full_clean() by default; >>>> they document that here ( >>>> https://docs.djangoproject.com/en/dev/ref/models/instances/?from=olddocs#django.db.models.Model.full_clean >>>> ). I interpret the Django docs recommendation to this situation as: plugin >>>> writers should either call full_clean() directly when handling model data >>>> directly, or bake it into their save() methods. >>>> >>>> pulpcore has a similar question w.r.t its models. Maybe pulpcore should >>>> be calling full_clean() more, or baking the full_clean() call into save() >>>> in its models? pulpcore and plugins should try to think about how this >>>> applies to their code. >>>> >>>> My opinion is that the impact is low enough that we don't need to make >>>> an adjustment before the GA. I'd like to hear from anyone if you think >>>> there is something we should do before the GA. >>>> >>>> >>>> >>>> On Mon, Dec 2, 2019 at 10:20 AM Tatiana Tereshchenko < >>>> ttere...@redhat.com> wrote: >>>> >>>>> Bump. >>>>> Any thoughts? >>>>> >>>>> If we decide to change something, we'd better do it before GA, I think. >>>>> >>>>> >>>>> On Mon, Nov 18, 2019 at 10:07 PM David Davis <davidda...@redhat.com> >>>>> wrote: >>>>> >>>>>> Without diving into the django code, I'm guessing that the problem is >>>>>> the default value for text fields is a blank string. I bet if you set the >>>>>> default=None on fields that don't accept null, they would become >>>>>> 'required' >>>>>> (although I don't think this concept actually exists in django at the >>>>>> model >>>>>> level). >>>>>> >>>>>> I think Fabricio's solution would work. However, it doesn't 'require' >>>>>> fields so much as prevent blank strings from being saved in the database. >>>>>> And I think that's what we want? >>>>>> >>>>>> David >>>>>> >>>>>> >>>>>> On Mon, Nov 18, 2019 at 3:15 PM Fabricio Aguiar < >>>>>> fabricio.agu...@redhat.com> wrote: >>>>>> >>>>>>> the best way that I found so far is this: >>>>>>> https://stackoverflow.com/a/56272674/5253051 >>>>>>> [image: image.png]4 >>>>>>> <https://stackoverflow.com/a/56272674/5253051> >>>>>>> >>>>>>> Best regards, >>>>>>> Fabricio Aguiar >>>>>>> Software Engineer, Pulp Project >>>>>>> Red Hat Brazil - Latam <https://www.redhat.com/> >>>>>>> +55 11 999652368 >>>>>>> >>>>>>> >>>>>>> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko < >>>>>>> ttere...@redhat.com> wrote: >>>>>>> >>>>>>>> I noticed that there is no enforcement at the DB level to require >>>>>>>> certain fields to be present on a model. >>>>>>>> >>>>>>>> I haven't checked all the field types but at least for TextField >>>>>>>> it's seems to be true. >>>>>>>> Even though `null` is False by default (`blank` as well), I can >>>>>>>> save a model instance without most of fields set. >>>>>>>> >>>>>>>> As an example, for UpdateRecord [0] in RPM plugin, plugin writer >>>>>>>> can do UpdateRecord().save() and it will be fine, all the fields are >>>>>>>> set to >>>>>>>> empty string :/ It wouldn't be possible to save it twice but it's due >>>>>>>> to >>>>>>>> the uniqueness constraints. >>>>>>>> >>>>>>>> In case plugin writer doesn't set properly all the required fields, >>>>>>>> bad/corrupted model instances will be silently saved in the DB and >>>>>>>> plugin >>>>>>>> can potentially have data migration issues later. >>>>>>>> >>>>>>>> Any suggestions how to handle that? Or do I miss anything? >>>>>>>> >>>>>>>> Tanya >>>>>>>> >>>>>>>> [0] >>>>>>>> https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/models/advisory.py#L77-L93 >>>>>>>> _______________________________________________ >>>>>>>> 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 >>>>> >>>> _______________________________________________ >>> 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
