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

Open
#279 1 comment 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.