[Pulp-dev] 'id' versus 'pulp_id' on Content

Daniel Alley dalley at redhat.com
Mon Jun 11 18:45:17 UTC 2018 

I think we should just rename id -> _id and leave it at that.  It's the
only one that is immediately problematic and seems to have the highest
chance of collision.

If we also renamed last_updated -> _last_updated, it would be right next to
"last_synced" and "last_published" and would look inconsistent.  _id would
look fine because we also have _href.

@Jeff, What do you think?

On Mon, Jun 11, 2018 at 9:13 AM, Brian Bouterse <bbouters at redhat.com> wrote:

> @dalley that is correct for the Django behaviors I believe. With this
> change for the model plugin writers will subclass, it would have "pk" as
> reserved and "_id" since we define the primary key in the ancestor class
> they inherit from. It also would have "_created" and "_last_updated"
> reserved. This should cause minimal collisions with the plugin writer's
> defined fields.
>
> On Sat, Jun 9, 2018 at 1:51 AM, Daniel Alley <dalley at redhat.com> wrote:
>
>> Django reserves "pk" as a direct attribute, which  just maps to whichever
>> field is defined as the primary key, which by default is an "id" field.
>>
>> So "pk" is always reserved and "id" is reserved by default, unless you
>> overdid it by defining your own primary key field.
>>
>> On Fri, Jun 8, 2018, 4:57 PM Brian Bouterse <bbouters at redhat.com> wrote:
>>
>>> That's good testing. The info I got in #django said differently. Your
>>> test looks better.
>>>
>>> This means there is at least 1 field name that plugin writers just can't
>>> use, but the issue I've heard about was for 'id' not 'pk'. We are in
>>> control of that one, so I want to bring it back to that. This does clarify
>>> that .pk can always be used which should make renaming object.id to
>>> object._id even less of a pain since object.pk can always be used. +1
>>> to the convention @dalley recommended.
>>>
>>> On Fri, Jun 8, 2018 at 4:13 PM, David Davis <daviddavis at redhat.com>
>>> wrote:
>>>
>>>> I did a quick test and it looks like pk can't be used as a field name:
>>>>
>>>> pulp_app.HelloWorld.pk: (fields.E003) 'pk' is a reserved word that
>>>> cannot be used as a field name.
>>>>
>>>> This kind of leads me to believe that there are going to be certain
>>>> field names that plugin writers simply cannot use and trying to ameliorate
>>>> this situation isn’t going to work.
>>>>
>>>>
>>>> David
>>>>
>>>> On Fri, Jun 8, 2018 at 3:57 PM, Brian Bouterse <bbouters at redhat.com>
>>>> wrote:
>>>>
>>>>> @dalley: I agree we have practical reasons motivating the 3 field name
>>>>> changes, of which the most important is 'id' to '_id'. w.r.t using "
>>>>> object.pk", that could create similar problems. If a plugin writer
>>>>> defines a 'pk' field then core code using using 'object.pk' will
>>>>> cause core code to receive their attribute and not the primary key. I think
>>>>> overall the strategy I think to minimize collisions we should use
>>>>> 'object._id' directly. How does that sound?
>>>>>
>>>>> @jortel: We're blocked on your -1 vote expressed for 3704. We have
>>>>> practical plugin writer issues with the current state. Can you elaborate on
>>>>> why we shouldn't go forward with https://pulp.plan.io/issues/3704
>>>>>
>>>>> On Thu, Jun 7, 2018 at 1:52 PM, Daniel Alley <dalley at redhat.com>
>>>>> wrote:
>>>>>
>>>>>> The article[1] you mentioned states that 'ID' *should* be used for
>>>>>>> the PK
>>>>>>
>>>>>>
>>>>>> It does say this, but it says that the reasons for doing that are
>>>>>> because id is "short, simple, and unambiguous", and that the reason you
>>>>>> shouldn't prefix is because "the extra prefix is redundant".  I think it's
>>>>>> really good advice for the general case, but the reasoning is based in
>>>>>> practicality and not strong convention, and in our case we do have some
>>>>>> other practical reasons to not do this.
>>>>>>
>>>>>> I don't feel super strongly in either direction at this point.  I
>>>>>> think my personal preference is to change "id" to something else, and use a
>>>>>> convention of "object.pk" instead of "object.id".  The "pk"
>>>>>> attribute maps to whatever the primary key if we use that, we don't need to
>>>>>> care what the field is called.
>>>>>>
>>>>>> Re: Brian
>>>>>>
>>>>>> When encountered, what they expressed is "Why would Pulp reserve
>>>>>>> common field that I need to define on my subclass?" My feeling here is that
>>>>>>> we don't actually have a good reason.
>>>>>>>
>>>>>>
>>>>>> "id" is technically reserved (unless you override it) by Django,
>>>>>> since it is the default PK field.  If they were to directly subclass
>>>>>> `django.db.models.Model` they would have the same problem.  This is the
>>>>>> only reason I'm a little conflicted, otherwise I be 100% in favor of
>>>>>> changing it.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, May 29, 2018 at 12:46 PM, Jeff Ortel <jortel at redhat.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On 05/29/2018 08:24 AM, Brian Bouterse wrote:
>>>>>>>
>>>>>>> On Fri, May 25, 2018 at 7:39 PM, Dana Walker <dawalker at redhat.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> I'm basically -1 for the reasons Jeff enumerated but if he is ok
>>>>>>>> with this, I'm happy to go ahead with it.
>>>>>>>>
>>>>>>>> [Jeff]:
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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)":
>>>>>>>> https://launchbylunch.com/posts/2014/Feb/16/sql-naming-conventions/
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> That is true, but this article also talks about avoiding reserved
>>>>>>> words as well. I think we're hearing 'id' is a commonly reserved word for
>>>>>>> content types being modeled by plugin writers.
>>>>>>>
>>>>>>>
>>>>>>> The article[1] you mentioned states that 'ID' *should* be used for
>>>>>>> the PK which means it is inappropriate for natural key fields defined by
>>>>>>> plugin writers.  The reserved words caution in the article are DDL/DML
>>>>>>> reserved words "Ex: Avoid using words like user, lock, or table."
>>>>>>> not reserved by plugins.
>>>>>>>
>>>>>>> [1] https://launchbylunch.com/posts/2014/Feb/16/sql-naming-conve
>>>>>>> ntions/#primary-keys
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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/20180611/df6e05f4/attachment.htm>

More information about the Pulp-dev mailing list