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

Mon Dec 2 21:25:58 UTC 2019

On Mon, Dec 2, 2019 at 3:53 PM David Davis <daviddavis at 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 <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/e946e63e/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/e946e63e/attachment.png>