Skip to content
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.

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

Fix N+1 SQL query by creating a user validator to call current_user a single time. #23057

Conversation

petecheslock
Copy link

Currently in the base_controller if the doorkeeper_token is not set then the require_user function will call current_user multiple times unnecessarily.

def current_resource_owner
@current_user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token
end
def current_user
current_resource_owner || super
rescue ActiveRecord::RecordNotFound
nil
end
def require_authenticated_user!
render json: { error: 'This method requires an authenticated user' }, status: 401 unless current_user
end
def require_not_suspended!
render json: { error: 'Your login is currently disabled' }, status: 403 if current_user&.account&.suspended?
end
def require_user!
if !current_user
render json: { error: 'This method requires an authenticated user' }, status: 422
elsif !current_user.confirmed?
render json: { error: 'Your login is missing a confirmed e-mail address' }, status: 403
elsif !current_user.approved?
render json: { error: 'Your login is currently pending approval' }, status: 403
elsif !current_user.functional?
render json: { error: 'Your login is currently disabled' }, status: 403
else
update_user_sign_in
end
end

To fix this we'll create a function to validate the user so that we can pass current_user to it a single time.

Here is a link to an AppMap showing the function calling this SQL query.

5

Screen Shot 2022-12-19 at 10 49 59 AM

@ClearlyClaire
Copy link
Contributor

It seems to me we'd better change Api::BaseController#current_user and/or Api::BaseController#current_resource_owner to avoid that behavior.

@mjankowski
Copy link
Contributor

Closing in favor of relying on the existing memoization in those methods.

If I'm missing something, can you re-open with a sql query trace during this request to show multiple queries? (or otherwise describe how to replicate) ... I think rails should be caching this query at the AR level, and the controller method should be returning a memoized i-var.

@mjankowski mjankowski closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants