Modifying params in the endpoint will change the default

[ashkan18]

When using default of a param, modifying the param within the request will change the default. Here is a spec for the issue:

require 'spec_helper'

module FreezeDefaultForParamsSpec

  class Users < Grape::API
    resource :users do
      params do
        optional :sort, default: '-created_at'
      end
      get do
        params[:sort].slice!(0)
        params[:sort]
      end
    end
  end

  class Main < Grape::API
    mount Users
  end
end

describe Grape::API do
  subject { FreezeDefaultForParamsSpec::Main }

  def app
    subject
  end

  it 'should not change the default for the param' do
    get '/users'
    expect(last_response.status).to eql 200
    expect(last_response.body).to eql 'created_at'

    get '/users'
    expect(last_response.status).to eql 200
    expect(last_response.body).to eql 'created_at'
  end
end

For the first get in the spec, default is correct which is -created_at but on the second get, default sort param has changed to created_at because of params[:sort].slice!(0) in get.

Ideally we'd need to pass a clone of params, as an alternative we may need to freeze default.

[dblock]

I think we probably don't want to clone the default every time, which could be unnecessarily expensive for every request, but definitely freeze it.

[jlfaber]

@dblock If we freeze default, that might cause existing client code to suddenly start throwing RuntimeErrors (if they are mutating param values). Admittedly, they're probably not getting the behavior they expect on subsequent calls after the default has been modified. So there's already an error present. But it seems to me that the best course is to clone the default when it is assigned to a param.

This is also appealing because it means that params are uniformly modifiable whether they have a default value or not. Freezing the default would cause the two cases to behave differently, which I think is a much more confusing semantic.

Also, I haven't actually verified it, but I think the test spec above will only pass if default is cloned. If we freeze default, the slice! call is going to raise a RuntimeError.

[dblock]

I worry about unintentional performance overheads, but maybe I am worried about nothing. Either way I'll take a PR.

[dblock]

This was fixed in 8b72144.

[ashkan18]

awesome! Thanks! 👍