On Thu, 2012-09-06 at 14:20 +0200, [email protected] wrote:
> From: Michal Fojtik <[email protected]>
>
> * Overiding PATH_INFO with REQUEST_URI cause incorrect
> incorrect mapping of application.
>
> Signed-off-by: Michal fojtik <[email protected]>
ACK, some minor comments:
> ---
> server/lib/deltacloud/core_ext/string.rb | 4 ++++
> server/lib/sinatra/rack_matrix_params.rb | 20 +++++++++++++-------
> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/server/lib/deltacloud/core_ext/string.rb
> b/server/lib/deltacloud/core_ext/string.rb
> index bc382ed..483fe33 100644
> --- a/server/lib/deltacloud/core_ext/string.rb
> +++ b/server/lib/deltacloud/core_ext/string.rb
> @@ -73,6 +73,10 @@ class String
> "#{self[0..(length/2)]}#{end_string}"
> end
>
> + def remove_matrix_params
> + self.gsub(/;([^\/]*)/, '').gsub(/\?(.*)$/, '')
> + end
Why do we also remove query params ? Also, this method is so specialised
that it might be better to leave it as an instance method in the
MatrixParams middleware.
> diff --git a/server/lib/sinatra/rack_matrix_params.rb
> b/server/lib/sinatra/rack_matrix_params.rb
> index e999f89..8a489d3 100644
> --- a/server/lib/sinatra/rack_matrix_params.rb
> +++ b/server/lib/sinatra/rack_matrix_params.rb
> @@ -36,15 +36,21 @@ module Rack
> #
> # All HTTP methods are supported, in case of POST they will be passed as
> a
> # regular <form> parameters.
> -
> def call(env)
> - # Copy PATH_INFO to REQUEST_URI if Rack::Test
> - env['REQUEST_URI'] = env['PATH_INFO'] if env['rack.test']
> - env['REQUEST_PATH'] = env['PATH_INFO'] if env['rack.test']
> +
> + # This ugly hack should fix the issue with Rack::Test where
> + # these two variables are empty and Rack::Test will always
> + # return 404.
> + #
> + if env['rack.test']
> + env['REQUEST_URI'] = env['PATH_INFO']
> + env['REQUEST_PATH'] = env['PATH_INFO']
> + end
Strictly speaking, these two should be File::join(env['SCRIPT_NAME'],
env['PATH_INFO']) .. shouldn't be a big deal in the test environment.
> # Split URI to components and then extract ;var=value pairs
> - uri_components = env['REQUEST_URI'].split('/')
> matrix_params = {}
> + uri_components = (env['rack.test'] ? env['PATH_INFO'] :
> env['REQUEST_URI']).split('/')
There's no need for the conditional here: we set env['REQUEST_URI'] =
env['PATH_INFO'] above if we are running tests.
David
