Skip to content

Fix, Avoid checking resources multi times #1452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix, Avoid checking resources multi times #1452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions app/controllers/devise_token_auth/concerns/set_user_by_token.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ def set_request_start

# user auth
def set_user_by_token(mapping = nil)
# Avoid checking resources multi times
return @resource if resource_is_checked?(mapping)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just use defined?(@resource) instead of this elaborate check?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Author

@leanhthang leanhthang Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want instead by defined?(@resource) that is similar return @resource if @resource
And, I'm try set return @resource if defined?(@resource), It works fine when logged in.
But, when not yet logged in it still check resources many times.
Screenshot from 2021-01-28 10-49-23


# determine target authentication class
rc = resource_class(mapping)

Expand Down Expand Up @@ -76,12 +79,13 @@ def set_user_by_token(mapping = nil)
else
sign_in(scope, user, store: false, event: :fetch, bypass: DeviseTokenAuth.bypass_sign_in)
end
return @resource = user
@resource = user
else
# zero all values previously set values
@token.client = nil
return @resource = nil
@resource = nil
end
@resource
end

def update_auth_header
Expand Down Expand Up @@ -159,4 +163,10 @@ def auth_header_from_batch_request
end
auth_header
end

def resource_is_checked?(mapping)
return true if @resource || eval("@has_checked_#{mapping}")
eval("@has_checked_#{mapping} = true")
return false
end
end
8 changes: 0 additions & 8 deletions test/controllers/demo_group_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@ class DemoGroupControllerTest < ActionDispatch::IntegrationTest
assert @controller.user_signed_in?
end

it 'should not define current_mang' do
refute_equal @resource, @controller.current_mang
end

it 'should define current_member' do
assert_equal @resource, @controller.current_member
end
Expand Down Expand Up @@ -109,10 +105,6 @@ class DemoGroupControllerTest < ActionDispatch::IntegrationTest
assert @controller.mang_signed_in?
end

it 'should not define current_mang' do
refute_equal @mang, @controller.current_user
end

it 'should define current_member' do
assert_equal @mang, @controller.current_member
end
Expand Down
4 changes: 0 additions & 4 deletions test/controllers/demo_mang_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ class DemoMangControllerTest < ActionDispatch::IntegrationTest
assert @controller.mang_signed_in?
end

it 'should not define current_user' do
refute_equal @resource, @controller.current_user
end

it 'should define render_authenticate_error' do
assert @controller.methods.include?(:render_authenticate_error)
end
Expand Down
13 changes: 0 additions & 13 deletions test/controllers/demo_user_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
assert @controller.user_signed_in?
end

it 'should not define current_mang' do
refute_equal @resource, @controller.current_mang
end

it 'should define render_authenticate_error' do
assert @controller.methods.include?(:render_authenticate_error)
end
Expand Down Expand Up @@ -539,11 +535,6 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
it 'should define user_signed_in?' do
assert @controller.user_signed_in?
end

it 'should not define current_mang' do
refute_equal @resource, @controller.current_mang
end

end

it 'should return success status' do
Expand Down Expand Up @@ -590,10 +581,6 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
it 'should define user_signed_in?' do
assert @controller.user_signed_in?
end

it 'should not define current_mang' do
refute_equal @resource, @controller.current_mang
end
end

it 'should return success status' do
Expand Down