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

Query issue fix #414

Merged
merged 5 commits into from
Jun 17, 2024
Merged

Query issue fix #414

merged 5 commits into from
Jun 17, 2024

Conversation

Domejko
Copy link
Contributor

@Domejko Domejko commented Jun 13, 2024

Description

Regarding #267

  • Refactored CustomUserMiddleware to replace AuthenticationMiddleware
  • Removed CustomUserMiddleware from backend/models.py and placed it in backend/middleware.py
  • In settings/settings.py replaced "django.contrib.auth.middleware.AuthenticationMiddleware" with "backend.middleware.CustomUserMiddleware"

Checklist

  • Ran the Black Formatter and
    djLint-er on any new code
    (checks
    will
    fail without)
  • Made any changes or additions to the documentation where required
  • Changes generate no new warnings/errors
  • New and existing unit tests pass locally with my
    changes

What type of PR is this?

  • 🐛 Bug Fix

Added/updated tests?

  • 🙅 no, because they aren't needed

Related PRs, Issues etc

@TreyWW
Copy link
Owner

TreyWW commented Jun 13, 2024

Oh i've just seen what you mean, in our middleware we dont even call the additional fetch for profile. Maybe it's somewhere else. I'll check it out now, i could've swore it was here

@TreyWW
Copy link
Owner

TreyWW commented Jun 13, 2024

Ah this is what I was thinking of https://github.com/TreyWW/MyFinances/blob/main/backend/models.py#L41

@Domejko
Copy link
Contributor Author

Domejko commented Jun 17, 2024

Problem is that both AuthenticationMiddleware and CustomUserMiddleware where using query to fetch the user object. I removed AuthenticationMiddleware from MIDDLEWARE and adjusted CustomUserMiddleware so that it would replace it and populate request.user with our CustomUser instance.

With this changes we don't get double query issue and user profile is also accessible in request.user.

@TreyWW
Copy link
Owner

TreyWW commented Jun 17, 2024

Amazing. Does this pass all tests still and everything visually work on the site? Just wondering if any Django packages use the middleware or if they will automatically move over to our new one

@Domejko
Copy link
Contributor Author

Domejko commented Jun 17, 2024

Yes it was passing tests on my machine and on website everything work as intended. As for AuthenticationMiddleware I did check and it looks like it's not used anywhere else in Django.

I will fix that mypy error and will push that PR.

@Domejko Domejko marked this pull request as ready for review June 17, 2024 12:01
@TreyWW TreyWW merged commit 8b571e9 into TreyWW:main Jun 17, 2024
12 checks passed
@TreyWW
Copy link
Owner

TreyWW commented Jun 17, 2024

Thanks @Domejko, really appreciate the support recently!

@Domejko Domejko deleted the query-issue-fix branch July 9, 2024 09:40
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.

performance bug: Causing extra user queries
2 participants