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. 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
