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

Rails 7.1: nested object cannot handle its parameters properly #49601

Closed
eroluysal opened this issue Oct 12, 2023 · 5 comments
Closed

Rails 7.1: nested object cannot handle its parameters properly #49601

eroluysal opened this issue Oct 12, 2023 · 5 comments
Milestone

Comments

@eroluysal
Copy link

eroluysal commented Oct 12, 2023

Steps to reproduce

Rails 7.1 cannot properly handle parameters from multipart/formdata requests.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "rails", "~> 7.1.0"
end

require "rack/test"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    post "/countries" => "countries#create"
  end
end

class CountriesController < ActionController::Base
  include Rails.application.routes.url_helpers

  def create
    puts country_params
    render plain: "Home"
  end

  private
    def country_params
      params.fetch(:country, {}).permit(:name, metadatum_attributes: [:title, :description])
    end
end

require "minitest/autorun"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    post "/countries", {
      country: {
        name: "Turkey",
        # metadatum_attributes: {
        #   title: "metadatum title",
        #   description: "metadatum description"
        # }
        "metadatum_attributes[title]": "metadatum title",
        "metadatum_attributes[description]": "metadatum description",
      }
    }
    assert last_response.ok?
  end

  private
    def app
      Rails.application
    end
end

Expected behavior

Actual behavior

It was working smoothly in version 7.0.8. However, after the 7.1.0 update, it gives this error on the console.

Unpermitted parameter: :metadatum_attributes[title. Context: { controller: CountriesController, action: create, request: #<ActionDispatch::Request:0x000000010cd11ae8>, params: {"country"=>{"name"=>"Hello", "image"=>#<ActionDispatch::Http::UploadedFile:0x00000001101349d8 @tempfile=#<Tempfile:/var/folders/73/lcd4nwrj3xsbm4mj4xtgxx4r0000gp/T/RackMultipart20231012-15400-mfpjrl.jpg>, @content_type="image/jpeg", @original_filename="dolmabahce-palace.jpg", @headers="Content-Disposition: form-data; name=\"country[image]\"; filename=\"dolmabahce-palace.jpg\"\r\nContent-Type: image/jpeg\r\n">, "about"=>"Test", "metadatum_attributes[title"=>{"]"=>"Hello World"}}, "format"=>:json, "controller"=>"countries", "action"=>"create"} }

System configuration

Rails version: 7.1.1

Ruby version: 3.2.2

@eroluysal eroluysal changed the title Rails 7.1 breaks handling nested params definitions Rails 7.1: nested object cannot handle its parameters properly Oct 12, 2023
@fatkodima
Copy link
Member

Can you create a reproduction test script using one of the templates in https://github.com/rails/rails/tree/main/guides/bug_report_templates ?

@fatkodima fatkodima added actionpack more-information-needed When reporter needs to provide more information labels Oct 12, 2023
@rafaelfranca rafaelfranca added this to the 7.1.2 milestone Oct 12, 2023
@eroluysal
Copy link
Author

eroluysal commented Oct 13, 2023

@fatkodima I just updated the issue. And I added the test codes.

When I run the test, as you can see the result is now;

Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...
Run options: --seed 41159

# Running:

I, [2023-10-13T21:09:09.379388 #45765]  INFO -- : Started POST "/countries" for 127.0.0.1 at 2023-10-13 21:09:09 +0300
{"name"=>"Turkey"}
.

Finished in 0.087516s, 11.4265 runs/s, 11.4265 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

I cannot get the metadatum_attributes parameter.

When I run the 7.0.8 version, it looks like this:

Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...
Run options: --seed 44

# Running:

I, [2023-10-13T21:16:41.921134 #46030]  INFO -- : Started POST "/countries" for 127.0.0.1 at 2023-10-13 21:16:41 +0300
{"name"=>"Turkey", "metadatum_attributes"=>#<ActionController::Parameters {"title"=>"metadatum title", "description"=>"metadatum description"} permitted: true>}
.

Finished in 0.089626s, 11.1575 runs/s, 11.1575 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

Am I doing something wrong?

thanks.

@rails-bot rails-bot bot removed the more-information-needed When reporter needs to provide more information label Oct 13, 2023
@fatkodima
Copy link
Member

Thanks. I see some garbage in params. There was a similar issue in rails recently? 🤔 Can't find it quickly. Will look into this.

@fatkodima
Copy link
Member

fatkodima commented Oct 13, 2023

I debugged, that this started to fail after #47133 gets merged into Rails. That PR updates requirement to Rack 3.
Then I debugged Rack and identified that it started to fail after rack/rack#1797 gets merged. That PR switched for Regexp parsing (for some part) to manually parsing query strings.

I believe, that your example worked before because of some "unexpected behaviour" via regexp parsing that led to that "feature". But you should really use a hash in your tests, like the commented section from your reproduction script

# metadatum_attributes: {
#   title: "metadatum title",
#   description: "metadatum description"
# }

and for curl use --form 'country[metadatum_attributes][title]=Hello World' instead of --form 'country[metadatum_attributes[title]]=Hello World' (note the difference in brackets).

cc @jeremyevans

@jeremyevans
Copy link
Contributor

This is basically the same as rack/rack#2128. The general opinion of Rack maintainers is that using country[metadatum_attributes[title]] was relying on unintended and undefined behavior in Rack, and not something we want to support in Rack going forward. Code should switch to country[metadatum_attributes][title], which Rack has always recommended and which works in all versions of Rack that support nested parsing. If you have strong feelings about this, please discuss at rack/rack#2128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants