<div dir="ltr">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?<div><br></div><div>+1 to before GA.</div><div><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>David<br></div></div></div></div></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 3:41 PM Tatiana Tereshchenko <<a href="mailto:ttereshc@redhat.com">ttereshc@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Thanks for finding the cause of the lack of validation and for sharing the link to the docs.<div>Adding full_clean() solves the problem with UpdateRecord which I brought up.<br></div><div><div><br></div><div>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.</div><div>In addition, it can potentially affect data migration, if some required fields are missing.</div><div><br></div><div>To add it everywhere, we can override the `save` method for our base model <a href="https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/base.py#L9" target="_blank">https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/base.py#L9</a></div><div><br></div><div>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.</div><div>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.</div></div><div><br></div><div>What do you think? </div><div><br></div><div>1. Should we add validation for all the models or just selectively?</div><div>2. Before or after GA?</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 8:24 PM Brian Bouterse <<a href="mailto:bmbouter@redhat.com" target="_blank">bmbouter@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Thank you for bumping this thread.</div><div><br></div><div>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 ( <a href="https://docs.djangoproject.com/en/dev/ref/models/instances/?from=olddocs#django.db.models.Model.full_clean" target="_blank">https://docs.djangoproject.com/en/dev/ref/models/instances/?from=olddocs#django.db.models.Model.full_clean</a> ). 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.</div><div><br></div><div>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.</div><div><br></div><div>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.<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 10:20 AM Tatiana Tereshchenko <<a href="mailto:ttereshc@redhat.com" target="_blank">ttereshc@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Bump.<div>Any thoughts?</div><div><br></div><div>If we decide to change something, we'd better do it before GA, I think.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 18, 2019 at 10:07 PM David Davis <<a href="mailto:daviddavis@redhat.com" target="_blank">daviddavis@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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).<div><br></div><div>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?<br><div><div><div><div><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><br></div><div>David<br></div></div></div></div></div></div></div></div><br></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 18, 2019 at 3:15 PM Fabricio Aguiar <<a href="mailto:fabricio.aguiar@redhat.com" target="_blank">fabricio.aguiar@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">the best way that I found so far is this:<div><a href="https://stackoverflow.com/a/56272674/5253051" target="_blank">https://stackoverflow.com/a/56272674/5253051</a><br><div><img src="cid:ii_k34v8x550" alt="image.png" width="562" height="356">4<br></div><div><a href="https://stackoverflow.com/a/56272674/5253051" target="_blank"></a><br></div><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><br>Best regards,</div><div dir="ltr"><span style="color:rgb(0,0,0);font-family:RedHatText,sans-serif;font-size:14px;font-weight:700;text-transform:capitalize">Fabricio</span><span style="color:rgb(0,0,0);font-family:RedHatText,sans-serif;font-size:14px;font-weight:700;text-transform:capitalize"> </span><span style="color:rgb(0,0,0);font-family:RedHatText,sans-serif;font-size:14px;font-weight:700;text-transform:capitalize">Aguiar</span><div>Software Engineer, Pulp Project</div><div><a href="https://www.redhat.com/" style="color:rgb(0,136,206);font-family:RedHatText,sans-serif;font-size:12px;margin:0px" target="_blank">Red Hat Brazil - Latam</a><br></div><div>+55 11 999652368</div><div><img src="https://marketing-outfit-prod-images.s3-us-west-2.amazonaws.com/f5445ae0c9ddafd5b2f1836854d7416a/Logo-RedHat-Email.png" width="96" height="22"></div></div></div></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 18, 2019 at 5:09 PM Tatiana Tereshchenko <<a href="mailto:ttereshc@redhat.com" target="_blank">ttereshc@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I noticed that there is no enforcement at the DB level to require certain fields to be present on a model.<div><br></div><div>I haven't checked all the field types but at least for TextField it's seems to be true.</div><div>Even though `null` is False by default (`blank` as well), I can save a model instance without most of fields set.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Any suggestions how to handle that? Or do I miss anything?</div><div><br></div><div>Tanya</div><div><br></div><div>[0] <a href="https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/models/advisory.py#L77-L93" target="_blank">https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/models/advisory.py#L77-L93</a></div></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>
</blockquote></div>
</div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>
</blockquote></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>