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

[Brian Bouterse]

Recently a group of developers investigated if pulpcore needed to make any
changes regarding the concerns discussed in this thread from a while ago.
Here are the conclusions we reached, please let us know if you have other
ideas or concerns:

* We lack documentation advising plugin writers that they need to use DRF
serializers to validate models. This includes cases where the data didn't
come from users but from a remote source during sync. That docs addition is
being tracked here https://pulp.plan.io/issues/5927
* No pulpcore code changes are needed since Pulpcore does not create any
objects, e.g. Content, it only saves objects already created by plugin code
in the Stages API first stage.

We'll proceed with the docs and that should resolve this issue. Please let
us know if you have concerns, questions, or ideas.

Thanks to the devs who collaborated in this effort.

-Brian


On Tue, Dec 10, 2019 at 3:39 AM Tatiana Tereshchenko 
wrote:

> +1 to have additional serializer for validation content fields and use
> current serializers for upload case
> +1 to validate during sync
>
> On Fri, Dec 6, 2019 at 8:31 PM David Davis  wrote:
>
>> I talked to @ttereshc this afternoon and a couple questions came up.
>>
>> First, in pulp_rpm we have a PackageSerializer[0] that handles creating
>> and displaying packages. The problem is that the package field values are
>> derived from the artifact and not the user so most fields are readonly and
>> not required. I'm imagining we'll have to split this serializer in two (ie
>> a PackageSerializer and a PackageUploadSerializer) in order to use it to
>> validate package data. Does that make sense or is there a better way?
>>
>> Secondly, I don't think we're validating data during sync. What if a user
>> syncs data from a remote that has bad data? I think we'll need to have the
>> stages somehow use serializers as well? If others agree, I can file an
>> issue.
>>
>> [0]
>> https://github.com/pulp/pulp_rpm/blob/326d189bbae9267d59282d62ada1fa36467a2d8f/pulp_rpm/app/serializers.py#L69
>>
>> David
>>
>>
>> On Thu, Dec 5, 2019 at 6:32 AM David Davis  wrote:
>>
>>> This makes sense to me. I wonder if we should document what you've
>>> outlined in the plugin writers guide? If so, then maybe we should repurpose
>>> 5828?
>>>
>>> David
>>>
>>>
>>> On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse 
>>> wrote:
>>>
 After some IRC discussion during open floor, I think the consensus is
 that we should not have BaseModel call full_clean(). Plugin writers doing a
 lot of object creation without involving DRF serializers should call
 full_clean() either in application code or their specific model's save()
 method. Here's some recap about the motivations for this:

 * By having full_clean() called with all model save it gives Pulp "two
 validation layers" when there only needs to be one. DRF's validation layer
 is the serializer, and it isn't prepared to do error handling from a
 "second" layer. This is partly an echo of the concerns from Tom Christie's
 writing.
 * DRF is primarily designed to have data flow through its serializers.
 This issue is more problematic in cases where data is not flowing through
 the serializer. Thus we should try to include the serializer whenever
 possible.
 * If we cannot include the serializer, those are cases where we would
 specifically benefit from calling full_clean manually.

 Other thoughts and concerns are welcome. If nothing major comes up then
 we can close https://pulp.plan.io/issues/5828 as WONTFIX.



 On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz  wrote:

> On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
> >If anyone has concerns with us enabling Model validation by
> default on
> >all models please let us know soon.
>
> I don't know (yet) if I have concerns, but DRF seems to have, see
>
> https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform
>
> According to DRF's design, all validation logic should be at one
> place, which is the serializer.  This seems to be a controversial
> issue, see e.g.
> https://github.com/encode/django-rest-framework/issues/3144.  From
> that issue:
>
> What happens in the case where in your models you are forcing a
> full_clean (perhaps by including it in the save method)?  Will
> serializers know how to handle an exception from an explicit
> full_clean?
>
> And Tom Christie's answer:
>
> I'd recommend that application level validation should generally
> happen prior to save, not during it.  Personally I'd avoid
> full_clean, and instead ensure that state changing operations on
> model instances are only ever made via method calls that can provide
> a boundary that ensures that only valid state changes may ever be
> made by 

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

[Tatiana Tereshchenko]

+1 to have additional serializer for validation content fields and use
current serializers for upload case
+1 to validate during sync

On Fri, Dec 6, 2019 at 8:31 PM David Davis  wrote:

