Upgrade to Rails 8.1#784
Conversation
Test coverage89.32% line coverage reported by SimpleCov. |
07d29d2 to
ff7b90b
Compare
|
This is mostly done, there are some to_time warnings that I wanted to review (possibly fixed by enabling the setting in the upgrade file) |
For now I've just applied the regex timeout as it's a useful security improvement and is unlikely to cause issues.
Rails 8 introduces 'expect' that raises 400 errors when params are unexpected types rather than 500s.
Rails 8.1 sorts tables alphabetically
We don't use to_time directly anywhere and don't do much with timezones so this should be safe. Without it there are warnings about the change.
There was a problem hiding this comment.
Pull request overview
Upgrades the app from Rails 7.x to Rails 8.1, updating framework defaults, dependencies, and strong-parameter handling to match new Rails behavior and reduce parameter-related 500s.
Changes:
- Bump Rails to
~> 8.1and refreshGemfile.lockaccordingly. - Replace several
params.require(...).permit(...)usages withparams.expect(...). - Regenerate
db/schema.rbunder Rails 8.1 and add Rails 8 framework-default initializer + new bin scripts (bin/dev,bin/rubocop) and updatebin/setup.
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/requests/api_controller_spec.rb | Updates expected ParameterMissing message after Rails upgrade. |
| db/schema.rb | Rails 8.1 schema dump (version bump + alphabetical column ordering). |
| config/initializers/new_framework_defaults_8_0.rb | Introduces Rails 8 defaults initializer (with some defaults enabled). |
| bin/setup | Updates setup flow and optionally starts dev server via bin/dev. |
| bin/rubocop | Adds a dedicated RuboCop launcher script. |
| bin/dev | Adds a dev entrypoint script for running the Rails server. |
| app/controllers/api/schools_controller.rb | Converts school_params to params.expect. |
| app/controllers/api/school_teachers_controller.rb | Converts strong params to params.expect. |
| app/controllers/api/school_students_controller.rb | Converts strong params to params.expect. |
| app/controllers/api/school_classes_controller.rb | Converts strong params to params.expect. |
| app/controllers/api/projects/remixes_controller.rb | Converts remix params to params.expect (includes nested structures). |
| app/controllers/api/google_auth_controller.rb | Converts strong params to params.expect. |
| app/controllers/api/class_members_controller.rb | Converts strong params to params.expect. |
| Gemfile | Updates Rails requirement to ~> 8.1. |
| Gemfile.lock | Locks Rails 8.1 dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
abcampo-iry
left a comment
There was a problem hiding this comment.
I think overall really good, good job, I'm just leaving a couple of comments.
I think my understanding might be wrong but just want to raise it since we might need to do a bit more for the upgrade or maybe do smaller upgrades, so I would love to verify.
| # | ||
| # This file eases your Rails 8.0 framework defaults upgrade. | ||
| # | ||
| # Uncomment each configuration one by one to switch to the new default. |
There was a problem hiding this comment.
Question how, i'm not sure if this file needs to remain, or we should update this in the same PR:
module App
class Application < Rails::Application
# Initialize configuration defaults for originally generated Rails version.
config.load_defaults 7.1The recommendations says:
You should also first upgrade to Rails 8.0 in case you haven't and make sure your application still runs as expected before attempting an update to Rails 8.1
Are we happy with the risk, instead of following the steps 7.1 -> 7.2, 7.2 -> 8.0, 8.0 -> 8.1
There was a problem hiding this comment.
Thanks, I missed this - I didn't realise that there were a lot of new defaults in 7.2 and 8.1 and I missed updating the framework defaults (but did run the tests against 8.0)
I think your right, it would be safest to ship the change separately:
- Update to 7.2 framework defaults
- Upgrade to rails 8.0 and update framework defaults (with any exceptions we need)
- Upgrade to rails 8.1 and framework defaults (with any exceptions we need)
| params.require(:google_auth).require(:code) | ||
| params.require(:google_auth).require(:redirect_uri) | ||
| params.require(:google_auth).permit(:code, :redirect_uri) | ||
| params.expect(google_auth: %i[code redirect_uri]) |
There was a problem hiding this comment.
I was investigating a little bit, small nitpick related to the other changes, I think the syntax needs to be something like this the important part is the expect before permit:
def google_token_params
params
.expect(google_auth: %i[code redirect_uri])
.tap { |google_auth| google_auth.require(%i[code redirect_uri]) }
endor
def google_token_params
google_auth_params = params.expect(google_auth: %i[code redirect_uri])
google_auth_params.require(%i[code redirect_uri])
google_auth_params
end
Upgrade to Rails 8.1
This is so we get the latest features and bugfixes.
Rails 8 release notes: https://guides.rubyonrails.org/8_0_release_notes.html
Rails 8.1 release notes: https://guides.rubyonrails.org/8_1_release_notes.html
Some notable changes this deals with:
expectas an alternative topermitto prevent clients being able to trigger 500s. Note that this isn't a mandatory change, but rubocop now warns about uses of permit so I have updated them - (more here)[https://blog.saeloun.com/2024/12/10/rails-8-adds-parametersexpect-to-safely-filter-and-require-params/]