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

[Tatiana Tereshchenko]

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 <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/78203f18/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/78203f18/attachment.png>