> I talked to @ttereshc this afternoon and a couple questions came up.
>
> First, in pulp_rpm we have a PackageSerializer[0] that handles creating
> and displaying packages. The problem is that the package field values are
> derived from the artifact and not the user so most fields are readonly and
> not required. I'm imagining we'll have to split this serializer in two (ie
> a PackageSerializer and a PackageUploadSerializer) in order to use it to
> validate package data. Does that make sense or is there a better way?
>
> Secondly, I don't think we're validating data during sync. What if a user
> syncs data from a remote that has bad data? I think we'll need to have the
> stages somehow use serializers as well? If others agree, I can file an
> issue.
>
> [0]
> https://github.com/pulp/pulp_rpm/blob/326d189bbae9267d59282d62ada1fa36467a2d8f/pulp_rpm/app/serializers.py#L69
>
> David
>
>
> On Thu, Dec 5, 2019 at 6:32 AM David Davis  wrote:
>
>> This makes sense to me. I wonder if we should document what you've
>> outlined in the plugin writers guide? If so, then maybe we should repurpose
>> 5828?
>>
>> David
>>
>>
>> On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse 
>> wrote:
>>
>>> After some IRC discussion during open floor, I think the consensus is
>>> that we should not have BaseModel call full_clean(). Plugin writers doing a
>>> lot of object creation without involving DRF serializers should call
>>> full_clean() either in application code or their specific model's save()
>>> method. Here's some recap about the motivations for this:
>>>
>>> * By having full_clean() called with all model save it gives Pulp "two
>>> validation layers" when there only needs to be one. DRF's validation layer
>>> is the serializer, and it isn't prepared to do error handling from a
>>> "second" layer. This is partly an echo of the concerns from Tom Christie's
>>> writing.
>>> * DRF is primarily designed to have data flow through its serializers.
>>> This issue is more problematic in cases where data is not flowing through
>>> the serializer. Thus we should try to include the serializer whenever
>>> possible.
>>> * If we cannot include the serializer, those are cases where we would
>>> specifically benefit from calling full_clean manually.
>>>
>>> Other thoughts and concerns are welcome. If nothing major comes up then
>>> we can close https://pulp.plan.io/issues/5828 as WONTFIX.
>>>
>>>
>>>
>>> On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz  wrote:
>>>
 On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
 >If anyone has concerns with us enabling Model validation by
 default on
 >all models please let us know soon.

 I don't know (yet) if I have concerns, but DRF seems to have, see

 https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform

 According to DRF's design, all validation logic should be at one
 place, which is the serializer.  This seems to be a controversial
 issue, see e.g.
 https://github.com/encode/django-rest-framework/issues/3144.  From
 that issue:

 What happens in the case where in your models you are forcing a
 full_clean (perhaps by including it in the save method)?  Will
 serializers know how to handle an exception from an explicit
 full_clean?

 And Tom Christie's answer:

 I'd recommend that application level validation should generally
 happen prior to save, not during it.  Personally I'd avoid
 full_clean, and instead ensure that state changing operations on
 model instances are only ever made via method calls that can provide
 a boundary that ensures that only valid state changes may ever be
 made by the rest of the application.



 We need to be aware that we are leaving the path recommended by DRF
 if we implement this proposal and mix Django validation and DRF
 validation.  Unfortunately, I don't know what the alternative is.
 Using DRF serializers to construct all model instances looks clumsy
 when it comes to relations.

 ___
>>> Pulp-dev mailing list
>>> Pulp-dev@redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>> ___
> Pulp-dev mailing list
> Pulp-dev@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[David Davis]

I talked to @ttereshc this afternoon and a couple questions came up.

First, in pulp_rpm we have a PackageSerializer[0] that handles creating and
displaying packages. The problem is that the package field values are
derived from the artifact and not the user so most fields are readonly and
not required. I'm imagining we'll have to split this serializer in two (ie
a PackageSerializer and a PackageUploadSerializer) in order to use it to
validate package data. Does that make sense or is there a better way?

Secondly, I don't think we're validating data during sync. What if a user
syncs data from a remote that has bad data? I think we'll need to have the
stages somehow use serializers as well? If others agree, I can file an
issue.

[0]
https://github.com/pulp/pulp_rpm/blob/326d189bbae9267d59282d62ada1fa36467a2d8f/pulp_rpm/app/serializers.py#L69

David


On Thu, Dec 5, 2019 at 6:32 AM David Davis  wrote:

