<div dir="ltr">I'm basically -1 for the reasons Jeff enumerated but if he is ok with this, I'm happy to go ahead with it.<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[Jeff]:<br><div>In classic relational modeling, using ID as the primary key is
common practice. Especially when ORMs are involved. The "id" added
by plugin writers is a natural key so naming it ID goes against
convention.</div></blockquote><div><br></div><div>This is echoed here, for further reading (though perhaps this article is overly simplified for our needs) in the sections "Key Fields" and "Prefixes and Suffixes (are bad)":<br><a href="https://launchbylunch.com/posts/2014/Feb/16/sql-naming-conventions/">https://launchbylunch.com/posts/2014/Feb/16/sql-naming-conventions/</a><br></div><div><br>Plugin writers could use a surrogate key instead (unless I am misunderstanding your example on Errata, Brian):<br><a href="http://www.vertabelo.com/blog/technical-articles/natural-and-surrogate-primary-keys">http://www.vertabelo.com/blog/technical-articles/natural-and-surrogate-primary-keys</a><br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[Jeff]:<br><div>Every field in base models used by plugins has
potential for name collisions. Where does it end? Every column
having a pulp_ or _ prefix? </div></blockquote><div><br></div><div>Exactly. How far should we obfuscate our code base to prevent namespace collisions in derivative work? As David already pointed out, if we want to do this for id, then maybe also for created and for last_updated. Will we want to continue on to adding pulp or _ before everything in Pulp (no)? It seems unnecessary. So are we sure it's needed in these three cases?<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[Austin]:<br><div>In Pulp 2, having id fields bit us really badly. The reason may have
been specific to Mongoengine, but my understanding is that it is bad
practice anyway.</div></blockquote><br></div><div>Now, I will give the caveat that there's no "one true way" to the naming conventions, there are proponents for pulp (app) prefix, _ prefix, or leaving the id alone, and as we already saw with the yaml file issue recently, sometimes it's better to break a convention than to create bigger problems elsewhere. If we really were burned by the id fields in Pulp 2, that's a valid enough reason to change approaches in Pulp 3.<br><br>Can anyone elaborate on what these problems were? Was it related to MongoDB being non-relational and not something we're likely to run into on relational PostgreSQL?<br></div><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[David]:<br><div><div>At the very least, it might be nice to document which field names are reserved on the Content model.</div></div></blockquote><br>+1 on this. We want to encourage plugin writers by making things easy for them, but there are relatively few instances where they would need to do a workaround here, so documentation may suffice. Either way we go, we should document reserved names clearly for plugin writers to be aware of potential namespacing issues (you know? just document the heck out of everything :P).<br><br></div><div>And on that note...happy weekend folks!<br><br></div><div>--Dana<br></div><div> <br></div></div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div>
<p style="font-weight:bold;margin:0;padding:0;font-size:14px;text-transform:uppercase;margin-bottom:0"><span>Dana</span> <span>Walker</span></p>
<p style="font-weight:normal;font-size:10px;margin:0px 0px 4px;text-transform:uppercase"><span>Associate Software Engineer</span><span style="font-weight:normal;color:#aaa;margin:0"></span></p>
<p style="font-weight:normal;margin:0;font-size:10px;color:#999"><a style="color:#0088ce;font-size:10px;margin:0;text-decoration:none;font-family:'overpass',sans-serif" href="https://www.redhat.com" target="_blank">Red Hat <span><br><br></span></a></p>
<table border="0"><tbody><tr><td width="100px"><a href="https://red.ht/sig" target="_blank"> <img src="https://www.redhat.com/files/brand/email/sig-redhat.png" width="90" height="auto"></a> </td>
</tr></tbody></table>
</div></div></div></div>
<br><div class="gmail_quote">On Fri, May 25, 2018 at 12:15 PM, Brian Bouterse <span dir="ltr"><<a href="mailto:bbouters@redhat.com" target="_blank">bbouters@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I wrote up a Redmine issue to make this change here: <a href="https://pulp.plan.io/issues/3704" target="_blank">https://pulp.plan.io/issues/37<wbr>04</a> Please look at it and groom it if it looks ready.<br></div><div><br></div><div>@jortel I especially want to make sure you're comfortable with this change.<br></div><div><br></div><div>If anyone is -1 on this change please reply saying so.<br></div><div><div class="h5"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 24, 2018 at 5:06 PM, Brian Bouterse <span dir="ltr"><<a href="mailto:bbouters@redhat.com" target="_blank">bbouters@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>+1 to using _id, _created, and _last_updated only on MasterModel. It looks like leading underscores for field names are fine: <a href="https://stackoverflow.com/a/25509372" target="_blank">https://stackoverflow.com/a/25<wbr>509372</a> That will resolve this issue. Also everyone can still use .pk instead of ._id because Django automatically makes a .pk attribute on every model that also acts as the primary key regardless of the primary key's name. Also I don't think we have this issue elsewhere, only because Content is so heavily subclassed so I don't think we'll do leading _ to field names in other places.<br></div><div><br></div><div>@jortel I don't understand what you wrote; maybe try explaining it some more? The field "id" a plugin writer would define would contain the data they are obligated to hold for that content type. They didn't select the name "id"; it's the name the designers of that content type selected when they modeled that content type. So the designers of Errata itself chose Errata.id and what data should go inside there.<br></div></div><div class="m_-1755250678670839337m_2165946936433141867HOEnZb"><div class="m_-1755250678670839337m_2165946936433141867h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 23, 2018 at 10:33 AM, Jeff Ortel <span dir="ltr"><<a href="mailto:jortel@redhat.com" target="_blank">jortel@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
In classic relational modeling, using ID as the primary key is
common practice. Especially when ORMs are involved. The "id" added
by plugin writers is a natural key so naming it ID goes against
convention. Every field in base models used by plugins has
potential for name collisions. Where does it end? Every column
having a pulp_ or _ prefix? Plugins create relatively few tables
and it doesn't seem unreasonable for plugin writers to select other
names to resolve naming conflicts.<div><div class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123h5"><br>
<br>
<div class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123m_-4443883462590796228moz-cite-prefix">On 05/23/2018 07:50 AM, Brian Bouterse
wrote:<br>
</div>
</div></div><blockquote type="cite"><div><div class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123h5">
<div dir="ltr">
<div>Currently the Content model [0] has 'id' as it's primary
key which is inherited from MasterModel here [1]. By naming
our pk 'id', we are preventing plugin writers from also using
that field. That field name is common for content types. For
example: both RPM and Nuget content also expect to use the
'id' field to store data about the content type itself (not
Pulp's pk). We learned about the Nuget incompatibility at
ConfigMgmgtCamp from a community member. I learned about this
issue with RPM from @dalley.<br>
</div>
<div><br>
</div>
<div>The only workaround a plugin writer has is to call their
field 'rpm_id' or something like that. I don't see how it's
unavoidable that this renaming won't be passed directly onto
the user for things like filtering, creating units, etc. I
think that is an undesirable outcome just so that the Pulp pk
can be named 'id'.</div>
<div><br>
</div>
<div>One option would be to rename 'id' to 'pulp_id' at the
MasterModel. This is also somewhat ugly for Pulp developers,
but it would be (a) crystal clear to the user in all cases and
(b) allow Content writers to model their content types
correctly.</div>
<div><br>
</div>
<div>Another option would be to rename the pk for 'Content'
specifically and not at the MasterModel level. I think that
would create more confusion than benefit so I recommend doing
it at the MasterModel level.<br>
</div>
<div><br>
</div>
<div>What do you all think?</div>
<div><br>
</div>
<div>[0]: <a href="https://github.com/pulp/pulp/blob/6f492ee8fac94b8562dc62d87e6886869e052e7e/pulpcore/pulpcore/app/models/content.py#L106" target="_blank">https://github.com/pulp/pulp/b<wbr>lob/6f492ee8fac94b8562dc62d87e<wbr>6886869e052e7e/pulpcore/pulpco<wbr>re/app/models/content.py#L106</a><br>
</div>
<div>[1]: <a href="https://github.com/pulp/pulp/blob/d1dc089890f167617fe9917af087d5587708296b/pulpcore/pulpcore/app/models/base.py#L25" target="_blank">https://github.com/pulp/pulp/b<wbr>lob/d1dc089890f167617fe9917af0<wbr>87d5587708296b/pulpcore/pulpco<wbr>re/app/models/base.py#L25</a></div>
<div><br>
</div>
<div>-Brian<br>
</div>
</div>
<br>
<fieldset class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123m_-4443883462590796228mimeAttachmentHeader"></fieldset>
<br>
</div></div><span><pre>______________________________<wbr>_________________
Pulp-dev mailing list
<a class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123m_-4443883462590796228moz-txt-link-abbreviated" href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a>
<a class="m_-1755250678670839337m_2165946936433141867m_7602619907835113123m_-4443883462590796228moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pulp-dev" target="_blank">https://www.redhat.com/mailman<wbr>/listinfo/pulp-dev</a>
</pre>
</span></blockquote>
<br>
</div>
<br>______________________________<wbr>_________________<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<wbr>/listinfo/pulp-dev</a><br>
<br></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div>
<br>______________________________<wbr>_________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a><br>
<br></blockquote></div><br></div>