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