[Pulp-dev] Required fields for models at the DB level

Brian Bouterse bmbouter at redhat.com
Mon Dec 2 20:53:06 UTC 2019

On Mon, Dec 2, 2019 at 3:40 PM Tatiana Tereshchenko <ttereshc at redhat.com>

> 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
Having stronger validation I believe is a good thing. We should try this
out, having the base method call full_clean() and see how many tests in the
plugins fail. I expect the normal data should mostly drive this, and
perhaps we'll need to adjust some of the "blank" option to some model

If anyone has concerns with us enabling Model validation by default on all
models please let us know soon.

> 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?
Let's try to add on all

2. Before or after GA?
Let's try for before.

> On Mon, Dec 2, 2019 at 8:24 PM Brian Bouterse <bmbouter at 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 <ttereshc at 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 <daviddavis at 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.aguiar at 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 <
>>>>> ttereshc at 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 at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>> _______________________________________________
>>>>> Pulp-dev mailing list
>>>>> Pulp-dev at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>> _______________________________________________
>>> Pulp-dev mailing list
>>> Pulp-dev at redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191202/3c8e1719/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image.png
Type: image/png
Size: 64987 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191202/3c8e1719/attachment.png>

More information about the Pulp-dev mailing list