Is it possible to add a test for this?

I think I've isolated a test case. I made a fix to my PR, and will work on adding a test as soon as I can.

Not all rubies have the unary + operator for strings. 🤕 So still need a way to unfreeze the string while keeping the same encoding and that's backwards compatible.

Kinda stuck here because not sure how to support ruby 2.2.9.

Using the unary operator (+data).force_encoding(encoding).encode! keeps the routing_tests happy because the encodings match what the specs expect. Ruby 2.2.9 does not support the unfreeze operator, though.

Alternatively, using data.dup.force_encoding(encoding).encode! causes the routing_tests to fail. Having a hard time understanding how those tests work, so can't debug -- can someone please take a look at why those tests fail on commit f016c24 (which uses dup)?

According to RuboCop, the unary operator behaves slightly differently than dup:

In Ruby 2.3 or later, use unary plus operator to unfreeze a string literal instead of String#dup and String.new. Unary plus operator is faster than String#dup.

Note: String.new (without operator) is not exactly the same as +''. These differ in encoding. String.new.encoding is always ASCII-8BIT. However, (+'').encoding is the same as script encoding(e.g. UTF-8). So, if you expect ASCII-8BIT encoding, disable this cop.

@programmarchy Sorry for the late reply.

I have been thinking about the issue. At first, unfortunately, your patch won't work on Ruby-2.2.x as you said.
String#+@ and String#-@ were added at Ruby-2.3.x and Ruby-2.2.x doesn't have the same feature as far as I know.

What is the root essence of the issue? I guess, it's the API design.
In other words, if Sinatra doesn't allow developer to pass the params including immutable values, we should raise a humanized errors. Otherwise, we should be able to unfreeze frozen values of params before passing the value in our force_encoding.

The issue shouldn't be resolved by tricky hack and it should be treated as an API design issue.
If you'd like to continue to work on the issue, please tell me your opinion.
Thanks,

@namusyaka Thanks for looking into this.

Regarding API design issue, I think it would be strange to force the user to pass the param value as mutable. As a user of the API, I am not expecting a side effect to occur on that param value -- I'm expecting it to be a value that is read by sinatra, not mutated.

That said, how should it be addressed?

One option could be for sinatra to keep an internal copy of the param, and mutate the copy with force_encoding. The API would appear read-only to the user. However, hiding the force_encoding causes some information to get lost -- the data sent in the response won't exactly match the original inputs (which I think is what's causing some tests to fail...)

Another option could for sinatra to not force encoding, and instead force the caller of update_param to deal with encoding, and throw an error if the encoding is incorrect. Or possibly, add an argument to update_param('foo', 'bar', :force_encoding: false) that throws an error when bar is frozen and the force_encoding flag is true.

To process the user input, sinatra has already encoded params internally as you're doing.
At that time, the data doesn't match with the input.

What's your concern?
Perhaps, I couldn't understand what you said.

In additions to the comment for design, I don't like new flag idea because not only that it increase complexity of sinatra, your suggestion breaks backward compatibility. It really looks not reasonable.

My idea is just simple that changes code place of encoding and it's enough for fixing the issue.

I'm going to write a details as code example by EOW.

@programmarchy How about this?

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index eb94d3ed..4875e0ba 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -1089,7 +1089,7 @@ module Sinatra

     # Dispatch a request with error handling.
     def dispatch!
-      force_encoding(@params.merge!(@request.params))
+      @params.merge!(@request.params).each { |key, val| @params[key] = force_encoding(val.dup) }

       invoke do
         static! if settings.static? && (request.get? || request.head?)

Hello guys, this breaks our tests when we try to pass nil parameter value. Granted it's probably dumb test and nil values are not possible during runtime, but it's a regression anyway:

  def test_api_with_nil_param
    get "/#{host}/test", { 'a_param' => nil }
    assert last_response.ok?, "Last response was not ok"
  end

Leading to:

can't dup NilClass (TypeError)
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1092:in `dup'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1092:in `block in dispatch!'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1092:in `each'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1092:in `dispatch!'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:924:in `block in call!'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1076:in `block in invoke'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1076:in `catch'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:1076:in `invoke'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:924:in `call!'
/usr/local/rvm/gems/ruby-2.3.5@test_proxy_develop_pr_core-1/gems/sinatra-2.0.5/lib/sinatra/base.rb:913:in `call'

I am going to change our test and remove nil which does not make much sense, just leaving the comment here for googlers.