Don't require lib/gitlab/request_profiler/middleware.rb in config/initializers/request_profiler.rb
What does this MR do?
We should never use require
for application files that are in the auto-load paths (note that eager-load paths are included in the auto-load paths) since Rails takes care of auto-loading files, as long as they're named and placed in the correct place.
Also, when referencing a constant, in some cases it needs to be scoped properly to avoid ambiguous references.
For instance in this particular case, by referencing RequestProfiler
in Gitlab::RequestProfiler::Middleware#profile?
, Rails throws a A copy of Gitlab::RequestProfiler::Middleware has been removed from the module tree but is still active!
error. I believe the issue is as follows:
-
RequestProfiler
is expanded toGitlab::RequestProfiler::Middleware::RequestProfiler
and is undefined at this point. -
A call to
ActiveSupport::Dependencies::ModuleConstMissing#const_missing
is issued, which callsActiveSupport::Dependencies#load_missing_constant
. -
An exception is raised because of the following code
unless qualified_const_defined?(from_mod.name) && Inflector.constantize(from_mod.name).equal?(from_mod) raise ArgumentError, "A copy of #{from_mod} has been removed from the module tree but is still active!" end
It's not clear to me why this error occurs at this point, but what's sure is that replacing RequestProfiler
with Gitlab::RequestProfiler
in the middleware fixes the issue since it tells Rails that it should look for a Gitlab::RequestProfiler
constant instead of looking for Gitlab::RequestProfiler::Middleware::RequestProfiler
, thus not triggering the const_missing
behavior.
Another solution for this particular case would be to simply get rid of lib/gitlab/request_profiler.rb
and put all the logic in lib/gitlab/request_profiler/middleware.rb
directly, as suggested in https://gitlab.com/gitlab-org/gitlab-ce/issues/20452#note_20219167.
Are there points in the code the reviewer needs to double check?
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #20452 (closed)