Add support for models which use non-primary key columns for lookup (i.e. external_id columns) #199
Conversation
lib/global_id/identification.rb
Outdated
| end | ||
| end | ||
|
|
||
| def global_id_method |
There was a problem hiding this comment.
Do we need this method? We can just set the column to be :id by default.
There was a problem hiding this comment.
👍 I updated the implementation to use the class_attribute extension from ActiveSupport and brought consistency to the naming
| if model_class.global_id_column == :id | ||
| model_class.find gid.model_id | ||
| else | ||
| model_class.find_by model_class.global_id_column => gid.model_id | ||
| end |
There was a problem hiding this comment.
💭 Simplifying on find_by (or find_by!) for ActiveRecord users feels nice, but I stopped short of doing that because I recall seeing that globalid doesn't want to make ORM assumptions
There was a problem hiding this comment.
It already does with the includes? I think we should be using find_by! as well to match the find.
Also, should locate_many be updated as well?
There was a problem hiding this comment.
@rafaelfranca good feedback 👍
I'm looking at implementing #locate_many and I'm a little stumped. Here's my hang-up:
The current implementation uses .find with an array of IDs for the default path. That's supported in ActiveRecord for configurable primary keys and composite primary keys, but what I'm missing is mimicking where!(model.global_id_column => array_of_ids) behavior.
I could take a page out of ActiveRecord's book and implement something similar to the totals checked in the private API of find_some... but I'm trying to be mindful of the risk of overloading BaseLocator
I'm going to take some time and come back to this over the next few days.
tylerwillingham
force-pushed
the
twilling/alternative-primary-keys-plus-locator
branch
2 times, most recently
from
57d724b to
13a4a69
Compare
tylerwillingham
force-pushed
the
twilling/alternative-primary-keys-plus-locator
branch
from
13a4a69 to
b551218
Compare
rafaelfranca
force-pushed
the
twilling/alternative-primary-keys-plus-locator
branch
from
b551218 to
bbce26c
Compare
This allows overriding the column used to define GIDs
Because of Identification.global_id_column, we can keep this internal API much cleaner and avoid mucking with hash state to pass around this column
`Model.primary_key` is actually irrelevant to this feature, the only thing that matters is the model method. For composite keys, or primary keys not named "id", we still get the important value from `Model#id`, the only time we don't is when we need to use a non-PK column.
…tribute This simplifies the implementation's API substantially by defining both instance and class methods, and rails/globalid already depends on ActiveSupport
rafaelfranca
force-pushed
the
twilling/alternative-primary-keys-plus-locator
branch
from
bbce26c to
763f230
Compare
tylerwillingham
force-pushed
the
twilling/alternative-primary-keys-plus-locator
branch
4 times, most recently
from
abb5ddd to
763f230
Compare
With this proposal,
GlobalID::Identificationwould implementglobal_id_columnto set the desired column for GlobalID to reference on a per-model basisi.e.
I think this is particularly useful for teams in the process of migrating off of auto-incremented primary keys, or are interested in using unsigned GIDs with some level of obscurity that's not attainable without starting to make a bigger change like actually implementing UUIDs as PKs
This PR extends beyond #185's initial implementation and add's support to the default locator to use these alternative columns for look-up