> This makes sense to me. I wonder if we should document what you've
> outlined in the plugin writers guide? If so, then maybe we should repurpose
> 5828?
>
> David
>
>
> On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse 
> wrote:
>
>> After some IRC discussion during open floor, I think the consensus is
>> that we should not have BaseModel call full_clean(). Plugin writers doing a
>> lot of object creation without involving DRF serializers should call
>> full_clean() either in application code or their specific model's save()
>> method. Here's some recap about the motivations for this:
>>
>> * By having full_clean() called with all model save it gives Pulp "two
>> validation layers" when there only needs to be one. DRF's validation layer
>> is the serializer, and it isn't prepared to do error handling from a
>> "second" layer. This is partly an echo of the concerns from Tom Christie's
>> writing.
>> * DRF is primarily designed to have data flow through its serializers.
>> This issue is more problematic in cases where data is not flowing through
>> the serializer. Thus we should try to include the serializer whenever
>> possible.
>> * If we cannot include the serializer, those are cases where we would
>> specifically benefit from calling full_clean manually.
>>
>> Other thoughts and concerns are welcome. If nothing major comes up then
>> we can close https://pulp.plan.io/issues/5828 as WONTFIX.
>>
>>
>>
>> On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz  wrote:
>>
>>> On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
>>> >If anyone has concerns with us enabling Model validation by default
>>> on
>>> >all models please let us know soon.
>>>
>>> I don't know (yet) if I have concerns, but DRF seems to have, see
>>>
>>> https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform
>>>
>>> According to DRF's design, all validation logic should be at one
>>> place, which is the serializer.  This seems to be a controversial
>>> issue, see e.g.
>>> https://github.com/encode/django-rest-framework/issues/3144.  From
>>> that issue:
>>>
>>> What happens in the case where in your models you are forcing a
>>> full_clean (perhaps by including it in the save method)?  Will
>>> serializers know how to handle an exception from an explicit
>>> full_clean?
>>>
>>> And Tom Christie's answer:
>>>
>>> I'd recommend that application level validation should generally
>>> happen prior to save, not during it.  Personally I'd avoid
>>> full_clean, and instead ensure that state changing operations on
>>> model instances are only ever made via method calls that can provide
>>> a boundary that ensures that only valid state changes may ever be
>>> made by the rest of the application.
>>>
>>>
>>>
>>> We need to be aware that we are leaving the path recommended by DRF
>>> if we implement this proposal and mix Django validation and DRF
>>> validation.  Unfortunately, I don't know what the alternative is.
>>> Using DRF serializers to construct all model instances looks clumsy
>>> when it comes to relations.
>>>
>>> ___
>> Pulp-dev mailing list
>> Pulp-dev@redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[David Davis]

This makes sense to me. I wonder if we should document what you've outlined
in the plugin writers guide? If so, then maybe we should repurpose 5828?

David


On Tue, Dec 3, 2019 at 12:14 PM Brian Bouterse  wrote:

> After some IRC discussion during open floor, I think the consensus is that
> we should not have BaseModel call full_clean(). Plugin writers doing a lot
> of object creation without involving DRF serializers should call
> full_clean() either in application code or their specific model's save()
> method. Here's some recap about the motivations for this:
>
> * By having full_clean() called with all model save it gives Pulp "two
> validation layers" when there only needs to be one. DRF's validation layer
> is the serializer, and it isn't prepared to do error handling from a
> "second" layer. This is partly an echo of the concerns from Tom Christie's
> writing.
> * DRF is primarily designed to have data flow through its serializers.
> This issue is more problematic in cases where data is not flowing through
> the serializer. Thus we should try to include the serializer whenever
> possible.
> * If we cannot include the serializer, those are cases where we would
> specifically benefit from calling full_clean manually.
>
> Other thoughts and concerns are welcome. If nothing major comes up then we
> can close https://pulp.plan.io/issues/5828 as WONTFIX.
>
>
>
> On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz  wrote:
>
>> On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
>> >If anyone has concerns with us enabling Model validation by default
>> on
>> >all models please let us know soon.
>>
>> I don't know (yet) if I have concerns, but DRF seems to have, see
>>
>> https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform
>>
>> According to DRF's design, all validation logic should be at one
>> place, which is the serializer.  This seems to be a controversial
>> issue, see e.g.
>> https://github.com/encode/django-rest-framework/issues/3144.  From
>> that issue:
>>
>> What happens in the case where in your models you are forcing a
>> full_clean (perhaps by including it in the save method)?  Will
>> serializers know how to handle an exception from an explicit
>> full_clean?
>>
>> And Tom Christie's answer:
>>
>> I'd recommend that application level validation should generally
>> happen prior to save, not during it.  Personally I'd avoid
>> full_clean, and instead ensure that state changing operations on
>> model instances are only ever made via method calls that can provide
>> a boundary that ensures that only valid state changes may ever be
>> made by the rest of the application.
>>
>>
>>
>> We need to be aware that we are leaving the path recommended by DRF
>> if we implement this proposal and mix Django validation and DRF
>> validation.  Unfortunately, I don't know what the alternative is.
>> Using DRF serializers to construct all model instances looks clumsy
>> when it comes to relations.
>>
>> ___
> Pulp-dev mailing list
> Pulp-dev@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[Brian Bouterse]

