Skip to content

Fix update_all to correctly handle Arel nodes#22

Merged
a5-stable merged 1 commit intokufu:featuresfrom
rscq:fix/update-all-with-arel-nodes
Feb 19, 2026
Merged

Fix update_all to correctly handle Arel nodes#22
a5-stable merged 1 commit intokufu:featuresfrom
rscq:fix/update-all-with-arel-nodes

Conversation

@rscq
Copy link
Copy Markdown

@rscq rscq commented Feb 17, 2026

Summary

Fix update_all to correctly handle updates containing Arel nodes.

Background

The current update_all implementation serializes update values using sanitize_sql_for_assignment.
However, this method cannot convert Arel nodes (e.g., Arel::Nodes::Addition) to SQL, causing runtime errors in the following cases:

  • increment! / decrement! (internally generates Arel expressions like column + 1)
  • counter_culture gem (uses Arel expressions for counter updates)

This issue has also been reported in the upstream repository (citusdata#282).

Changes

Aligned with Rails' own update_all implementation by using _substitute_values for Hash updates.

  • Hash updates: Uses _substitute_values to preserve Arel nodes as-is and convert other values to bind parameters
  • String updates: Processed with sanitize_sql_for_assignment as before
  • Optimistic Locking: Reproduces the same lock_version auto-increment behavior as Rails

Reference: Rails implementation

https://github.com/rails/rails/blob/8-1-stable/activerecord/lib/active_record/relation.rb#L615-L625

Note

Due to the use of _substitute_values, the generated SQL now uses bind parameters ($1, $2...) instead of literal values.
Existing SQL string comparison tests have been adjusted accordingly.

Copy link
Copy Markdown
Collaborator

@a5-stable a5-stable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! It looks good to me.
The failing test doesn’t seem related to this PR. It should be resolved in #23

@a5-stable a5-stable merged commit 51439d3 into kufu:features Feb 19, 2026
74 of 80 checks passed
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.

2 participants