fail-on-template-vars: improve compatibility with Django behavior

[xavfernandez]

With OneToOneField, Django raises Model.DoesNotExist which is converted by its template engine to string_if_invalid: https://github.com/django/django/blob/5.0.7/django/template/base.py#L932-L933

It is usually falsy, hence the need for InvalidVarException.__bool__ to return bool(self.origin_value) to be consistent with Django's default behavior.

However to trigger InvalidVarException behavior and its dreaded InvalidVarException.__mod__, it needs to go through this check: https://github.com/django/django/blob/5.0.7/django/template/base.py#L716 and thus also needs to be truthy hence the stack inspecting __bool__ method to know what to return.

[bluetech]

The purpose of the pytest-django feature is to loudly fail on invalid. Can you explain a bit more why the ObjectDoesNotExist case is different from the other invalid variable scenarios?

[xavfernandez]

The purpose of the pytest-django feature is to loudly fail on invalid. Can you explain a bit more why the ObjectDoesNotExist case is different from the other invalid variable scenarios?

This is not about not failing loudly but about being consistent with Django's default behavior.

When a ObjectDoesNotExist is raised in a template (typically when a OneToOneField is used, this error is catched and transformed into the configured string_if_invalid value.

It is usually falsy (the default being "") but when --fail-on-template-vars is used, it becomes truthy hence the need to override InvalidVarException.__bool__.

If you comment the added __bool__ method in this PR, you do not get a pytest.fail(msg) but:

E       AssertionError: assert '<div>This sh... appear</div>' == '<div></div>'
E         
E         - <div></div>
E         + <div>This should not appear</div>

This is an issue because when enabling --fail-on-template-vars for the first time, users get useful crashes for undefined variables (that's what users want) but they will also get tests failing because --fail-on-template-vars currently slightly changes the behavior of OneToOneField/ObjectDoesNotExist machinery in templates (and I don't think that users want that).

[adamchainz]

Django’s string_if_invalid feature is flawed in many ways, as I covered in this blog post. Attempting to fix one case here is probably not worth it and could lead to other unexpected template changes.

As I concluded in my post, I think string_if_invalid should be removed from Django and replaced with a Jinja-style undefined object.

[xavfernandez]

I agree that string_if_invalid is flawed and I'm not a fan of Django's template permissive behavior regarding undefined variables.
That's actually why I'm trying to build on and improve pytest-django's InvalidVarException to easily detect such issues.

I've read your blog post (and discovered a few issues I wasn't aware of 😬 ) but by simply adding a :

def __str__(self):
    return self.origin_value

to the current InvalidVarException implementation and setting it in your string_if_invalid_examples.py you either:

• get consistent result with/without string_if_invalid (example 7,8,9)
• or you get the expected exception "Undefined template variable error".

The current implementation is still flawed but not that often.

This PR would handle an hypothetical example 11.

In this situation, I think that perfect is the enemy of good and that this PR improves the current situation.

Even if I disagree with your conclusion and would actually recommend the use of pytest-django's fail-on-template-vars option in new projects (with a few patches, like this PR 😅 ) I recognize that the string_if_invalid feature and pytest-django's InvalidVarException are a convoluted way of monkeypatching Django's template engine and we might actually get better results by directly monkeypatching the engine.

That's actually what I've been doing here
https://github.com/gip-inclusion/les-emplois/blob/c84f8032ec21a14bf3deaffc2c796391c4c1c350/tests/conftest.py#L280-L341
(thanks for your patchy project btw ❤️ ) to detect more cases.
But after writing all this answer, I'm now wondering if I should stop using string_if_invalid altogether in favor of additional patches 🤔