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: add values_list("token, "expires") to reduce csl apps load time (closes Issue #1663) #1682

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

Conversation

devsrivv
Copy link

Proposed changes

  • Add values_list("token, "expires") in between .order_by("-expires") and .first()

Brief description of rationale

Instead of retrieving all of the fields from the AccessToken object (which leads to slower load times), only retrieve necessary fields to filter the most recent Token. In turn, this will improve the load time of CSL apps.

#1663

@devsrivv devsrivv requested a review from a team as a code owner May 11, 2024 22:46
@devsrivv devsrivv changed the title (format fix) - Add values_list("token, "expires") to reduce CSL Apps load time (Closes Issue #1663) fix: add values_list("token, "expires") to reduce csl apps load time (closes Issue #1663) May 13, 2024
@coveralls
Copy link

Coverage Status

coverage: 79.467% (+0.02%) from 79.447%
when pulling be3270d on devsrivv:dev
into 02c1bd4 on tjcsl:dev.

Copy link
Member

@NotFish232 NotFish232 left a comment

Choose a reason for hiding this comment

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

Hey Dev, thanks for the PR! Could you please provide some numbers indicating the speed up this change would have in loading csl apps? Thanks!

@devsrivv
Copy link
Author

Hey Justin, I can't figure out a way to practically measure the speed of this change in my dev environment as the links lead to CSL apps of the actual ion (signed in to my account instead of the test account). However, I tested the change in my dev environment and I can confirm that it functions.

Originally, the query filtered through all 10 fields id, user, source_refresh_token, token, id_token, application, expires, scope, created, updated from oauth2_provider.models.AccessToken

I made it so that it only queries the token and expires fields along with the necessary user and application fields so that it saves time (reducing the size from 10 fields to 4 fields), which theoretically saves 60% more time.

Does the logic behind my solution make sense? If not, is there any way to check the issue in my dev environment? Thanks

@alanzhu0
Copy link
Member

I'm not sure values_list saves time here, which is why a quantitative measurement is needed. It seems to me what's taking a long time is filtering through the AccessToken model, not accessing the fields, but maybe your solution works. You could create your own OAuth application to test the timing. Have you done the Django blog assignment? That would work.

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

4 participants