After some IRC discussion during open floor, I think the consensus is that
we should not have BaseModel call full_clean(). Plugin writers doing a lot
of object creation without involving DRF serializers should call
full_clean() either in application code or their specific model's save()
method. Here's some recap about the motivations for this:

* By having full_clean() called with all model save it gives Pulp "two
validation layers" when there only needs to be one. DRF's validation layer
is the serializer, and it isn't prepared to do error handling from a
"second" layer. This is partly an echo of the concerns from Tom Christie's
writing.
* DRF is primarily designed to have data flow through its serializers. This
issue is more problematic in cases where data is not flowing through the
serializer. Thus we should try to include the serializer whenever possible.
* If we cannot include the serializer, those are cases where we would
specifically benefit from calling full_clean manually.

Other thoughts and concerns are welcome. If nothing major comes up then we
can close https://pulp.plan.io/issues/5828 as WONTFIX.



On Mon, Dec 2, 2019 at 6:52 PM Simon Baatz  wrote:

> On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
> >If anyone has concerns with us enabling Model validation by default on
> >all models please let us know soon.
>
> I don't know (yet) if I have concerns, but DRF seems to have, see
>
> https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform
>
> According to DRF's design, all validation logic should be at one
> place, which is the serializer.  This seems to be a controversial
> issue, see e.g.
> https://github.com/encode/django-rest-framework/issues/3144.  From
> that issue:
>
> What happens in the case where in your models you are forcing a
> full_clean (perhaps by including it in the save method)?  Will
> serializers know how to handle an exception from an explicit
> full_clean?
>
> And Tom Christie's answer:
>
> I'd recommend that application level validation should generally
> happen prior to save, not during it.  Personally I'd avoid
> full_clean, and instead ensure that state changing operations on
> model instances are only ever made via method calls that can provide
> a boundary that ensures that only valid state changes may ever be
> made by the rest of the application.
>
>
>
> We need to be aware that we are leaving the path recommended by DRF
> if we implement this proposal and mix Django validation and DRF
> validation.  Unfortunately, I don't know what the alternative is.
> Using DRF serializers to construct all model instances looks clumsy
> when it comes to relations.
>
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[Simon Baatz]

On Mon, Dec 02, 2019 at 03:53:06PM -0500, Brian Bouterse wrote:
>If anyone has concerns with us enabling Model validation by default on
>all models please let us know soon.

I don't know (yet) if I have concerns, but DRF seems to have, see
https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform

According to DRF's design, all validation logic should be at one
place, which is the serializer.  This seems to be a controversial
issue, see e.g. 
https://github.com/encode/django-rest-framework/issues/3144.  From
that issue:

What happens in the case where in your models you are forcing a
full_clean (perhaps by including it in the save method)?  Will
serializers know how to handle an exception from an explicit
full_clean?

And Tom Christie's answer:

I'd recommend that application level validation should generally
happen prior to save, not during it.  Personally I'd avoid
full_clean, and instead ensure that state changing operations on
model instances are only ever made via method calls that can provide
a boundary that ensures that only valid state changes may ever be
made by the rest of the application.



We need to be aware that we are leaving the path recommended by DRF
if we implement this proposal and mix Django validation and DRF
validation.  Unfortunately, I don't know what the alternative is. 
Using DRF serializers to construct all model instances looks clumsy
when it comes to relations.


___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[David Davis]

I'm on it.

David


On Mon, Dec 2, 2019 at 4:26 PM Brian Bouterse  wrote:

>
>
> On Mon, Dec 2, 2019 at 3:53 PM David Davis  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 
>> 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 
>>> 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 <
 ttere...@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 
> 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.agu...@redhat.com> wrote:
>>
>>> the best way that I found so far is this:
>>> https://stackoverflow.com/a/56272674/5253051
>>> [image: image.png]4
>>> 
>>>
>>> Best regards,
>>> Fabricio Aguiar
>>> Software Engineer, Pulp Project
>>> Red Hat Brazil - Latam 
>>> +55 11 999652368
>>>
>>>
>>> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
>>> ttere...@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.

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

