Repository navigation
Conversation
(cherry picked from commit d2640b3)
The Rails 6.0 and 6.1 appraisals could not load a single spec on current
Ruby: ActiveSupport::LoggerThreadSafeLevel references ::Logger without
requiring it, and concurrent-ruby 1.3.5 stopped doing that require on its
behalf. Every spec file died with
NameError: uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger
before any example ran. This went unnoticed because CI was not actually
installing the appraisal gemfiles until the previous commit.
Rails 7.0.8.7 fixed this inside Active Support by adding the missing
require; 6.0 and 6.1 are end of life and will not get that fix, so the
spec helper does the require itself. With it, rails_6.1 and
active_record_6.1 run 102 examples with 0 failures locally.
Booting the dummy Rails application in spec_helper writes tmp/local_secret.txt (and tmp/development_secret.txt on older Rails) into the repository root. Those files are generated secrets and should never be committed.
generate_in_condition_subquery picks build_arel's arguments by version,
but only two shapes were handled: build_arel(connection) for >= 7.2 and
build_arel with no arguments for everything older. The real signatures are
6.0 build_arel(aliases)
6.1-7.1 build_arel(aliases = nil)
7.2-8.0 build_arel(connection, aliases = nil)
8.1 build_arel(aliases)
So on 6.0 every update_all / delete_all inside MultiTenant.with raised
ArgumentError (7 failing specs), and on 8.1 the connection was passed as
the aliases argument, failing with
NoMethodError: undefined method `[]=' for an instance of PostgreSQLAdapter
(3 failing specs). Passing the connection only on 7.2 and 8.0 and an
explicit nil everywhere else matches all four signatures.
rails_6.0 now passes 102/102; rails_7.1, 7.2 and 8.0 are unchanged at
102/102; Rails 8.1 goes from 4 failures to 1, which is a separate issue.
Rails 8.1 changed how the PostgreSQL visitor compiles update_all on a
relation with joins. Instead of
UPDATE t SET ... WHERE t.id IN (SELECT t.id FROM t JOIN ...)
it now emits an UPDATE ... FROM with a self-join on the primary key:
UPDATE "projects" "__active_record_update_alias" SET "name" = $1
FROM "projects" INNER JOIN "managers" ON ...
WHERE "projects"."id" = "__active_record_update_alias"."id"
Citus rejects that self-join because it is not on the distribution
column:
PG::FeatureNotSupported: ERROR: complex joins are only supported when
all distributed tables are co-located and joined on their distribution
columns
MultiTenant.with(record) goes through the gem's own update_all and was not
affected, but update_all without a tenant, or with the tenant given as an
id (which falls through to Rails' implementation), failed on 8.1. The
existing "updates the records without a current tenant" spec caught the
first case; this adds a spec for the second.
For multi-tenant models the self-join now also matches on the partition
key ("__active_record_update_alias"."account_id" = "projects"."account_id"),
which Citus can route, and which keeps the updated rows inside the tenant
when one is set. Older Rails never builds the alias, so nothing changes
there.
Rails 8.1: 103/103. rails_6.0, 7.0 and 8.0: 103/103.
Rails 8.1 removed the `alias` attribute from `Arel::Nodes::Function` (and its subclasses like NamedFunction, Count, etc.). Guard `visit obj.alias` calls with `respond_to?` checks to maintain compatibility with both Rails 8.0 and 8.1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit 2c62e1b)
The previous commit (from citusdata#284) stops the depth-first visitor calling #alias on Arel::Nodes::Function, which Rails 8.1 removed, but nothing in the suite reached that code: the tenant visitor skips SELECT projections, so Project.count and friends never touch a Function node. It only shows up when a function sits in a WHERE or HAVING clause. This spec puts a NamedFunction in WHERE and Sum/Count in HAVING inside MultiTenant.with. Without the citusdata#284 change it fails on Rails 8.1 with NoMethodError: undefined method `alias' for an instance of Arel::Nodes::NamedFunction and with it the spec passes on 8.1, 8.0, 7.0 and 6.1, and still checks that only the current tenant's rows come back.
Adds rails-8.1 and active-record-8.1 appraisals (gemfiles generated with
`appraisal generate`) and Ruby 3.4 to the CI matrix.
Now that each job really installs its appraisal gemfile, the matrix has
to skip combinations Rails does not support, or those jobs fail at
`bundle install` rather than telling us anything about this gem:
* Rails 7.2 requires Ruby >= 3.1
* Rails 8.0 and 8.1 require Ruby >= 3.2
* Rails 6.0 and 6.1 do not boot on Ruby 3.4 (they predate the
stdlib gems that 3.4 moved out of the default set)
Locally against Citus 14 on PostgreSQL 18, Ruby 3.4.11 passes the full
suite on rails_7.0, 7.1, 7.2, 8.0, 8.1 and active_record_8.1, and
rails_6.1 fails to load on 3.4, which is why it is excluded.
The static-checks job ran `gem install rubocop`, so every new RuboCop release could turn CI red without a line of this gem changing. On master, RuboCop 1.71 reports no offenses and the current release (1.91) reports 45, all from cops added or tightened in between. Pin to 1.71 (the newest release that is clean on this tree) in the Gemfile and run it through `bundle exec`, so the version only moves when someone bumps it on purpose and fixes what the new cops find. The job also ran setup-ruby with bundler-cache before checking out the repository, when there is no Gemfile yet to cache; checkout now comes first, and the job uses Ruby 3.4 rather than whatever "latest" means on the day. The appraisal gemfiles are regenerated to pick up the pin.
…ata#279 Both issues come from the same line. Since citusdata#223, update_all inside MultiTenant.with builds its SET clause with Arel.sql(klass.send(:sanitize_sql_for_assignment, updates)) sanitize_sql_for_assignment quotes every value in the hash, including Arel nodes, so anything that is meant to be SQL rather than a value gets type-cast into a literal: * citusdata#278: update_all(name: Arel.sql('UPPER(name)')) stores the string "UPPER(name)" instead of running UPPER. * citusdata#279: increment_counter / update_counters pass an Arel expression (COALESCE("replies_count", 0) + 1), which is cast to NULL, so the counter is wiped instead of incremented. This is what breaks Devise's lockable and counter caches. These specs reproduce both on Rails 6.1, 7.1, 7.2 and 8.1 (got "UPPER(name)" and nil respectively). They are marked pending so the suite stays green; the fix in the next commit removes the markers, and RSpec will fail loudly if a pending spec ever starts passing without that. Adds a replies_count integer column to comments to have a counter to increment.
[Patrick Donahue: cherry-picked from citusdata#282, which carried this commit from AmitSin's fork. It makes the tenant-scoped update_all build its SET clause the way Active Record's own update_all does: a hash goes through _substitute_values (Arel nodes are kept as SQL, Arel.sql is wrapped in a grouping, plain values are bound) and the optimistic locking column is bumped; only a string or array still goes through sanitize_sql_for_assignment. That fixes citusdata#278 and citusdata#279. Changes while picking: resolved the spec/schema.rb conflict with the replies_count column from the previous commit, and removed the pending markers from the citusdata#278 / citusdata#279 specs, which now pass on Rails 6.0, 6.1, 7.0, 7.2, 8.0 and 8.1.] (cherry picked from commit e1d0766)
(cherry picked from commit 31f9a1d)
With CI really installing each appraisal, every Rails 6.0 job passed
its specs and then failed the build on coverage:
Line coverage (65.15%) is below the expected minimum coverage (80.00%).
query_rewriter.rb only requires arel_visitors_depth_first.rb when
Arel::Visitors::DepthFirst is missing, which is Rails 6.1 and later. On
6.0 Arel still has it, so the gem's copy is never loaded and its 180
lines all show as uncovered. Filter that file out of the report when
Arel provides its own visitor; it is still measured on 6.1+, where it is
the code that actually runs.
rails_6.0 with CI=true: 83.92% line coverage (was 65.09%). Rails 8.1 is
unchanged at 84.98%.
…lize with Module#prepend Same issue as citusdata#216. `strong_migrations` overrides `ActiveRecord::SchemaDumper#initialize` using `Module#prepend`, which caused a `SystemStackError` when used with `activerecord-multi-tenant`. https://github.com/ankane/strong_migrations/blob/8e9baaa05c35fc9fdf206160c00b309c6e3a8bb6/lib/strong_migrations/schema_dumper.rb#L3-L7 Fix the issue by changing the `alias` in `activerecord-multi-tenant` to use `prepend`. (cherry picked from commit f2dc5bf)
Nothing in the suite exercised the schema dumper extension, so neither its output nor the SystemStackError fixed in the previous commit (citusdata#271) had a test. The first spec dumps the test schema and checks the create_distributed_table / create_reference_table lines, including a camel-case distribution column. The second reproduces the crash reported with strong_migrations: it starts a fresh Ruby process, prepends a module to ActiveRecord::SchemaDumper#initialize *before* requiring this gem (which is what strong_migrations does when it is earlier in the Gemfile), and dumps the schema. Against the old alias-based code it fails with -e:9:in `initialize': stack level too deep (SystemStackError) and with citusdata#271 it dumps normally. The subprocess is needed because the bug depends on load order, which cannot be recreated once the gem is loaded in the test process. Passes on rails_6.0, 7.0, 7.2, 8.0 and 8.1.
Add visit_Arel_SelectManager to MultiTenant::ArelVisitorsDepthFirst in reference to https://github.com/rails/rails/blob/1b3fc3c82e36a1c5f19f174e318166a11bd0b301/activerecord/lib/arel/visitors/to_sql.rb#L358-L361 (cherry picked from commit f8b7036)
The previous commit (citusdata#236) taught the gem's copy of the depth-first visitor to descend into an Arel::SelectManager, which fixes TypeError: Cannot visit Arel::SelectManager for a condition like `id IN (<arel subquery>)` inside MultiTenant.with. But that copy is only loaded on Rails 6.1 and later. Rails 6.0 still ships Arel::Visitors::DepthFirst, which ArelTenantVisitor inherits from there, and it has no SelectManager visitor either, so the spec added in citusdata#236 still failed on rails_6.0 with the same TypeError. Define visit_Arel_SelectManager on ArelTenantVisitor itself, which covers both base classes, and drop the now-redundant copy. rails_6.0, 7.0 and 8.1: 113 examples, 0 failures.
(cherry picked from commit a53dad9)
Follows the previous commit (citusdata#280), which brought the README up to Rails 8.0. The suite now passes on Rails 8.1 and Ruby 3.4, so say so, and spell out which Ruby/Rails combinations are tested so nobody has to read the workflow file to find out whether theirs is covered.
Nothing has been logged since 2.4.0, although master has carried Rails 7.2 and 8.0 support and a dozen fixes since late 2023. List those merged PRs together with the work on this branch, so the next release notes can be cut from this section instead of reconstructed from git log.
…ulti-tenancy (cherry picked from commit dbf398a)
|
tachyurgy please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This was referenced Oct 2, 2026
Open
Contributor
|
fantastic PR thanks tachyurgy! hope you are added as maintainer of the gem 🤞 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This gets master green on every Rails version from 6.0 to 8.1 (Ruby 3.0 to 3.4, Citus 10/11/12), so that a release can finally go out. #265 and #268 are people blocked on exactly that.
Most of it is work that was already sitting in open PRs here. I cherry-picked those with the original authors kept on the commits, and added what was still missing to get the suite passing.
From open PRs (authors credited on their commits)
update_allwithArel.sql,increment_counter/update_counters/increment!/decrement!insideMultiTenant.with(fixes Bug: Arel.sql in update_all becomes a literal value inside MultiTenant.with #278 and Bug:ActiveRecord::Base.increment_counterwithin a multi-tenant context, the counter column is set toNULLinstead of being incremented #279), plus bumping the lock columnSystemStackErrorin the schema dumper when another gem prepends toActiveRecord::SchemaDumper(also proposed in Replace Alias Method Chain With Prepend for SchemaDumper #245)TypeError: Cannot visit Arel::SelectManagerfor Arel subqueriesdelete_allfor models that are not multi-tenantNew in this PR
build_arelwas called with the wrong arguments on Active Record 6.0 and 8.1, soupdate_all/delete_allinsideMultiTenant.withraised on both. The four real signatures are(aliases)on 6.0,(aliases = nil)on 6.1 to 7.1,(connection, aliases = nil)on 7.2 and 8.0, and(aliases)again on 8.1.update_allasUPDATE ... FROMwith a self-join on the primary key, which Citus rejects ("complex joins are only supported when all distributed tables are co-located"). For multi-tenant models the self-join now also matches on the partition key, which Citus can route. Older Rails never builds that alias, so nothing changes there.SelectManagervisit from Fix TypeError: Cannot visit Arel::SelectManager when executing a subquery with Arel #236 also needed to apply on Rails 6.0update_allregressionsrequire 'logger'before Rails in the spec helper (needed on newer Ruby with Rails 6.x/7.0), RuboCop pinned so the static check is reproducibleUnreleasedsection listing all of the aboveEvery commit is small and runs green on its own, so it's easy to review one at a time or to drop anything you'd rather not take.
Green run on my fork: https://github.com/tachyurgy/activerecord-multi-tenant/actions/runs/36967652491
Since there is no release in the meantime, I've also published this branch as
activerecord-multi-tenant-nextso people stuck on Rails 7.2+ have something installable from RubyGems. SameMultiTenantAPI and require path. I'd much rather it go out here as 2.5.0, and I'm happy to help maintain this gem if that would be useful (more on #268).