Extract form object behavior#20
Open
Syati wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesForm Object Behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant UserParams
participant FormObject
participant ActiveModel
Request->>UserParams: provide parameter hash
UserParams->>FormObject: evaluate nested parameter requirement
FormObject->>ActiveModel: resolve model_name.param_key
ActiveModel-->>UserParams: return parameter key
UserParams-->>Request: permit and convert parameters
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Move model_name and form_class? into the FormObject concern so the custom "Parameters"/"Parameter"/"Form" suffix-stripping naming lives alongside the other form-only logic that depends on it (require_nested_parameters?, matches_model_name?). Non-Form Params subclasses now fall back to ActiveModel's default model_name (based on the class's own name) instead of having a suffix silently removed, making param_key/i18n_key resolution predictable without needing to mentally track which suffix was stripped. This is a breaking change for non-Form Parameters/Parameter classes: their model_name, param_key, and i18n_key now reflect the full class name (e.g. UserParameter -> "user_parameter" instead of "user"). Updated specs and locale fixtures accordingly.
process_action_controller_parameters now checks form_class? itself before deferring to FormObject#require_nested_parameters?, instead of require_nested_parameters? checking it internally. This makes the Params/FormObject boundary visible at the call site: reading process_action_controller_parameters alone tells you FormObject's logic is only consulted for Form classes, without having to jump into form_object.rb to learn that.
The method was down to two lines and had exactly one caller, so fold it into process_input_parameters's ActionController::Parameters branch instead of keeping a single-use indirection.
docs/form-objects.md and docs/strong-parameters.md still documented the old behavior where Parameters/Parameter suffixes were also stripped from model_name. Update the examples and i18n locale keys to match the current Form-only scoping.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ed1ab941
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name_without_suffix = name.sub(/Form$/, '') | ||
| ActiveModel::Name.new(self, namespace, name_without_suffix) | ||
| else | ||
| super |
There was a problem hiding this comment.
For existing StructuredParams::Params subclasses named ...Parameter or ...Parameters, this super path now falls back to ActiveModel's default name instead of the previous name.sub(/(Parameters?|Form)$/, '') behavior. Because permit still derives its required root from model_name.param_key, calls such as UserParameter.permit(params) now require :user_parameter instead of the previously supported :user, breaking existing controllers and i18n scopes that followed the documented suffix convention.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors StructuredParams::Params by extracting Rails form-object-specific behavior into a new StructuredParams::FormObject ActiveSupport::Concern, and updates tests/docs/signatures to match the resulting model_name / param_key behavior.
Changes:
- Added
StructuredParams::FormObjectconcern and mixed it intoStructuredParams::Params. - Adjusted
model_namecustomization to apply only toForm-suffixed classes (and updated specs/docs accordingly). - Updated i18n fixtures/overrides and regenerated
sig/outputs to reflect the new behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/locales/ja.yml | Updates attribute translation scopes to match new model_name.i18n_key values. |
| spec/permit_spec.rb | Updates nested root key in params hashes to match new param_key behavior. |
| spec/params_spec.rb | Updates i18n override keys for nested error messages. |
| spec/i18n_spec.rb | Updates i18n override keys used in translation resolution tests. |
| spec/form_object_spec.rb | Updates expectations around suffix stripping / model_name behavior. |
| sig/structured_params/params.rbs | Updates generated signature to reflect extracted concern + removed methods. |
| sig/structured_params/form_object.rbs | New generated signature for the concern. |
| lib/structured_params/params.rb | Includes the concern and routes ActionController::Parameters handling through it. |
| lib/structured_params/form_object.rb | New concern holding form helper methods + nested-params require heuristics + model_name override. |
| lib/structured_params.rb | Requires ActiveSupport concern and the new form_object file. |
| docs/strong-parameters.md | Updates examples/explanations for permit key selection (param_key). |
| docs/form-objects.md | Updates class-name conventions and i18n key examples. |
Comment on lines
+170
to
+171
| require = self.class.form_class? && require_nested_parameters?(params) | ||
| self.class.permit(params, require: require).to_h |
Comment on lines
222
to
233
| describe 'class name with "Parameters" suffix' do | ||
| it 'removes "Parameters" suffix from model_name' do | ||
| expect(OrderParameters.model_name.name).to eq('Order') | ||
| expect(OrderParameters.model_name.param_key).to eq('order') | ||
| it 'keeps the default ActiveModel model_name (no suffix removal outside Form classes)' do | ||
| expect(OrderParameters.model_name.name).to eq('OrderParameters') | ||
| expect(OrderParameters.model_name.param_key).to eq('order_parameters') | ||
| end | ||
| end | ||
|
|
||
| describe 'class name with "Parameter" suffix' do | ||
| it 'removes "Parameter" suffix from model_name' do | ||
| expect(PaymentParameter.model_name.name).to eq('Payment') | ||
| expect(PaymentParameter.model_name.param_key).to eq('payment') | ||
| it 'keeps the default ActiveModel model_name (no suffix removal outside Form classes)' do | ||
| expect(PaymentParameter.model_name.name).to eq('PaymentParameter') | ||
| expect(PaymentParameter.model_name.param_key).to eq('payment_parameter') | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extract form-object-specific behavior from
StructuredParams::Paramsinto anActiveSupport::Concern.Paramsretains shared model naming, typed parameter handling, validation, and serialization. This preserves the existingForm,Parameter, andParametersnaming behavior while isolating Rails form helper and nested form-root handling.Validation
bundle exec rubocop --no-server lib/structured_params.rb lib/structured_params/params.rb lib/structured_params/form_object.rb spec/form_object_spec.rb spec/permit_spec.rbbundle exec rbs-inline --output=sig lib/**/*.rbbundle exec steep checkbundle exec rspecSummary by CodeRabbit
New Features
model_name,to_model, and persistence behavior.Documentation
Bug Fixes
ParameterorParametersnow retain their default ActiveModel naming and i18n keys.