-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor: N+1 query #3867
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
base: main
Are you sure you want to change the base?
Refactor: N+1 query #3867
Changes from all commits
19cf724
aba8325
0363ca7
078b9d4
6d68958
2cae695
22deca9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,9 +39,17 @@ | |
|
|
||
| def agency(request): | ||
| """Get the agency from the request's session, or None""" | ||
| agency_id = request.session.get(_AGENCY) | ||
| if not agency_id: | ||
| return None | ||
|
|
||
| if getattr(request, "_cached_agency", None) and request._cached_agency.id == agency_id: | ||
| return request._cached_agency | ||
| try: | ||
| return models.TransitAgency.by_id(request.session[_AGENCY]) | ||
| except (KeyError, models.TransitAgency.DoesNotExist): | ||
| agency = models.TransitAgency.by_id(agency_id) | ||
| request._cached_agency = agency | ||
| return agency | ||
| except models.TransitAgency.DoesNotExist: | ||
| return None | ||
|
|
||
|
|
||
|
|
@@ -124,9 +132,17 @@ def enrollment_reenrollment(request): | |
|
|
||
| def flow(request) -> models.EnrollmentFlow | None: | ||
| """Get the EnrollmentFlow from the request's session, or None""" | ||
| flow_id = request.session.get(_FLOW) | ||
| if not flow_id: | ||
| return None | ||
|
|
||
| if getattr(request, "_cached_flow", None) and request._cached_flow.id == flow_id: | ||
| return request._cached_flow | ||
| try: | ||
| return models.EnrollmentFlow.by_id(request.session[_FLOW]) | ||
| except (KeyError, models.EnrollmentFlow.DoesNotExist): | ||
| flow = models.EnrollmentFlow.by_id(flow_id) | ||
| request._cached_flow = flow | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above |
||
| return flow | ||
| except models.EnrollmentFlow.DoesNotExist: | ||
| return None | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,9 @@ def _filter_empty(ls): | |||||||||||||||||||||
| # SECURITY WARNING: don't run with debug turned on in production! | ||||||||||||||||||||||
| DEBUG = os.environ.get("DJANGO_DEBUG", "false").lower() == "true" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # SECURITY WARNING: don't run with debug_toolbar turned on in production! | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For extra caution?
Suggested change
I also wonder if it'd be nice to move this down with the activation code? Everything else up here are core Django settings. |
||||||||||||||||||||||
| DEBUG_TOOLBAR = os.environ.get("DJANGO_DEBUG_TOOLBAR", "false").lower() == "true" | ||||||||||||||||||||||
|
thekaveman marked this conversation as resolved.
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ALLOWED_HOSTS = _filter_empty(os.environ.get("DJANGO_ALLOWED_HOSTS", "localhost").split(",")) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -107,6 +110,20 @@ def RUNTIME_ENVIRONMENT(): | |||||||||||||||||||||
| if DEBUG: | ||||||||||||||||||||||
| MIDDLEWARE.append("benefits.core.middleware.DebugSession") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if DEBUG_TOOLBAR: | ||||||||||||||||||||||
| INSTALLED_APPS.append("debug_toolbar") | ||||||||||||||||||||||
| MIDDLEWARE.insert(0, "debug_toolbar.middleware.DebugToolbarMiddleware") | ||||||||||||||||||||||
| INTERNAL_IPS = ["127.0.0.1"] | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this has no effect when overriding
Suggested change
If we weren't using Docker, I think we could not override |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def show_toolbar(request): | ||||||||||||||||||||||
| # Show the toolbar when in local development mode | ||||||||||||||||||||||
| return DEBUG | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| DEBUG_TOOLBAR_CONFIG = { | ||||||||||||||||||||||
| "SHOW_TOOLBAR_CALLBACK": show_toolbar, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+118
to
+124
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may not be worth the readability tradeoff, but you could simplify this with a lambda function:
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| HEALTHCHECK_USER_AGENTS = _filter_empty(os.environ.get("HEALTHCHECK_USER_AGENTS", "").split(",")) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| CSRF_COOKIE_AGE = None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more "Django" way to do this caching? Attaching a random prop on to the
requestfeels a little hacky and like it would be easy to get out of control.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree, I had the same feeling. I'll do a bit more searching for a cleaner, more "Django" way to do this. I think it's worth the effort it since it does lower the duplicated queries by a good bit especially on the
/eligibilitypart.