citusdata / citusdata/activerecord-multi-tenant
Bug: `ActiveRecord::Base.increment_counter` within a multi-tenant context, the counter column is set to `NULL` instead of being incremented
- Dominant language
- Ruby
- Stars
- 759
- Forks
- 113
- PR merge metrics
- No merged PRs in 30d
Description
## Environment
* Rails version: 7.2.x
* activerecord-multi-tenant version: Commit `0ac43aa98334a29ed3e5b33e0bcc5ee281f97254` (broken)
* activerecord-multi-tenant version: v2.4.0 release (works correctly)
* Ruby version: 3.4.2
* PostgreSQL version: 14+
## Description
When using `ActiveRecord::Base.increment_counter` within a multi-tenant context, the counter column is set to `NULL` instead of being incremented.
**IMPORTANT**: This is a **regression introduced in commit `0ac43aa98334a29ed3e5b33e0bcc5ee281f97254`** ("Support rails 7.2"). The released version 2.4.0 works correctly.
### Testing Results
✅ **v2.4.0 (release) with Rails 7.1**: WORKS
```sql
UPDATE "users" SET "failed_attempts" = COALESCE("failed_attempts", 0) + 1
WHERE "users"."tenant_id" = 1 AND "users"."id" = 1
```
❌ **Commit 0ac43aa98334a29ed3e5b33e0bcc5ee281f97254 with Rails 7.2**: BROKEN (same behavior)
```sql
UPDATE "users" SET "failed_attempts" = NULL
WHERE "users"."id" IN (
SELECT "users"."id" FROM "users"
WHERE "users"."tenant_id" = 1 AND "users"."id" = 1)
AND "users"."tenant_id" = 1
```
This indicates that changes made in the Support rails 7.22 PR (#239) introduced a regression.
## Expected Behavior
```ruby
User.increment_counter(:failed_attempts, user.id)
user.reload
# expected: failed_attempts should be 1
```
## Actual Behavior
```ruby
User.increment_counter(:failed_attempts, user.id)
user.reload
# actual: failed_attempts is NULL
```
## SQL Generated
The generated SQL shows the problem:
```sql
UPDATE "users" SET "failed_attempts" = NULL
WHERE "users"."id" IN (
SELECT "users"."id" FROM "users"
WHERE "users"."tenant_id" = 1 AND "users"."id" = 1
)
AND "users"."tenant_id" = 1
```
Instead of the expected:
```sql
UPDATE "users" SET "failed_attempts" = COALESCE("failed_attempts", 0) + 1
WHERE "users"."id" = 1 AND "users"."tenant_id" = 1
```
## Root Cause
I think, like in [issue 278](https://github.com/citusdata/activerecord-multi-tenant/issues/278), this lies within `lib/activerecord-multi-tenant/relation_extension.rb`, specifically the `update_all` override:
```ruby
def updates
return super if MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?
stmt = Arel::UpdateManager.new
stmt.table(table)
stmt.set Arel.sql(klass.send(:sanitize_sql_for_assignment, updates))
stmt.wheres = [generate_in_condition_subquery]
klass.connection.update(stmt, "#{klass} Update All").tap { reset }
end
```
When Rails 7.2's `increment_counter` calls `update_all`, the `sanitize_sql_for_assignment` method appears to be mishandling the increment expression, resulting in `NULL` assignment.
## Reproducer
I've created a minimal, self-contained reproducer script that demonstrates this issue. The script:
* Uses `bundler/inline` so no Gemfile is needed
* Creates its own database tables
* Demonstrates the bug
* Cleans up after itself
See attached `reproduce_increment_counter_bug.rb`. Please note, depending on your database setup, you may need to set the environment variables:
`PGHOST`, `PGUSER`, `PGPASSWORD`.
```ruby
### Running the reproducer:
```bash
# Create test database
createdb multi_tenant_test
# Run the script
ruby reproduce_increment_counter_bug.rb
# Cleanup (optional)
dropdb multi_tenant_test
```
### Expected output showing the bug
```
=== Setting up test data ===
Created user with failed_attempts: 0
=== Attempting to increment counter with increment_counter ===
UPDATE "users" SET "failed_attempts" = NULL WHERE ...
After increment_counter, failed_attempts: nil
❌ BUG REPRODUCED: failed_attempts is NULL instead of 1
```
I've included within the `gemfile do` a version showing how it used to work. To reproduce a working version, remove the comments from lines 32-34 and comment out lines 36-38. It sets the reproducer to Rails 7.1 and activerecord_multi_tenant v2.4.0 to show how it _should_ work.
## Impact
This breaks critical functionality that relies on counter increments:
* Devise's lockable module (failed login attempts)
* Any counter caches
* Custom counter implementations
## Regression Analysis
The regression was introduced between:
* **Working**: v2.4.0 (released version)
* **Broken**: Commit `0ac43aa98334a29ed3e5b33e0bcc5ee281f97254` (PR #239: "Support rails 7.22")
## Related PR
PR #239
The changes made for Rails 7.2 support appear to have broken `increment_counter` behavior on Rails 7.1 and 7.2.
## Impact
This regression blocks users from:
* Using the Rails 7.2 support added in PR #239
* Using commit `0ac43aa98334a29ed3e5b33e0bcc5ee281f97254` or any commits after it
* Applications using Devise's lockable module will see authentication failures
* Any application using counter caches will experience data corruption (NULL values)
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.