-
-
Notifications
You must be signed in to change notification settings - Fork 195
Fix/missing context vars #453
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
+ Coverage 97.39% 97.46% +0.07%
==========================================
Files 38 38
Lines 422 434 +12
==========================================
+ Hits 411 423 +12
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -188,6 +188,11 @@ def admin_interface_use_changeform_tabs(adminform, inline_forms): | |||
return has_tabs | |||
|
|||
|
|||
@register.simple_tag(takes_context=True) | |||
def resolve_variable(context, var_name, default=""): |
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.
Could you please rename the template tag to admin_interface_resolve_variable
?
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.
Done
renamed the template tag to admin_interface_resolve_variable as suggested.
@@ -188,6 +188,11 @@ def admin_interface_use_changeform_tabs(adminform, inline_forms): | |||
return has_tabs | |||
|
|||
|
|||
@register.simple_tag(takes_context=True) | |||
def resolve_variable(context, var_name, default=""): | |||
return context.get(var_name, default) |
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.
This works only for top-level context keys. It doesn't resolve dotted variables like user.username
, the correct approach is:
try:
return template.Variable(var_name).resolve(context)
except VariableDoesNotExist:
return default
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.
Thanks for the suggestion!
I initially considered using template.Variable(var_name).resolve(context) directly, but the issue is that when a variable (or part of a dotted lookup like var.dottedvar) is missing, Variable.resolve() raises VariableDoesNotExist, which gets logged by Django and pollutes the logs. which brings us to the same issue that we are trying to prevent .
To avoid this, I’ve added a lightweight pre-check that verifies variable (normal and dotted) exists in the context.
If it does, I then delegate to Variable.resolve() so Django still handles callables, translations, etc. If not, I return the default immediately.
Based on the above comments Kindly let me know If anything needed to be done or changed or explained.
|
tests/test_resolve_variable.py
Outdated
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.
Just a small thing: please use always the same variable name for coherence. I see sometimes you use res
, sometimes result
. Better to choose one (result
) and use it everywhere to keep the code more clear.
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.
Acknowledged, and thank you for pointing that out !
bits = var_name.split(".") | ||
current = context.flatten() # merge all and create a context dict | ||
|
||
for bit in bits: | ||
try: | ||
if isinstance(current, dict): | ||
current = current[bit] | ||
else: | ||
current = getattr(current, bit) | ||
except (KeyError, AttributeError, TypeError): | ||
return default |
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.
template.Variable(var_name).resolve(context)
already do all the resolution with dot syntax, so the code before is not necessary.
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.
Thanks for the feedback!
You’re right that template.Variable(var_name).resolve(context)
already handles dotted lookups. Reason I added the pre-check is because Variable.resolve()
logs a VariableDoesNotExist
exception for every missing lookup,
From Django’s source (django/template/base.py, around lines 913 and 939), you can see the logger call:
except Exception as e:
template_name = getattr(context, "template_name", None) or "unknown"
logger.debug(
"Exception while resolving variable '%s' in template '%s'.",
bit,
template_name,
exc_info=True,
)
So if we just wrap Variable.resolve() in a try/except, we still get log noise:
One possible workaround is to temporarily silence the django.template logger while resolving, and then restore its previous state:
logger = logging.getLogger("django.template")
old_level = logger.level
try:
logger.setLevel(logging.INFO)
return template.Variable(var_name).resolve(context)
except VariableDoesNotExist:
return default
finally:
logger.setLevel(old_level)
This approach does work, but it temporarily changes the global logger state.
At the moment I haven’t found an alternative solution
Please let me know if there’s a cleaner hook in Django’s template system to avoid this log pollution.
Or else we can proceed with this changes to the logging itself temporarily.
Kindly let me know you thoughts on this. |
name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo
Describe your changes
This PR adds a
resolve_variable
template tag to safely handle missing context variables inbase_site.html
, preventingVariableDoesNotExist
log noise.resolve_variable
returns an empty string or a custom default instead of raising exceptions.base_site.html
to use it foradminform
andinline_admin_formsets
.Missing context variables are harmless, but with template logging they generate
VariableDoesNotExist
logs, creating noise.resolve_variable
can prevent this when used as a guard.Alternative :-
In our production, we suppressed via Django’s
LOGGING
config insettings.py
:Related issue
#446
Checklist before requesting a review