It's not easy to use the proposed interface for query parameters.

[ioquatix]

      # Returns the data received in the query string.
      def GET
        if get_header(RACK_REQUEST_QUERY_STRING) == query_string
          if query_hash = get_header(RACK_REQUEST_QUERY_HASH)
            return query_hash
          end
        end

        set_header(RACK_REQUEST_QUERY_HASH, expand_params(query_param_list))
      end

      def query_param_list
        if get_header(RACK_REQUEST_QUERY_STRING) == query_string
          get_header(RACK_REQUEST_QUERY_PAIRS)
        else
          query_pairs = split_query(query_string, '&')
          set_header RACK_REQUEST_QUERY_STRING, query_string
          set_header RACK_REQUEST_QUERY_HASH, nil
          set_header(RACK_REQUEST_QUERY_PAIRS, query_pairs)
        end
      end

Rails sub-classes this, and calls super in def GET.

Even if a sub-class overrides expand_params, it's cached into the same key.

Therefore, the previous call to Rack::Request#GET caches the value of expand_params, and the later invocation of MyCustomRequest#GET may reuse the same cache even with different expand_params behaviour.

Additionally, setting set_header RACK_REQUEST_QUERY_HASH, nil won't trigger if query_hash = get_header(RACK_REQUEST_QUERY_HASH) later, so I'm not sure if that's supposed to be cached. I suggest we keep manipulation of RACK_REQUEST_QUERY_HASH out of query_param_list.

Rails PR: rails/rails#47652

cc @matthewd

[ioquatix]

I've made some local changes so the following is possible:

    if Rack.release >= "3"
      class QueryParser < ::Rack::QueryParser
        def missing_value
          nil
        end
      end

      QUERY_PARSER = QueryParser.make_default(32)

      def query_parser
        QUERY_PARSER
      end

      def query_hash_key
        "action_dispatch.request.query_hash"
      end

      def form_hash_key
        "action_dispatch.request.form_hash"
      end
    end

But, I still can't get the tests to pass.

[matthewd]

Yeah, I've been staring at this this evening, and pondering the fact that Rails effectively needs to fully override GET and POST.

The nil is clearing any existing cached value, not caching a new one -- it's covering the fact that the HASH and PAIRS both use the same STRING as their invalidation key.

ISTM the ideal model might be that the HASH cache is additionally keyed by something that varies on the expand_params implementation / query parser... but that's definitely adding complexity.

Another possibility, which I pondered even just looking at the similar patterns across GET, POST, query_param_list, and body_param_list, would be a helper that just encapsulates the "cache this value named 'X' as long as you see the same input value 'Y'" behaviour.

As-is, I think Rails needs to:

      # Returns the data received in the query string.
      def GET
        if get_header(RAILS_REQUEST_QUERY_STRING) == query_string
          if query_hash = get_header(RAILS_REQUEST_QUERY_HASH)
            return query_hash
          end
        end

        set_header RAILS_REQUEST_QUERY_STRING, query_string
        set_header(RAILS_REQUEST_QUERY_HASH, expand_params(query_param_list))
      end

With a helper, that might be:

      # Returns the data received in the query string.
      def GET
        cache_in_env(RAILS_REQUEST_QUERY_HASH, RAILS_REQUEST_QUERY_STRING, query_string) do
          expand_params(query_param_list)
        end
      end

      # where:
      def cache_in_env(cache_key, validity_key, current_input, &block)
        if get_header(validity_key) == current_input
          if cached_value = get_header(cache_key)
            return cached_value
          end
        end

        set_header validity_key, current_input
        set_header(cache_key, block.call(current_input))
      end

Obviously anything that means "overriding expand_params or query_parser without redefining the caching on GET and POST will corrupt the cache" is still Not Great -- even though it is the historical Rack::Request behaviour. Perhaps the base class should only store/use the cache when it knows it's working with base class instances directly? 🤔

(all above code is untested hand-waving, to be clear)

[ioquatix]

I found the problem.

      def env_for(uri, env)
        env = DEFAULT_ENV.merge(@env).merge!(env)

        env['HTTP_HOST'] ||= [uri.host, (uri.port if uri.port != uri.default_port)].compact.join(':')
        env['HTTPS'] = 'on' if URI::HTTPS === uri
        env['HTTP_X_REQUESTED_WITH'] = 'XMLHttpRequest' if env[:xhr]
        env['REQUEST_METHOD'] ||= env[:method] ? env[:method].to_s.upcase : 'GET'

        params = env.delete(:params)
        query_array = [uri.query]

        if env['REQUEST_METHOD'] == 'GET'
          # Treat params as query params
          if params
            append_query_params(query_array, params)
          end

This code, appears to convert params: "action" to QUERY_STRING => "action=".

It essentially seems like rack-test won't make it easy to specify "nil" parameters.

[ioquatix]

I'm going to be honest, I really dislike this extra complexity, but I'm at a loss as to how it could be better. I feel like we are kind of painted into a corner. It feels like the Rails way of doing things is really at odds with the way everything else works and it's creating a ton of friction.

Adding keys for the fields that are per-class makes sense to me, as an option.

[ioquatix]

Rack::Test will convert "action" into "action=":

      # Append a string version of the query params to the array of query params.
      def append_query_params(query_array, query_params)
        query_params = parse_nested_query(query_params) if query_params.is_a?(String)
        query_array << build_nested_query(query_params)
      end

[ioquatix]

rails/rails#47652 includes some changes to the test runner to bypass append_query_params.

[ioquatix]

I'm assuming you want to include the deep_munge into the expand_params right?

[jeremyevans]

@ioquatix Can this be closed?

[ioquatix]

Are we still willing to entertain deprecating if get_header(RACK_REQUEST_QUERY_STRING) == query_string?

[jeremyevans]

I'm still on board for deprecating that. I thought this issue was related to the query_param_list removed in 77cf057.

[ioquatix]

As I proposed a while ago, I think Rack::Request semantics need a total overhaul. However, I think the way caching now is over-engineered and hard to understand. So maybe we can make some small incremental improvements. I don't think this issue really captures the extent of what I think is needed so I'll close it.