Use alias_attribute to provide id_value alias for id attribute

[adrianna-chang-shopify]

Motivation / Background

Extends on #48533.

This allows access to the raw id column value on records for which an id column exists but is not the primary key. This is common amongst models with composite primary keys.

Detail

Use alias_attribute to provide a uid alias for the id attribute. That was one of the primary use cases described in the above pull request; as such, we've decided to provide it out of the box in Rails.

Additional information

Is there a better way to provide documentation for #uid than via a comment inserted above the alias_attribute call?

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

cc @nvasilevski @rafaelfranca

[matthewd]

I have a few concerns with this.

uid doesn't feel like a reasonable name for "exactly the column called id" -- it conveys no meaning, plus it reserves a different method name that people will already be using in their models (e.g. when referring to a POSIX user ID).

There are also plenty of other names that our predefined methods "reserve" -- we're surely not going to also predefine a usave and a uupdate etc.

It is our well-established convention that id is the name of the primary key. If an application has a column that is not (the whole) primary key but is named id, then that application should (honestly, rename the column... but failing that:) define an appropriate alias, just as they would for any other non-conventional column naming.

Finally, guaranteeing that every model has an alias defined prevents us from ever making performance optimizations for the extremely common case where a model does not have any aliases in use.

[adrianna-chang-shopify]

👋 Hey @matthewd , thanks for your feedback!

uid doesn't feel like a reasonable name

That's fair. We threw around some different options oid, id_value, id_column, etc. The problem, as you've stated already, is that these are either vague or are likely already be used in another context. We're used to id being the thing that means id. 😄

What we'd like to achieve here is providing applications with access to the raw id column when id is one part of a composite primary key. After discussion with @rafaelfranca we believed this use case was prevalent enough to warrant aliasing the attribute in Rails. Even read_attribute(:id) ends up giving you the primary key over the raw id column value (which we've talked about changing the behaviour of, but will take a deprecation cycle), so as it stands there isn't a good way to access the raw id value if a model has a CPK.

Finally, guaranteeing that every model has an alias defined prevents us from ever making performance optimizations

Also fair. We could provide a simple getter / setter for the id column that would access the raw column without using an alias, though then we don't get the <id>_was, <id>_changed, etc. attribute methods.

Anyways, I'll let Rafael weight in -- we can continue aliasing individual attributes in our application, so this isn't a blocker.

[rafaelfranca]

Although other reserved columns don't have readers and setters, it is totally possible to access them using read_attribute. It isn't with :id.

I believe renaming columns is out of question. Rails for years recommended, and still recommend people to create tables with an id column. As you application access grow, you might end up needing to use composite primary keys, that at this point the method id that used to return the value of the id column will stop doing that, and you don't have any alternative to get the id column other than regretting that Rails pushed you into a corner and now you either have to rename the column, which might not be realistic, or know you will need to add an alias because Rails again put you into a corner and the documented method to get the column value doesn't work, only for that column.

In my opinion this is a decision the framework needs to make if it is to add support to composite primary key. If I was going to design this from scratch I'd never have id returning the primary key, only the id column. Changing this a this point is almost impossible so this for me is the least of the evils.

I know this method will be another special method, but again, we can't design this properly given the restrictions. Any other name we though is worse than unique identifier. oid is object id, but has special meaning on Postgresql. I'd expect id_column to return the column object. id_value is ok as well.

[matthewd]

unique identifier

Given it's only part of the primary key, it is definitionally not unique. An application might do something more specific, but we can't imply that's reasonable to assume everywhere.

By the same logic, it's not an object identifier. I think _id or id_value are the only category of names that work -- ones that deliberately avoid imparting any further meaning. We should definitely be conscious of the fact that whatever name we select here, we're hard-breaking any application that contains a conflicting column: no deprecation, and exactly the same "even read_attribute won't work" limitation that we're trying to prevent.

---

I still think this is best addressed in-app, where one can apply a more contextually-aware name (e.g. if you know that the id column still contains a globally unique identifier, independent of the other components of the PK).

I definitely don't think the method(s)/alias should be there if there isn't an id column, though: that undercuts the whole idea that [unlike id] this is all about the actual id column. I'm tempted to claim it should only be present if there is an id column and that's not the configured primary key, to avoid it suddenly appearing as an additional did-you-mean option, etc, in every model of every application, but can probably be convinced on that point.

[adrianna-chang-shopify]

Okay, to summarize the feedback I'm hearing:

