Resolve "Refactor by_login and by_id_and_login in user model"
What does this MR do and why?
This MR is related to issue #1127 (closed)
-
Remove
unscoped
inby_login
inuser
modelThe reasons are as follows:
-
The program currently only calls the
by_login
method through theuser
model, and theuser
model does not set thedefault_scope
.Related screenshots : 1 -
unscoped
commit message(72f428c7) and MR(https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34) did not findunscoped
explanation for necessity -
They can still run through local tests and all test cases after removing
unscoped
.
So,
unscoped
is redundant inby_login
. -
-
Merge
by_id_and_login
intoby_login
The reasons are as follows:
-
by_login
andby_id_and_login
are both methods of user query and the code logic repetition rate is high -
by_id_and_login
commit(6f841a95) is used to 'Prevent interrupted 2FA sign-in from signing-in incorrect user',which can be merged intoby_login
for implementation -
The
by_id_and_login
method determines whetherlogin
belongs tousername
oremail
during sql query time.Moving the check to Ruby makes this method an additional 1.5 times faster compared to doing the check in the SQL query according to 72f428c7 -
They can still run through local tests and all test cases after merging
by_id_and_login
intoby_login
So,we should merge
by_id_and_login
intoby_login
-
Reference of source code
-
https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/user.rb
This reference is related to targe: Remove
unscoped
inby_login
inuser
modelFrom the
user.rb
file, it can be found that thedefault_scope
is not set in theuser
model. Combined with screenshot 1, it can be concluded that the parent class of theuser
model and the included modules do not havedefault_scope
set.As far as the
user
model itself is concerned, theunscoped
in theby_login
method makes redundant -
The following is the usages of
by_login
throughout the program:These references are related to targe: Remove
unscoped
inby_login
inuser
model- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/controllers/omniauth_callbacks_controller.rb#L28
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/controllers/sessions_controller.rb#L221
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/controllers/ee/sessions_controller.rb#L95
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/api/captcha_check.rb#L29
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/auth.rb#L95
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/auth.rb#L282
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/auth/auth_finders.rb#L106
As can be seen from the above,
by_login
is called directly through theuser
model rather than any of its subclasses, anddefault_scope
is not set when callingby_login
. So currently theunscoep
ofby_login
is redundant for the whole program -
https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/models/user_spec.rb#L2907
This reference is related to targe: Merge
by_id_and_login
intoby_login
The model spec of
by_login
before mergingThe original
by_login
model spec covers 740~747 and 751, 752 inby_login
in this MR.The new
by_login
model spec needs to cover 748~750 inby_login
in this MR.
Screenshots or screen recordings
screenshot-1:Here is a screenshot showing that the user model does not have default_scope set
Closes #1127 (closed)