diff --git a/app/controllers/projects/metrics_dashboard_controller.rb b/app/controllers/projects/metrics_dashboard_controller.rb index 9d0cbfd2a20e116c623905f11a3545ebd343026d..bc0a701b9fdf32e6bd3c060035fc9d53b808b85c 100644 --- a/app/controllers/projects/metrics_dashboard_controller.rb +++ b/app/controllers/projects/metrics_dashboard_controller.rb @@ -20,10 +20,11 @@ def show elsif default_environment redirect_to project_metrics_dashboard_path( project, - **permitted_params - .to_h - .symbolize_keys - .merge(environment: default_environment) + # Reverse merge the query parameters so that a query parameter named dashboard_path doesn't + # override the dashboard_path path parameter. + **permitted_params.to_h.symbolize_keys + .merge(environment: default_environment.id) + .reverse_merge(request.query_parameters.symbolize_keys) ) else render 'projects/environments/empty_metrics' diff --git a/config/routes/project.rb b/config/routes/project.rb index 4ba1f14222e7025df888766c1406aeca9f3bf5f9..3361193581e8ee966985187972154f61752f6810 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -25,7 +25,10 @@ # Use this scope for all new project routes. scope '-' do get 'archive/*id', constraints: { format: Gitlab::PathRegex.archive_formats_regex, id: /.+?/ }, to: 'repositories#archive', as: 'archive' - get 'metrics(/:dashboard_path)(/:page)', constraints: { dashboard_path: /.+\.yml/, page: 'panel/new' }, + # Since the page parameter can contain slashes (panel/new), use Rails' + # "Route Globbing" syntax (/*page) so that the route helpers do not encode + # the slash character. + get 'metrics(/:dashboard_path)(/*page)', constraints: { dashboard_path: /.+\.yml/, page: 'panel/new' }, to: 'metrics_dashboard#show', as: :metrics_dashboard, format: false namespace :metrics, module: :metrics do diff --git a/spec/requests/projects/metrics_dashboard_spec.rb b/spec/requests/projects/metrics_dashboard_spec.rb index 2f78d104529b0a0e974ddeeb6c0661342c0fb756..f0e0e6a02eea75f9c8f59150a2e0d04d2ea3ad47 100644 --- a/spec/requests/projects/metrics_dashboard_spec.rb +++ b/spec/requests/projects/metrics_dashboard_spec.rb @@ -25,15 +25,16 @@ end it 'retains existing parameters when redirecting' do - get "#{dashboard_route(dashboard_path: '.gitlab/dashboards/dashboard_path.yml')}/panel/new" - - expect(response).to redirect_to( - dashboard_route( - dashboard_path: '.gitlab/dashboards/dashboard_path.yml', - page: 'panel/new', - environment: environment - ) - ) + params = { + dashboard_path: '.gitlab/dashboards/dashboard_path.yml', + page: 'panel/new', + group: 'System metrics (Kubernetes)', + title: 'Memory Usage (Pod average)', + y_label: 'Memory Used per Pod (MB)' + } + send_request(params) + + expect(response).to redirect_to(dashboard_route(params.merge(environment: environment.id))) end context 'with anonymous user and public dashboard visibility' do @@ -110,15 +111,13 @@ describe 'GET :/namespace/:project/-/metrics/:page' do it 'returns 200 with path param page' do - # send_request(page: 'panel/new') cannot be used because it encodes '/' - get "#{dashboard_route}/panel/new?environment=#{environment.id}" + send_request(page: 'panel/new', environment: environment.id) expect(response).to have_gitlab_http_status(:ok) end it 'returns 200 with dashboard and path param page' do - # send_request(page: 'panel/new') cannot be used because it encodes '/' - get "#{dashboard_route(dashboard_path: 'dashboard.yml')}/panel/new?environment=#{environment.id}" + send_request(dashboard_path: 'dashboard.yml', page: 'panel/new', environment: environment.id) expect(response).to have_gitlab_http_status(:ok) end