• id_value seems to be the best choice naming-wise
• Part of the need for such a method / alias comes from the fact that read_attribute(:id) doesn't give you back the id column. If we were to deprecate this behaviour (and move towards read_attribute(:id) returning the raw id column value), would that replace the need for this PR? @rafaelfranca would you be okay with us addressing this with an in-app alias while we change #read_attribute's behaviour in Rails?
• We should only provide an alias / method for models with an id column. Re:

I'm tempted to claim it should only be present if there is an id column and that's not the configured primary key

My only counter to the latter part of this statement is that it would prevent us from being able to use the alias/method on an object that may or may not have id configured as the PK. At Shopify, for example, we have a lot of instances of <record.id> calls, where record may be a model with a composite primary key, or a model with an id primary key. We'd need to either use responds_to?(:id_value) or check whether the model uses a CPK in order to know which message to send if we wanted to access the value in the id column (which we do in most instances).

Unless there's further feedback, my next steps will be to

• Rename the alias to id_value
• Figure out a way to alias only if the AR model contains an id attribute

[rafaelfranca]

I definitely don't think the method(s)/alias should be there if there isn't an id column, though: that undercuts the whole idea that [unlike id] this is all about the actual id column. I'm tempted to claim it should only be present if there is an id column and that's not the configured primary key, to avoid it suddenly appearing as an additional did-you-mean option, etc, in every model of every application, but can probably be convinced on that point.

I agree with that.

I still think this is best addressed in-app

I don't think this is an "application" concern. It is a framework design concern. The framework that should never had done the decision to make id special and not return the column. So I think it is the framework that should fix it.

The framework is officially going to support composite primary keys, but it feels like this feature isn't really officially supported. Users of it are on their own to figure out issues like this.

They will probably find this issue and ask "Hey, why I can't get the value of the id column anymore?". Then, they will have to figure out that the methods for id that the framework provides are meant to return anything, not only the id column. They will probably reach out to read_attribute(:id) just to find out that doesn't work as well. And if you think they will come up with the alias_attribute solution by themselves, I think they will reach to _read_attribute(:id) first.

We could document this case, but as a framework designer I'd not feel good in writing documentation that basically means: Hey, we screw up in the past, and you are the ones that will have to do work to fix my mistake.

We could do this properly as well an deprecate id as "I return whatever it is being used as primary key", but deprecations for this will be very annoying, and we would have to change a lot of thinks, like the reliance of id on GlobalID. If this deprecation wan't so annoying to fix, that would be my preferred course of action.

Maybe a per-model, allowing to be per app, configuration that means "I have no custom primary keys in my app, don't should this deprecation to me" would be good enough.

If we were to deprecate this behaviour (and move towards read_attribute(:id) returning the raw id column value)

A deprecation like this is tricky because there is no way to mark valid usage of read_attribute(:id) without a two phases deprecation. If an user call read_attribute(:id), expecting the value of the column id they will get an deprecation unless they change the call to something else. Most cases, that call is correct, and they should not see any deprecation, neither change their code, but there isn't a way deprecate without having legit users needed to change their code.

[rafaelfranca]

I think this is implemented in wrong place. It should be called in the same place that we call attribute :name for example. (I believe it is in the schema loader)

[adrianna-chang-shopify]

I wasn't entirely sure where you were suggesting this be moved 😅 I moved it to #define_attribute, since we define all of the attributes for a model when load_schema! is called. Made sense that we'd register the alias when defining the id attribute. Let me know what you think.

[adrianna-chang-shopify]

I agree with that.

@rafaelfranca can you clarify whether you're in favour of having this alias show up for all models with an id column, or only for models where the PK is not id?

Most cases, that call is correct, and they should not see any deprecation, neither change their code, but there isn't a way deprecate without having legit users needed to change their code.

I was envisioning that the deprecation would only show up if read_attribute(:id) was called on a model for which the primary key was not id, indicating to users that read_attribute(:id) would be changing to return the raw id value instead of the primary key. Is there a reason this wouldn't work?

[eileencodes]

In April I explored what it would look like to deprecate the primary key as id in favor of id returning the attribute and implementing a new primary_key_value method to get the pk. I don't love the name primary_key_value and at the time, alias_attribute seemed like a suitable solution. But maybe if more of us are starting to feel the special treatment if id is wrong, this will give an idea of what a deprecation would look/feel like.

FWIW I think my branch goes too far but it at least gives an idea of how invasive this is.

[rafaelfranca]

@adrianna-chang-shopify for all models with an id column.