Move project shorthand url helper generation from initializers (!51865) · Merge requests · GitLab.org / GitLab

Mehmet Emin INAC requested to merge fix_project_shorthand_routes into master Jan 18, 2021

What does this MR do?

This MR moves the shorthand URL helper generation for the projects resource from the initializers to the route definitions step. The reason is, initializers in Rails run only once while booting the application but routes are getting (re)defined whenever the code reload happens. There is another workaround to fix this by just using the to_prepare which would look like the following but it's just a workaround;

Workaround with the `to_prepare` hook

Rails.application.reloader.to_prepare do
  # Devise (see initializers/8_devise.rb) already reloads routes if
  # eager loading is enabled, so don't do this twice since it's
  # expensive.
  Rails.application.reload_routes! unless config.eager_load

  project_url_helpers = Module.new do
    extend ActiveSupport::Concern

    Gitlab::Application.routes.named_routes.helper_names.each do |name|
      next unless name.include?('namespace_project')

      define_method(name.sub('namespace_project', 'project')) do |project, *args|
        send(name, project&.namespace, project, *args)
      end
    end
  end

  # We add the MilestonesRoutingHelper because we know that this does not
  # conflict with the methods defined in `project_url_helpers`, and we want
  # these methods available in the same places.
  Gitlab::Routing.add_helpers(project_url_helpers)
  Gitlab::Routing.add_helpers(TimeboxesRoutingHelper)
end

The benefit of this change

You can see the benefit of this change by following the below steps;

Open the rails console
Define a new route for the projects in config/routes/project.rb like so;

get :foo, to: 'foo', as: :foo

Reload the application with the reload! command
Try to use the new helper like so;

project = Project.last

Gitlab::Routing.url_helpers.namespace_project_foo_path(project.namespace, project)
Gitlab::Routing.url_helpers.project_foo_path(project)

In the current upstream/master this would raise a NoMethodError but with this change, all the new routes and their shorthands will be available in the reload functionality enabled environments.

Another reason to move this logic from the after_initialize callback to after route definitions, is because the existing approach requires us to have routes to be defined before Rails defines the routes itself. This means it requires us to define the routes twice which is an expensive operation that takes around 4 seconds for each run.

Also by using the direct DSL method, we can remove the need of having the Gitlab::Routing module completely.

This MR has been extracted from !51672 (closed) and will unblock the further developments on it.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

[-] Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
[-] Tested in all supported browsers
[-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

[-] Label as security and @ mention @gitlab-com/gl-security/appsec
[-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
[-] Security reports checked/validated by a reviewer from the AppSec team

Edited Jan 20, 2021 by Mehmet Emin INAC

Move project shorthand url helper generation from initializers