Legacy rails ruby #1
Conversation
Conflicts: lib/positioning.rb lib/positioning/mechanisms.rb test/test_positioning.rb
📝 WalkthroughWalkthroughBroad downgrades for legacy compatibility: CI matrix narrowed to Ruby 2.7.8, Rails ~> 4.2, Postgres only. Gemspec and Gemfile dependency constraints reduced. Core positioning logic adjusted for older AR/Rails APIs (selection, change tracking, range queries). Tests updated for compatibility and logging enabled. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/support/active_record.rb (1)
115-115: Make SQL logging opt-in to keep CI output and runtime lean.Always-on SQL logs can drown CI; gate via env.
Apply:
-ActiveRecord::Base.logger = ActiveSupport::Logger.new($stdout) +if ENV["SQL_LOG"] == "1" + logger = ActiveSupport::Logger.new($stdout) + logger.level = Logger::INFO + ActiveRecord::Base.logger = logger +endpositioning.gemspec (1)
31-38: Alphabetize runtime deps (and optionally dev deps) to satisfy Gemspec/OrderedDependencies.Minor style nit: place
activerecordbeforeactivesupport. The diff above covers runtime deps; consider alphabetizing dev deps as well if you keep expanding this list.lib/positioning.rb (1)
50-53: Setter dirty tracking: consider dropping the explicitwill_change!.After
super(position), Rails’ writer already marks the attribute dirty when the value changes. Callingwill_change!post-assignment can record an incorrect “was” value in some versions and is likely unnecessary now that retrieval usespluck. Simplify:- # Flipped order here to make rails 4 happy - super(position) - send :"#{column}_will_change!" + # Rely on default writer for dirty tracking (Rails 4–7) + super(position)If you still need explicit dirty marking for edge cases, guard it with a version check and call it before assignment only where required.
lib/positioning/mechanisms.rb (2)
112-119: Prefer Arel over string SQL; also quiets Brakeman.Build predicates with Arel and wrap update_all expressions with Arel.sql to make intent explicit and reduce false positives.
Apply this diff:
- if range.end.nil? # This branch for rails < 6 - scope.where("#{quoted_column} >= ?", range.begin).update_all "#{quoted_column} = #{quoted_column} * -1" - else - scope.where("#{@column}": range).update_all "#{quoted_column} = #{quoted_column} * -1" - end - - scope.where("#{quoted_column} <= ?", -1).update_all "#{quoted_column} = #{quoted_column} * -1 + 1" + arel_col = base_class.arel_table[@column] + if range.end.nil? # rails < 6 (endless range) + scope.where(arel_col.gteq(range.begin)).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1") + else + scope.where(@column => range).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1") + end + + scope.where(arel_col.lteq(-1)).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1 + 1")
122-131: Mirror the Arel refactor in contract.Keep both paths consistent and scanner-friendly.
Apply this diff:
- if range.end.nil? # This branch for rails < 6 - scope.where("#{quoted_column} >= ?", range.begin).update_all "#{quoted_column} = #{quoted_column} * -1" - else - scope.where("#{@column}": range).update_all "#{quoted_column} = #{quoted_column} * -1" - end - - scope.where("#{quoted_column} <= ?", -1).update_all "#{quoted_column} = #{quoted_column} * -1 - 1" + arel_col = base_class.arel_table[@column] + if range.end.nil? # rails < 6 (endless range) + scope.where(arel_col.gteq(range.begin)).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1") + else + scope.where(@column => range).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1") + end + + scope.where(arel_col.lteq(-1)).update_all Arel.sql("#{quoted_column} = #{quoted_column} * -1 - 1")test/test_positioning.rb (1)
1490-1491: Stabilize equality assertions across AR versions.Relation equality can vary; compare arrays to avoid surprises.
Apply this diff:
- assert_equal list.authors, models # TODO: if broken, add to_a for older rails - assert_equal other_list.authors, other_models # TODO: if broken, add to_a for older rails + assert_equal list.authors.to_a, models + assert_equal other_list.authors.to_a, other_models
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
.github/workflows/main.yml(1 hunks)Gemfile(1 hunks)lib/positioning.rb(2 hunks)lib/positioning/healer.rb(1 hunks)lib/positioning/mechanisms.rb(3 hunks)positioning.gemspec(2 hunks)test/models/category.rb(1 hunks)test/models/post.rb(1 hunks)test/support/active_record.rb(1 hunks)test/test_positioning.rb(7 hunks)
🧰 Additional context used
🪛 RuboCop (1.76.1)
positioning.gemspec
[convention] 32-32: Dependencies should be sorted in an alphabetical order within their section of the gemspec. Dependency activerecord should appear before activesupport.
(Gemspec/OrderedDependencies)
🪛 ast-grep (0.38.6)
lib/positioning/mechanisms.rb
[warning] 112-112: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: scope.where("#{quoted_column} >= ?", range.begin)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 125-125: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: scope.where("#{quoted_column} >= ?", range.begin)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Brakeman (7.0.2)
lib/positioning/mechanisms.rb
[weak] 118-118: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
[weak] 128-128: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
[weak] 113-113: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
[weak] 115-115: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
[weak] 131-131: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
[weak] 126-126: Possible SQL injection
Type: SQL Injection
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Ruby 2.7.8, DB postgresql, Rails ~> 4.2.0
🔇 Additional comments (14)
.github/workflows/main.yml (1)
18-20: Verify Rails 4.2 on Ruby 2.7 in ubuntu-latest; consider pinning Postgres.
Rails 4.2 on Ruby 2.7 can be brittle and GitHub’s ubuntu-latest image may ship varying libpq versions. Manually confirm on a fresh runner by installing Bundler and runninggem install bundler bundle install --jobs=3 --retry=3 bundle exec rake -TIf you see intermittent failures, add a Postgres service container (e.g.,
postgres:12) to lock in headers/libs.test/models/post.rb (1)
2-2: LGTM: removal ofoptional: trueis necessary for Rails 4.2 compatibility.Ensure fixtures/factories always provide a Blog when creating Post records under this suite.
test/models/category.rb (1)
2-2: LGTM: aligns association API with Rails 4.2 (nooptional:keyword).Double-check any tests that relied on
parentbeing nil to avoid unintended validation assumptions.lib/positioning/healer.rb (1)
11-12: Reselect → unscope+select+distinct looks correct; confirm AR 4.2 method coverage and no behavior drift.
- Semantics match the intent of reselect while remaining Rails 4–safe. Dropping order via
unscope(:order)is fine here since grouping bydistinctdoesn’t rely on ordering.- Please verify on AR 4.2 that:
Relation#unscope(:select)andRelation#distinctare present,ActiveRecord::Base#sliceis available (used later to build thewherehash),Relation#lock!behaves as expected in your target versions (Rails typically useslockon relations andlock!on records).Would you like a small script to assert these capabilities against your CI matrix?
positioning.gemspec (2)
12-12: Ruby 2.7.8 × Rails 4.2 compatibility risks—please validate.Rails 4.2 predates Ruby 2.7 and may hit keyword-argument and BigDecimal changes. Ensure your CI truly runs Rails 4.2 on Ruby 2.7.8 without patch gems.
I can provide a quick script to print
RUBY_VERSION,ActiveRecord::VERSION::STRING, and run a smoke query if helpful.
33-38: Recheck dev dependency versions for Ruby 2.7.
- bigdecimal "~> 1.4.0" may conflict with Ruby 2.7’s stdlib BigDecimal (2.x). You may not need this pin on 2.7.
- pg "< 1.0": OK for Rails 4.2; consider narrowing to a known-good (e.g., "~> 0.21") if CI confirms.
- sqlite3 "~> 1.7.2": compatible with Ruby 2.7, but Rails 4.2 historically paired with older sqlite3; confirm test matrix is green.
If you want, I’ll generate a Gemfile.lock sanity script to assert resolution on Ruby 2.7.8 across adapters.
lib/positioning.rb (1)
22-23: Base class guard is Rails 4–safe.
unless base_class == selfcorrectly mirrorsbase_class?for older Rails.lib/positioning/mechanisms.rb (2)
100-101: Compat: pick → limit(1).pluck(...).first is fine.Good fallback for AR 4.x without behavior change.
176-176: Compat: pluck-first for relative record is OK.Matches the position_was change and remains efficient.
test/test_positioning.rb (5)
106-107: Using map for cross-AR compatibility is fine.Slightly more object work, but OK in tests.
225-225: Compat: limit(1).pluck(...).first is fine.Aligns with library change.
515-515: Chained where for deterministic SQL shape.Good for stable to_sql assertions across AR versions.
525-528: Explicit where chaining: good.Makes the expected SQL order obvious.
1565-1565: previous_changes.include?(:list_id) is a reasonable cross-version check.Works on AR 4.x+. LGTM.
| end | ||
| when "postgresql" | ||
| gem "pg", "~> 1.5.5" | ||
| gem "pg", "< 1.0" # Decreased to enable compatibility |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tighten pg constraint; '< 1.0' is too broad and risky on Ruby 2.7/modern libpq.
Pin to a known Rails 4.2-compatible minor (e.g., 0.21.x) to avoid surprise downgrades, build issues, or security regressions. Consider gating by RAILS_VERSION if you plan to support newer Rails in other matrices later.
Proposed change:
- gem "pg", "< 1.0" # Decreased to enable compatibility
+ gem "pg", "~> 0.21.0" # Rails 4.2 compatibility; stable on common CI imagesQuick check script to validate availability and Ruby requirement metadata:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -s https://rubygems.org/api/v1/versions/pg.json \
| jq -r '[.[] | select(.number=="0.21.0")][0] | {version:.number, ruby_version}'Length of output: 216
Fix pg version constraint for Ruby 2.7 compatibility. (Gemfile:28) Version 0.21.0 declares a Ruby requirement of “>= 2.0 and < 2.5,” so pinning ~> 0.21.0 will fail under Ruby 2.7. Gate the 0.21.x constraint by Ruby or Rails version (e.g. only for Ruby < 2.5 or Rails 4.2) and allow pg >= 1.0 for Ruby >= 2.5.
🤖 Prompt for AI Agents
In Gemfile around line 28, the current pin "pg", "< 1.0" breaks Ruby 2.7 because
pg 0.21.x requires Ruby < 2.5; change the declaration to gate the 0.21.x
constraint to older Rubies/Rails and allow pg >= 1.0 for newer runtimes.
Concretely, detect Ruby (or Rails) version in the Gemfile (e.g. if RUBY_VERSION
< "2.5" or if Rails.version starts_with?("4.2")), then add the legacy pin for
"~> 0.21.0" (or "< 1.0") inside that conditional, otherwise declare "pg", ">=
1.0"; keep only one gem declaration active so Bundler picks the compatible pg
for the running Ruby/Rails.
| spec.add_dependency "activesupport", ">= 4.2" # Decreased to enable compatibility | ||
| spec.add_dependency "activerecord", ">= 4.2" # Decreased to enable compatibility |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add upper bounds to Rails deps to prevent accidental installs on unsupported Rails versions.
Given the PR targets Rails ~> 4.2, leave headroom but cap major upgrades. Example below also satisfies the “ordered dependencies” RuboCop hint.
Apply:
- spec.add_dependency "activesupport", ">= 4.2" # Decreased to enable compatibility
- spec.add_dependency "activerecord", ">= 4.2" # Decreased to enable compatibility
+ spec.add_dependency "activerecord", ">= 4.2", "< 5.0" # Rails 4.x only
+ spec.add_dependency "activesupport", ">= 4.2", "< 5.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec.add_dependency "activesupport", ">= 4.2" # Decreased to enable compatibility | |
| spec.add_dependency "activerecord", ">= 4.2" # Decreased to enable compatibility | |
| spec.add_dependency "activerecord", ">= 4.2", "< 5.0" # Rails 4.x only | |
| spec.add_dependency "activesupport", ">= 4.2", "< 5.0" |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 32-32: Dependencies should be sorted in an alphabetical order within their section of the gemspec. Dependency activerecord should appear before activesupport.
(Gemspec/OrderedDependencies)
🤖 Prompt for AI Agents
In positioning.gemspec around lines 31-32, the Rails dependencies lack upper
bounds and may allow installs on unsupported major Rails versions; change
spec.add_dependency "activesupport", ">= 4.2" and spec.add_dependency
"activerecord", ">= 4.2" to include an upper bound like ">= 4.2", "< 5.0" for
each, and reorder the dependency lines alphabetically (activerecord before
activesupport) to satisfy the RuboCop ordered-dependencies hint.
| puts "THIS ONE" | ||
| @second_item.update list: @second_list, position: " 2" # NOTE: The same position it already had; adding hack to ensure rails sees this as a change remove when rails detects it right again | ||
| @third_item.update list: @second_list, position: 1 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove debug output and avoid string “hack” in tests.
- Drop puts to keep CI noise-free.
- Prefer fixing dirty-tracking in the setter over relying on " 2" to coerce a change.
Apply this diff in the test:
- puts "THIS ONE"
- @second_item.update list: @second_list, position: " 2" # NOTE: The same position it already had; adding hack to ensure rails sees this as a change remove when rails detects it right again
+ @second_item.update list: @second_list, position: 2If this fails on older AR due to change tracking, consider adjusting the library setter to mark the attribute dirty before write (so the test can stay clean). For example (outside this file):
# lib/positioning.rb (example)
def #{column}=(value)
attribute_will_change! :#{column} # call before assignment for AR 4.x
super(value)
endTo confirm no stray debug prints remain:
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*puts\s+' --type rbLength of output: 59
To verify that no stray puts debug statements remain, rerun the search with proper file-type matching:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search all Ruby files for debug output using 'puts'
rg -n '^\s*puts ' -g '*.rb'Length of output: 98
Remove debug output and string “hack” in the test
--- test/test_positioning.rb
+++ test/test_positioning.rb
@@ -810,3 +810,3 @@
- puts "THIS ONE"
- @second_item.update list: @second_list, position: " 2" # hack to force dirty-tracking
+ @second_item.update list: @second_list, position: 2Run the full test suite to ensure no failures. If this change breaks under older ActiveRecord versions (due to dirty-tracking), consider marking the attribute dirty in the setter (e.g., calling attribute_will_change! before assignment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| puts "THIS ONE" | |
| @second_item.update list: @second_list, position: " 2" # NOTE: The same position it already had; adding hack to ensure rails sees this as a change remove when rails detects it right again | |
| @third_item.update list: @second_list, position: 1 | |
| @second_item.update list: @second_list, position: 2 | |
| @third_item.update list: @second_list, position: 1 |
🤖 Prompt for AI Agents
In test/test_positioning.rb around lines 810 to 812, remove the debug puts "THIS
ONE" and replace the string-hack update (@second_item.update list: @second_list,
position: " 2") with a normal numeric assignment matching intent (e.g.,
position: 2), then run the full test suite; if tests fail on older ActiveRecord
due to dirty-tracking, update the model setter to call attribute_will_change!
(or the equivalent) before assigning the position so AR detects the change.
Summary by CodeRabbit