fix: inherit from GlobalID's BaseLocator#318
Conversation
Version 1.4 of the globalid gem introduces a model_class method that brings deprecation warnings with it. Subclassing GlobalID's BaseLocator class solves that and additionally makes the locate_many method tenant-safe for consumers calling it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates ActiveRecord::Tenanted’s GlobalID integration to comply with GlobalID gem v1.4+ expectations by switching the custom locator to inherit from GlobalID::Locator::BaseLocator, eliminating deprecation warnings and aligning behavior with the upstream locator API.
Changes:
- Make
ActiveRecord::Tenanted::GlobalId::Locatorinherit fromGlobalID::Locator::BaseLocatorand implement#model_class(gid). - Delegate
#locate/#locate_manytosuperwhile enforcing tenant context safety checks. - Add unit tests for
#model_classandlocate_manytenant-safety behaviors.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/active_record/tenanted/global_id.rb | Switch locator to inherit from BaseLocator, add model_class, and add tenant validation for locate_many before delegating to super. |
| test/unit/global_id_test.rb | Add coverage for model_class and for locate_many behavior across tenant contexts (missing/wrong/none/correct). |
Comments suppressed due to low confidence (1)
lib/active_record/tenanted/global_id.rb:40
- Error message has a grammatical typo: "does not belong the current tenant" is missing "to", which may be surfaced to end users and makes the message harder to read.
if gid_tenant != current_tenant
raise WrongTenantError, "GlobalID #{gid.to_s.inspect} does not belong the current tenant #{current_tenant.inspect}"
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 175e026e1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def locate(gid, options = {}) | ||
| ensure_tenant_context_safety(gid) | ||
| gid.model_class.find(gid.model_id) | ||
| super |
There was a problem hiding this comment.
In apps whose bundle still resolves GlobalID 1.1 or older, this bare super forwards both gid and the default options hash to GlobalID::Locator::BaseLocator#locate, but those versions define locate(gid) with only one parameter. Since the gemspec does not require a newer GlobalID, any GlobalID::Locator.locate call in those apps will now raise ArgumentError instead of loading the record; either constrain the dependency to a version whose BaseLocator accepts options or avoid forwarding the second argument when the superclass cannot accept it.
Useful? React with 👍 / 👎.
| def locate(gid, options = {}) | ||
| ensure_tenant_context_safety(gid) | ||
| gid.model_class.find(gid.model_id) | ||
| ensure_tenant_context_safety(gid, gid.model_class) | ||
| super(gid) | ||
| end |
| def locate_many(gids, options = {}) | ||
| gids.each { |gid| ensure_tenant_context_safety(gid, gid.model_class) } | ||
| super | ||
| end |
Version 1.4 of the globalid gem expects custom locators to implement
model_class(gid)or inherit fromGlobalID::Locator::BaseLocator. Without that API, apps using GlobalID 1.4 or later emit deprecation warnings when resolving GIDs. A narrow fix for that deprecation warning is attempted in #317.This change makes
ActiveRecord::Tenanted::GlobalId::Locatorinherit fromGlobalID::Locator::BaseLocatorinstead, which additionally adds tenant validation tolocate_manybefore delegating tosuperfor consumers calling it.