pallets / jinja

A very fast and expressive template engine.
https://jinja.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.6k forks source link

Misleading error when attribute in getitem(obj, attr) throws AttributeError #1910

Closed agudallago closed 10 months ago

agudallago commented 10 months ago
import os
from jinja2 import environment

class Foo:
  @property
  def bar(self):
    return os.system2 # throws AttributeError

foo = Foo()
environment.Environment().getitem(foo, "bar")._undefined_message

getitem() gives a misleading error when the property bar we are trying to access in object foo gives an AttributeError.

The expected message (which in turn is propagated to the render method) says:

'__main__.Foo object' has no attribute 'bar'

whereas it should say that bar exists, but has itself thrown an AttributeError.

This looks like a stupid thing but it's incredibly hard to debug if it happens inside render, since debugging the rendering process in the HTML line by line is super obscure.

Environment:

davidism commented 10 months ago

Happy to review a PR, but I don't see what we could do here.

agudallago commented 10 months ago

Handle the AttributeError in a graceful way. Check if the function exists by looking into the dir() list for example.

davidism commented 10 months ago

You're asking "detect where an attribute error was raised" which is not trivial, and may not be possible or without other issues. "Just handle it gracefully" isn't really helpful. Again, I'm happy to review a PR, since you seem to know what you're looking for and how to do it.

agudallago commented 10 months ago

https://github.com/pallets/jinja/blob/d594969d722ceb4e8f3da8861befc9c0ac87ae1b/src/jinja2/environment.py#L478

There you're already handling the AttributeError. What I'm saying is when you handle, you can check if attr is part of obj or not. And then output a different message to the undefined.

Am I being too vague?

davidism commented 10 months ago

Yes, I need you to tell me exactly what that code should do to detect this situation.

davidism commented 10 months ago

What you're describing is a general issue with Python, it's not possible to tell the difference between and attribute that doesn't exist and a property that raises an AttributeError at runtime without expensive/imperfect introspection. After runtime, when the traceback is built and the frames printed out, you can look backwards and see what actually raised the AttributeError, and know that it's correct. But at runtime that's not viable, examining the stack is expensive and should not be done on every undefined, hasattr has the same problem as getattr, and dir can be overridden or inaccurate (for example, if __getattr__ is defined).

agudallago commented 10 months ago

I understand doing proper introspection is expensive. But how does any extra expensive or imperfect introspection make this a worse user experience than getting a misleading error, if the program anyway is bound to raise an exception that in most programs would actually make your program ends or retries due to invalid arguments (either your html is broken, or. your context is).

I'm not sure how the dir can be inaccurate, you probably know a lot more about Python low level, but to me sounds like a cheap enough thing to check if the attribute is there and would make the error a lot more helpful for the user. And the library is supposed to help the user.