Merge branch 'dm-default-request-format-html' into 'master'

Disable format determination based on path extension

Closes #54278

See merge request gitlab-org/gitlab-ce!23454

app/controllers/concerns/uploads_actions.rb

 # frozen_string_literal: true
 
 module UploadsActions
-  extend ActiveSupport::Concern
-
   include Gitlab::Utils::StrongMemoize
   include SendFileUpload
 
   UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
 
-  included do
-    prepend_before_action :set_html_format, only: :show
-  end
-
   def create
     link_to_file = UploadService.new(model, params[:file], uploader_class).execute
 
@@ -61,13 +55,6 @@ module UploadsActions
 
   private
 
-  # Explicitly set the format.
-  # Otherwise rails 5 will set it from a file extension.
-  # See https://github.com/rails/rails/commit/84e8accd6fb83031e4c27e44925d7596655285f7#diff-2b8f2fbb113b55ca8e16001c393da8f1
-  def set_html_format
-    request.format = :html
-  end
-
   def uploader_class
     raise NotImplementedError
   end

app/controllers/projects/artifacts_controller.rb

@@ -9,7 +9,6 @@ class Projects::ArtifactsController < Projects::ApplicationController
   before_action :authorize_read_build!
   before_action :authorize_update_build!, only: [:keep]
   before_action :extract_ref_name_and_path
-  before_action :set_request_format, only: [:file]
   before_action :validate_artifacts!, except: [:download]
   before_action :entry, only: [:file]
 
@@ -110,12 +109,4 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
     render_404 unless @entry.exists?
   end
-
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  def set_request_format?
-    request.format != :json
-  end
 end

app/controllers/projects/blob_controller.rb

@@ -9,7 +9,6 @@ class Projects::BlobController < Projects::ApplicationController
   include ActionView::Helpers::SanitizeHelper
   prepend_before_action :authenticate_user!, only: [:edit]
 
-  before_action :set_request_format, only: [:edit, :show, :update, :destroy]
   before_action :require_non_empty_project, except: [:new, :create]
   before_action :authorize_download_code!
 
@@ -242,18 +241,6 @@ class Projects::BlobController < Projects::ApplicationController
       .last_for_path(@repository, @ref, @path).sha
   end
 
-  # In Rails 4.2 if params[:format] is empty, Rails set it to :html
-  # But since Rails 5.0 the framework now looks for an extension.
-  # E.g. for `blob/master/CHANGELOG.md` in Rails 4 the format would be `:html`, but in Rails 5 on it'd be `:md`
-  # This before_action explicitly sets the `:html` format for all requests unless `:format` is set by a client e.g. by JS for XHR requests.
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  def set_request_format?
-    params[:id].present? && params[:format].blank? && request.format != "json"
-  end
-
   def show_html
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last

app/controllers/projects/commits_controller.rb

@@ -12,7 +12,6 @@ class Projects::CommitsController < Projects::ApplicationController
   before_action :assign_ref_vars, except: :commits_root
   before_action :authorize_download_code!
   before_action :set_commits, except: :commits_root
-  before_action :set_request_format, only: :show
 
   def commits_root
     redirect_to project_commits_path(@project, @project.default_branch)
@@ -71,19 +70,6 @@ class Projects::CommitsController < Projects::ApplicationController
     @commits = set_commits_for_rendering(@commits)
   end
 
-  # Rails 5 sets request.format from the extension.
-  # Explicitly set to :html.
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  # Rails 5 sets request.format from extension.
-  # In this case if the ref ends with `.atom`, it's expected to be the html response,
-  # not the atom one. So explicitly set request.format as :html to act like rails4.
-  def set_request_format?
-    request.format.to_s == "text/html" || @commits.ref.ends_with?("atom")
-  end
-
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42330')
   end

config/initializers/action_dispatch_http_mime_negotiation.rb

+# Starting with Rails 5, Rails tries to determine the request format based on
+# the extension of the full URL path if no explicit `format` param or `Accept`
+# header is provided, like when simply browsing to a page in your browser.
+#
+# This is undesireable in GitLab, because many of our paths will end in a ref or
+# blob name that can end with any extension, while these pages should still be
+# presented as HTML unless otherwise specified.
+
+# We override `format_from_path_extension` to disable this behavior.
+
+module ActionDispatch
+  module Http
+    module MimeNegotiation
+      def format_from_path_extension
+        nil
+      end
+    end
+  end
+end

config/routes/wiki.rb

@@ -6,7 +6,7 @@ scope(controller: :wikis) do
     post '/', to: 'wikis#create'
   end
 
-  scope(path: 'wikis/*id', as: :wiki, format: false, defaults: { format: :html }) do
+  scope(path: 'wikis/*id', as: :wiki, format: false) do
     get :edit
     get :history
     post :preview_markdown

spec/controllers/projects/blob_controller_spec.rb

@@ -35,6 +35,11 @@ describe Projects::BlobController do
         let(:id) { 'binary-encoding/encoding/binary-1.bin' }
         it { is_expected.to respond_with(:success) }
       end
+
+      context "Markdown file" do
+        let(:id) { 'master/README.md' }
+        it { is_expected.to respond_with(:success) }
+      end
     end
 
     context 'with file path and JSON format' do

spec/routing/project_routing_spec.rb

@@ -36,36 +36,33 @@ describe 'project routing' do
   shared_examples 'RESTful project resources' do
     let(:actions) { [:index, :create, :new, :edit, :show, :update, :destroy] }
     let(:controller_path) { controller }
-    let(:id) { { id: '1' } }
-    let(:format) { {} } # response format, e.g. { format: :html }
-    let(:params) { { namespace_id: 'gitlab', project_id: 'gitlabhq' } }
 
     it 'to #index' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#index", params) if actions.include?(:index)
+      expect(get("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#index", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:index)
     end
 
     it 'to #create' do
-      expect(post("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#create", params) if actions.include?(:create)
+      expect(post("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#create", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:create)
     end
 
     it 'to #new' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/new")).to route_to("projects/#{controller}#new", params) if actions.include?(:new)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/new")).to route_to("projects/#{controller}#new", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:new)
     end
 
     it 'to #edit' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/1/edit")).to route_to("projects/#{controller}#edit", params.merge(**id, **format)) if actions.include?(:edit)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/1/edit")).to route_to("projects/#{controller}#edit", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:edit)
     end
 
     it 'to #show' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#show", params.merge(**id, **format)) if actions.include?(:show)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#show", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:show)
     end
 
     it 'to #update' do
-      expect(put("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#update", params.merge(id)) if actions.include?(:update)
+      expect(put("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#update", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:update)
     end
 
     it 'to #destroy' do
-      expect(delete("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#destroy", params.merge(**id, **format)) if actions.include?(:destroy)
+      expect(delete("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#destroy", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:destroy)
     end
   end
 
@@ -154,13 +151,12 @@ describe 'project routing' do
     end
 
     it 'to #history' do
-      expect(get('/gitlab/gitlabhq/wikis/1/history')).to route_to('projects/wikis#history', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1', format: :html)
+      expect(get('/gitlab/gitlabhq/wikis/1/history')).to route_to('projects/wikis#history', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
     end
 
     it_behaves_like 'RESTful project resources' do
       let(:actions)    { [:create, :edit, :show, :destroy] }
       let(:controller) { 'wikis' }
-      let(:format) { { format: :html } }
     end
   end