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

Mon Dec 2 21:28:00 UTC 2019

I'm on it.

David

On Mon, Dec 2, 2019 at 4:26 PM Brian Bouterse <bmbouter at redhat.com> wrote:

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