kezabelle / django-shouty-templates

A monkeypatch to make Django error super loudly about invalid template access
BSD 2-Clause "Simplified" License
7 stars 0 forks source link

False positive for null variables in template #10

Open GitRon opened 1 year ago

GitRon commented 1 year ago

Hi @kezabelle!

First, thanks for this super nice package! Really helps you avoid query problems.

I encountered a false positive IMHO when you pass an empty/null/None variables to the template.

MissingVariable at /my/site Token 'attribute' of 'my_variable.attribute' in template 'my_site/page.html' does not resolve.

I often work with default template tags in my templates like this:

{{ my_variable.attribute|default:'-' }}

Firstly, I think it's not an database-related issue so I'd be happy to keep these NullPointers as they are. I'd settle for a global flag if the tool should complain about that 😎

Secondly, Django explicitly provides these template tags - and more than one - to work like I do. Prohibiting this doesn't feel djangoesque to me.

Best regards Ronny

kezabelle commented 1 year ago

Hey @GitRon, thanks for trying the package out.

I don't think it's documented as a caveat (which it probably should be), but this behaviour is tested, because I did anticipate it: https://github.com/kezabelle/django-shouty-templates/blob/a6cf34b110b3170cf8bca0dfba1577dcce0c1c0c/shouty.py#L1524-L1547

Off the top of my head (it's been a while), I believe what I've always done in such situations is to add the occurrence to the blocklist setting, so that it is silenced and behaves as normal. I think the description for how to do that should be in the exception message raised, but it'd be something like the following (untested) in your project's settings:

SHOUTY_VARIABLE_BLACKLIST = {
    # ...
    "my_variable.attribute": ("my_site/page.html",)
    # ...
}

which would silence it for that template alone, though you can also use ("*",) to silence it for all templates I think.


I personally consider the conditional absence of an attribute[^1] to be an error because it breaks the implicit contract the template has, and by enforcing the attribute always exists (even if it's None or "" or 0 or whatever) you are being technically correct. Django itself definitely takes the more lax approach, where sometimes variables or attributes just plain aren't there, which is why this package ships with a fairly large default blocklist to allow the builtin contrib.admin to work, etc.

And to be fair to both Django/you/et al, it's not always easy or appropriate to be that strict, which is why the SHOUTY_VARIABLE_BLACKLIST exists as a project setting, for when this package gets in the way of your expected behaviour -- Granted, it's a bit more manual work in your project vs. default and default_if_none (and any other filters) just worked out of the box, but in my experience it's only necessary every so often and should be a "simple enough" change (assuming I'm right in remembering the exception tells you what to do 🤷) in lieu of my being able to correctly special-case everything[^2]


If you didn't know about SHOUTY_VARIABLE_BLACKLIST and using that (above) fixes your issue, I'd appreciate any feedback you have about how I might make the readme clearer for these kinds of things. If you did already know about the SHOUTY_VARIABLE_BLACKLIST and you've already used it and it didn't help/fix your issue, I'd love to know more details.

[^1]: Which I think this is? It's not that it's None IIRC, it's that it's literally not found. [^2]: The last time I looked in detail at this, the issue was that the resolving happens before any default or custom template tags/filters are applied, and I couldn't figure out a sensible/simple/workable way of knowing when it's appropriate to silence the MissingVariable exceptions in every case.

GitRon commented 1 year ago

Hi @kezabelle - thx for the very detailled answer! You are correct, the errors tells you to add the variable to the blacklist when the variable is missing itself. But in my case, it's a classic "NullPointer". My "my_var" is not there and when I try to access my_var.attribute I get the error. I'm not sure if I could fix this with the blacklist, BUT:

Django allows this behavious and the existance of all those templatetags might be even a sign to encourage people to use silent fail as a feature and not as a bug. Therefore, I would be stoked about a flag I can set which tells your otherwise awesome package to ignore NullPointers in the template. IMHO, having a nullpointer is something completely different than a missing ORM attribute which leads to extra queries - and this is what this project is all about, isn't it? 🤠

I think using your two packages in your tests and locally when developing is a great way of avoiding unnecessary pressure on the DB and I wanted to refactor my project. It's obviously a lot of work, I have many instances where I have to make some workarounds to get it going - but I'll buy this to get the benefit. But changing all my templates and the way I work with them is a) too much and b) doesn't seem to contribute to the goal of reducing queries.

Don't you agree?

Best
Ronny