[Brian Bouterse]

On Mon, Dec 2, 2019 at 3:53 PM David Davis  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 
> 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 
>> 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 <
>>> ttere...@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 
 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.agu...@redhat.com> wrote:
>
>> the best way that I found so far is this:
>> https://stackoverflow.com/a/56272674/5253051
>> [image: image.png]4
>> 
>>
>> Best regards,
>> Fabricio Aguiar
>> Software Engineer, Pulp Project
>> Red Hat Brazil - Latam 
>> +55 11 999652368
>>
>>
>> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
>> ttere...@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 

Re: [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 
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  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 
>> 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 
>>> 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.agu...@redhat.com> wrote:

> the best way that I found so far is this:
> https://stackoverflow.com/a/56272674/5253051
> [image: image.png]4
> 
>
> Best regards,
> Fabricio Aguiar
> Software Engineer, Pulp Project
> Red Hat Brazil - Latam 
> +55 11 999652368
>
>
> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
> ttere...@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@redhat.com
>> 

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

[Brian Bouterse]

On Mon, Dec 2, 2019 at 3:40 PM Tatiana Tereshchenko 
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
>
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
fields.

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.
>
Agreed

>
> 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  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 
>> 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 
>>> 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.agu...@redhat.com> wrote:

> the best way that I found so far is this:
> https://stackoverflow.com/a/56272674/5253051
> [image: image.png]4
> 
>
> Best regards,
> Fabricio Aguiar
> Software Engineer, Pulp Project
> Red Hat Brazil - Latam 
> +55 11 999652368
>
>
> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
> ttere...@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 

Re: [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  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 
> 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 
>> 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.agu...@redhat.com> wrote:
>>>
 the best way that I found so far is this:
 https://stackoverflow.com/a/56272674/5253051
 [image: image.png]4
 

 Best regards,
 Fabricio Aguiar
 Software Engineer, Pulp Project
 Red Hat Brazil - Latam 
 +55 11 999652368


 On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
 ttere...@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@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
 ___
 Pulp-dev mailing list
 Pulp-dev@redhat.com
 https://www.redhat.com/mailman/listinfo/pulp-dev

>>> ___
>> Pulp-dev mailing list
>> Pulp-dev@redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
>

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

[Brian Bouterse]

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 
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 
> 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.agu...@redhat.com> wrote:
>>
>>> the best way that I found so far is this:
>>> https://stackoverflow.com/a/56272674/5253051
>>> [image: image.png]4
>>> 
>>>
>>> Best regards,
>>> Fabricio Aguiar
>>> Software Engineer, Pulp Project
>>> Red Hat Brazil - Latam 
>>> +55 11 999652368
>>>
>>>
>>> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <
>>> ttere...@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@redhat.com
 https://www.redhat.com/mailman/listinfo/pulp-dev

>>> ___
>>> Pulp-dev mailing list
>>> Pulp-dev@redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>> ___
> Pulp-dev mailing list
> Pulp-dev@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[Tatiana Tereshchenko]

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  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.agu...@redhat.com> wrote:
>
>> the best way that I found so far is this:
>> https://stackoverflow.com/a/56272674/5253051
>> [image: image.png]4
>> 
>>
>> Best regards,
>> Fabricio Aguiar
>> Software Engineer, Pulp Project
>> Red Hat Brazil - Latam 
>> +55 11 999652368
>>
>>
>> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko 
>> 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@redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>> ___
>> Pulp-dev mailing list
>> Pulp-dev@redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[David Davis]

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 
wrote:

> the best way that I found so far is this:
> https://stackoverflow.com/a/56272674/5253051
> [image: image.png]4
> 
>
> Best regards,
> Fabricio Aguiar
> Software Engineer, Pulp Project
> Red Hat Brazil - Latam 
> +55 11 999652368
>
>
> On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko 
> 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.py#L440-L463
>> ___
>> Pulp-dev mailing list
>> Pulp-dev@redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
> ___
> Pulp-dev mailing list
> Pulp-dev@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[Fabricio Aguiar]

the best way that I found so far is this:
https://stackoverflow.com/a/56272674/5253051
[image: image.png]4


Best regards,
Fabricio Aguiar
Software Engineer, Pulp Project
Red Hat Brazil - Latam 
+55 11 999652368


On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko 
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.py#L440-L463
> ___
> Pulp-dev mailing list
> Pulp-dev@redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

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

[Tatiana Tereshchenko]

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.py#L440-L463
___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev