-
-
Notifications
You must be signed in to change notification settings - Fork 523
chore(rails): remove 5.0 leftovers #2760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2760 +/- ##
=======================================
Coverage 97.24% 97.24%
=======================================
Files 130 130
Lines 5229 5229
=======================================
Hits 5085 5085
Misses 144 144
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Rails Version Compatibility Issue
The FILE_NAME case statement now returns nil for Rails versions below 5.2. Using this nil in require statements creates malformed paths, leading to LoadError exceptions. The resulting LoadError messages are unclear, not explicitly indicating an unsupported Rails version.
sentry-rails/spec/dummy/test_rails_app/app.rb#L25-L38
sentry-ruby/sentry-rails/spec/dummy/test_rails_app/app.rb
Lines 25 to 38 in cf8ee0b
| FILE_NAME = | |
| case Gem::Version.new(Rails.version) | |
| when ->(v) { v.between?(v5_2, v6_0) } | |
| "5-2" | |
| when ->(v) { v.between?(v6_0, v6_1) } | |
| "6-0" | |
| when ->(v) { v.between?(v6_1, v7_0) } | |
| "6-1" | |
| when ->(v) { v > v7_0 && v < v7_1 } | |
| "7-0" | |
| when ->(v) { v >= v7_1 } | |
| "7-1" | |
| end |
| case Gem::Version.new(Rails.version) | ||
| when ->(v) { v < v5_2 } | ||
| "5-0" | ||
| when ->(v) { v.between?(v5_2, v6_0) } | ||
| "5-2" | ||
| when ->(v) { v.between?(v6_0, v6_1) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Rails 7.0.0 incorrectly loads 6-1.rb config due to Gem::Version.between?'s inclusive upper bound, missing 7-0.rb specific configurations.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
When running with Rails 7.0.0, the version selection logic incorrectly matches v.between?(v6_1, v7_0) because Gem::Version.between? is inclusive of both endpoints. This causes the application to load configs/6-1.rb and apps/6-1.rb instead of the correct 7-0.rb files. Consequently, Rails 7.0-specific configurations, such as Zeitwerk::Registry.loaders.clear, are not loaded, leading to incorrect application behavior or test failures.
💡 Suggested Fix
Adjust the version comparison for Rails 7.0.0. Either modify v.between?(v6_1, v7_0) to exclude the upper bound (e.g., v.between?(v6_1, Gem::Version.new("6.999.999"))) or change the subsequent condition to v >= v7_0 && v < v7_1.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry-rails/spec/dummy/test_rails_app/app.rb#L27-L30
Potential issue: When running with Rails 7.0.0, the version selection logic incorrectly
matches `v.between?(v6_1, v7_0)` because `Gem::Version.between?` is inclusive of both
endpoints. This causes the application to load `configs/6-1.rb` and `apps/6-1.rb`
instead of the correct `7-0.rb` files. Consequently, Rails 7.0-specific configurations,
such as `Zeitwerk::Registry.loaders.clear`, are not loaded, leading to incorrect
application behavior or test failures.
Did we get this right? 👍 / 👎 to inform future reviews.
#skip-changelog