Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for non-PK columns #185

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tylerwillingham
Copy link

With this proposal, GlobalID::Identification would implement global_id_column to set the desired column for GlobalID to reference on a per-model basis

i.e.

class Person
  attr_accessor :id, :external_id

  global_id_column :external_id
end

Person.new(id: "id-value", external_id: "external-id-value").to_gid.to_s
# => gid://app/Person/external-id-value

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 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.
@tylerwillingham
Copy link
Author

A question I'm anticipating and open to solving is how do actually find the associated record?

I'm not sure how much we want to couple the solution inside of globalid to ActiveRecord. As-is, this would work in one of my applications because we use the friendly_id gem. I think this is easily achievable with where and find_by! but I don't know how agnostic from ActiveRecord this gem is intended to be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant