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

[David Davis]

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 <ttereshc at 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 <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
>>>
>> _______________________________________________
> 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/55eb370c/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/55eb370c/attachment.png>