Skip to content
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

Use new params.expect syntax instead of params.require #573

Merged
merged 1 commit into from
Sep 15, 2024

Conversation

jeromedalbert
Copy link
Contributor

Problem

PR rails/rails#51674 updated controller generator templates in the Rails codebase. But, as DHH explained, we need to do the same update to templates in jbuilder, because they are used by default for new Rails apps.

Solution

Port the changes from rails/rails#51674 to this codebase, only for Rails versions 8.0+:

  • Change templates to use params.expect(post: [ :title, ... ]) instead of params.require(:post).permit(:title, ...).

    Notice the spaces around [ and ] in order to be compliant with rubocop-rails-omakase rules, which are enabled by default for new Rails apps.

  • Change templates to use @post = params.expect(:id) instead of @post = params[:id].

cc @martinemde

@MatheusRich
Copy link

@jeromedalbert add a Changelog entry, please

Copy link

@martinemde martinemde 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 following up with this. I think we should not change the fetch cases. Otherwise looks good (with changelog as mentioned).

end

# Only allow a list of trusted parameters through.
def <%= "#{singular_table_name}_params" %>
<%- if Rails::VERSION::MAJOR >= 8 -%>
<%- if attributes_names.empty? -%>
params.expect(<%= singular_table_name %>: {})

Choose a reason for hiding this comment

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

If we replace params.fetch like this it changes the behavior. Now, even though there are no attributes expected, you must send at least one attribute or you'll get a 400 error.

I suggest changing only the case with attributes and leaving this one alone.

Copy link
Contributor Author

@jeromedalbert jeromedalbert Sep 14, 2024

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

You're exactly right. I submitted rails/rails#52932 to fix it. Thanks for noticing that I was in violation of my own recommendation. Thanks!

Choose a reason for hiding this comment

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

This was the purpose of params.allow. Please someone from rails core let me know if that's worth a second, more focused attempt and I can extract the original code back out.

test/scaffold_api_controller_generator_test.rb Outdated Show resolved Hide resolved
@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Sep 14, 2024

@MatheusRich This project does not seem to have a Changelog file. 😅

After looking at this:

"changelog_uri" => "https://github.com/rails/jbuilder/releases/tag/v#{s.version}",

It looks like changelogs are made directly from GitHub at release time: https://github.com/rails/jbuilder/releases

So I guess I'm good on my end, but let me know if I missed something.

@dhh dhh merged commit 1a18149 into rails:main Sep 15, 2024
12 checks passed
martinemde added a commit to martinemde/rails that referenced this pull request Sep 15, 2024
The conversions have the unintended consequence of requiring
params when none are expected on the default generated templates.

Originally pointed out in rails/jbuilder#573.
martinemde added a commit to martinemde/rails that referenced this pull request Sep 15, 2024
The conversions have the unintended consequence of requiring
params when none are expected on the default generated templates.

Adds a missing test for the api controller generation.

Originally pointed out in rails/jbuilder#573.
martinemde added a commit to martinemde/rails that referenced this pull request Sep 15, 2024
The conversions have the unintended consequence of requiring params
when none are expected. Ensure generated templates work as is.
Adds a missing test for the api controller generation.

Originally pointed out in rails/jbuilder#573.
@jeromedalbert jeromedalbert deleted the params-expect-syntax branch September 15, 2024 22:35
derricklannaman referenced this pull request in powerhome/power-web-development-interview Sep 24, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jbuilder](https://github.com/rails/jbuilder)
([changelog](https://github.com/rails/jbuilder/releases/tag/v2.13.0))
| `2.12.0` -> `2.13.0` |
[![age](https://developer.mend.io/api/mc/badges/age/rubygems/jbuilder/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/rubygems/jbuilder/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/rubygems/jbuilder/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/rubygems/jbuilder/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>rails/jbuilder (jbuilder)</summary>

###
[`v2.13.0`](https://github.com/rails/jbuilder/releases/tag/v2.13.0)

[Compare
Source](https://github.com/rails/jbuilder/compare/v2.12.0...v2.13.0)

#### What's Changed

- Redirect to `@record` or path in controller generator by
[@&#8203;jeromedalbert](https://github.com/jeromedalbert) in
[https://github.com/rails/jbuilder/pull/569](https://github.com/rails/jbuilder/pull/569)
- Return early from collection partial rendering if blank by
[@&#8203;tylerjc](https://github.com/tylerjc) in
[https://github.com/rails/jbuilder/pull/560](https://github.com/rails/jbuilder/pull/560)
- Add missing ':see_other' status code in generated destroy controller
method by [@&#8203;ldeld](https://github.com/ldeld) in
[https://github.com/rails/jbuilder/pull/538](https://github.com/rails/jbuilder/pull/538)
- Remove OpenStruct references from Jbuilder by
[@&#8203;mtsmfm](https://github.com/mtsmfm) in
[https://github.com/rails/jbuilder/pull/567](https://github.com/rails/jbuilder/pull/567)
- Use new `params.expect` syntax instead of `params.require` by
[@&#8203;jeromedalbert](https://github.com/jeromedalbert) in
[https://github.com/rails/jbuilder/pull/573](https://github.com/rails/jbuilder/pull/573)

#### New Contributors

- [@&#8203;jeromedalbert](https://github.com/jeromedalbert)
made their first contribution in
[https://github.com/rails/jbuilder/pull/570](https://github.com/rails/jbuilder/pull/570)
- [@&#8203;tylerjc](https://github.com/tylerjc) made their
first contribution in
[https://github.com/rails/jbuilder/pull/560](https://github.com/rails/jbuilder/pull/560)
- [@&#8203;ldeld](https://github.com/ldeld) made their first
contribution in
[https://github.com/rails/jbuilder/pull/538](https://github.com/rails/jbuilder/pull/538)
- [@&#8203;mtsmfm](https://github.com/mtsmfm) made their first
contribution in
[https://github.com/rails/jbuilder/pull/567](https://github.com/rails/jbuilder/pull/567)

**Full Changelog**:
rails/jbuilder@v2.12.0...v2.13.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/powerhome/power-web-development-interview).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
The conversions have the unintended consequence of requiring params
when none are expected. Ensure generated templates work as is.
Adds a missing test for the api controller generation.

Originally pointed out in rails/jbuilder#573.
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.

4 participants