Handle missing default mod version, don't close db before rendering templates #378

HebaruSan · 2021-07-23T19:37:09Z

Problem

If a logged in admin user tries to open https://spacedock.info/mod/1967 , a 500 error occurs:

We found this in the server log:

2021-07-23 15:22:54,835 ERROR Backend:496 Exception on /mod/1967 [GET]
Traceback (most recent call last):
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/common.py", line 64, in go
    ret = f(*args, **kwargs)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/blueprints/mods.py", line 185, in mod
    return render_template("mod.html",
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/templating.py", line 137, in render_template
    return _render(
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/templating.py", line 120, in _render
    rv = template.render(context)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/environment.py", line 1090, in render
    self.environment.handle_exception()
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/environment.py", line 832, in handle_exception
    reraise(*rewrite_traceback_stack(source=source))
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/_compat.py", line 28, in reraise
    raise value.with_traceback(tb)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/mod.html", line 1, in top-level template code
    {% extends "layout.html" %}
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/layout.html", line 180, in top-level template code
    {% block body %}{% endblock %}
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/mod.html", line 99, in block "body"
    {{ latest.gameversion.friendly_version }}
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/environment.py", line 471, in getattr
    return getattr(obj, attribute)
jinja2.exceptions.UndefinedError: 'None' has no attribute 'gameversion'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1822, in handle_user_exception
    return handler(e)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/app.py", line 173, in handle_generic_exception
    return render_template("error_5XX.html", error=e), e.code or 500
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/templating.py", line 136, in render_template
    ctx.app.update_template_context(context)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 838, in update_template_context
    context.update(func())
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/app.py", line 295, in inject
    'admin': is_admin(),
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/helpers.py", line 8, in is_admin
    return current_user.admin
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/werkzeug/local.py", line 347, in __getattr__
    return getattr(self._get_current_object(), name)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/sqlalchemy/orm/attributes.py", line 480, in __get__
    return self.impl.get(state, dict_)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/sqlalchemy/orm/attributes.py", line 926, in get
    value = state._load_expired(state, passive)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/sqlalchemy/orm/state.py", line 675, in _load_expired
    self.manager.expired_attribute_loader(self, toload, passive)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/sqlalchemy/orm/loading.py", line 1343, in load_scalar_attributes
    raise orm_exc.DetachedInstanceError(
sqlalchemy.orm.exc.DetachedInstanceError: Instance <User at 0x7f6ba3187400> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/14/bhk3)
2021-07-23 15:22:55,340 ERROR SpaceDock:496 500 Internal Server Error: The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

Causes

The mod is unpublished, so only admin users would see the mod template rendered, others get the 403 screen
The mod has no default_version, so the mod template throws an exception when it tries to access a property of latest
While handling this exception, the with_session wrapper closes the db session, which makes current_user an invalid object (it is a rich sqlalchemy object which is stripped of its db connection at close)
The template for 500 errors is rendered, but it uses layout.html which raises DetachedInstanceError because current_user is invalid

Changes

Now if Mod.default_version is not set, the backend code will default to using the one at the top of the list for the main compatibility field
Now the mod template checks whether latest is null before using it, so the first exception won't be thrown
Now with_session and handle_generic_exception no longer close the db; instead this is moved to teardown_request, which is called after the request is handled, so the 500 error template can be rendered with a valid current_user object instead of throwing an exception
https://pythonise.com/series/learning-flask/python-before-after-request

Unsuccessful investigation

I was hoping that we could update our mypy configuration to catch this class of template errors; a latest.game_version.friendly_version reference in the backend Python code would have raised mypy errors because latest is of type Optional[Mod], which may be None.

Sadly, this does not seem to be an option. I found just two pages even coming close to the idea of type checking for jinja2 templates:

https://www.eyrie.org/~eagle/journal/2019-11/001.html
A discussion of the problems that can slip through static type checking when jinja2 templates are involved:

Forget to pass in owner? Exception or silent failure during template rendering depending on your Jinja2 options. Pass in the name of a service when the template was expecting a rich object? Exception or silent failure. Typo in the name of the parameter? Exception or silent failure. Better hope your test suite is thorough.

Unfortunately the proposed solution is to create a "dataclass" for each template, with two downsides:
1. A lot of extra code to maintain in addition to the templates
2. No way to check if the dataclass is correct, you just have to trust that it accurately represents the template
This seems like a non-solution to me.
https://jinja2schema.readthedocs.io/en/latest/
https://github.com/aromanovich/jinja2schema
Parses a jinja2 template and outputs a representation of the types of its inputs. But not in a format usable by mypy, and the project is idle for 4-7 years.

Unfortunately it appears that no one has gone all the way to integrating jinja2 with mypy, so we will have to continue handling these one case at a time.

We looked into adding a pytype test to the GitHub workflows, but it can't handle ChainableUndefined, so that is left out of this pull request.
https://github.com/google/pytype

HebaruSan · 2021-07-26T16:48:55Z

Hi @dumblob, do you mind if I ask what you meant by "(e.g. pytype from Google seems to do that somehow if I'm not mistaken)" in python/mypy#8249? Had you seen somewhere that pytype is able to do type checking for jinja2 template parameters? Maybe there is a setting that I am missing.

dumblob · 2021-07-26T19:00:43Z

Hi @HebaruSan - that's my mistake. I've done some quick testing back then and pytype seemed to handle those, but it actually didn't and falsly claimed they're "correct". So I'm sorry for the fuss - pytype doesn't support jinja templates 😢.

HebaruSan · 2021-07-26T20:10:22Z

Thanks for the response @dumblob! That makes sense and clears it up. Fingers crossed that some project somewhere creates this functionality someday! 🤞

templates/mod.html

DasSkelett · 2021-07-29T00:14:24Z

Interesting, Jinja2 seems to do some magic to catch some of the None attribute access errors (or null reference exceptions as they're called everywhere else...).
If you take a look at e.g. the confirm-update modal, it tries to access latest.friendly_version, which in pure Python would throw an AttributeError. However in our case it doesn't, Jinja2 simply replaces it with a space.
Only once you are trying to go two levels deep, i.e. access the attribute of an attribute of a variable that is None, it throws the error we observed,

DasSkelett · 2021-07-29T00:21:57Z

Now this looks interesting:

Allows using default values with chains of items or attributes that may
contain undefined values without raising a jinja2.exceptions.UndefinedError.

An undefined that is chainable, where both __getattr__ and
__getitem__ return itself rather than raising an
:exc:UndefinedError.

By default JInja seems to catch AttributeErrors for objects passed to the template, and returns an object of type Undefined. This is the behavior I mentioned in the above comment. However trying to access an attribute of an Undefined object is no longer caught and raises an UndefinedError.
This ChainableUndefined is a class that returns itself instead of raising the error, thus it prevents exceptions even if accessing attributes of attributes of None objects.

I think that is something worth activating. Maybe for production only, so we have a chance to catch potentially unwanted behavior during development.

if not app.debug:
    app.jinja_env.undefined = ChainableUndefined

HebaruSan · 2021-07-29T01:00:27Z

Nice find! Agreed that this looks helpful, done.

HebaruSan · 2021-07-29T01:03:19Z

Is from jinja2 import ChainableUndefined correct or not? Mypy complains about it locally and in the tests, but the server starts and runs fine. Maybe we need a version constraint on types-Jinja2?

DasSkelett · 2021-07-29T01:08:51Z

Hm, it looks like it's installing the matching types- packages: types-Jinja2-2.11.2. That should be fine, the new class has been added in 2.11.0.
Maybe they forgot to add it to the types package initially? I haven't fully figured out how they work yet.

DasSkelett · 2021-07-29T01:23:07Z

Yeah, it's completely missing from the stubs package: https://github.com/python/typeshed/blob/master/stubs/Jinja2/jinja2/runtime.pyi#L85

The metadata says it's for Jinja 2.11, but that seems to be outright wrong: https://github.com/python/typeshed/blob/master/stubs/Jinja2/METADATA.toml

DasSkelett · 2021-07-29T11:46:20Z

That stub solved it for mypy, but not pytype :(
Apparently pytype does not support adding custom .pyi files yet:

That's a pretty important feature for a type checker...

So that means, either ignore that error, or get rid of pytype again. Or fix the upstream typeshed stubs, but who knows how long it will take until they get released and arrive in all the tools.

Edit: even worse, this feature exists, but only in a closed-source variant:

Hello! Unfortunately, open-source pytype doesn't support this yet. (We do internally, which is why that paragraph in the docs exists.)

[Q] How to supply user-defined stubs? google/pytype#665

HebaruSan · 2021-07-29T13:03:36Z

Yeah, Pytype isn't offering us any particular assistance here. I'll split that off to another branch in case we decide we want it later.

…emplates

DasSkelett

🤞 especially interested in the effects of db session closing, let's see how it behaves.

DasSkelett · 2021-10-20T09:45:13Z

Turns out the thing we fixed here I've broken again in #385

2021-10-20 09:37:55,181 ERROR SpaceDock:6884 'None' has no attribute 'created'
Traceback (most recent call last):
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/common.py", line 76, in go
    ret = f(*args, **kwargs)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/blueprints/mods.py", line 184, in mod
    return render_template("mod.html",
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/templating.py", line 137, in render_template
    return _render(
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/flask/templating.py", line 120, in _render
    rv = template.render(context)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/environment.py", line 1090, in render
    self.environment.handle_exception()
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/environment.py", line 832, in handle_exception
    reraise(*rewrite_traceback_stack(source=source))
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/lib/python3.8/site-packages/jinja2/_compat.py", line 28, in reraise
    raise value.with_traceback(tb)
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/mod.html", line 1, in top-level template code
    {% extends "layout.html" %}
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/layout.html", line 10, in top-level template code
    {%- block opengraph %}
  File "/var/www/virtual/spacedock.info/htdocs/SpaceDock/KerbalStuff/../templates/mod.html", line 13, in block "opengraph"
    <meta property="og:description" content="{{ latest.friendly_version }} for {{ ga.abbrev }} {{ latest.gameversion.friendly_version }} | Download: {{ size_versions[latest.id] }} | Released on: {{ latest.created.strftime("%Y-%m-%d") }}
jinja2.exceptions.UndefinedError: 'None' has no attribute 'created'

HebaruSan · 2021-11-13T15:53:42Z

Without ChainableUndefined, I get UndefinedError: 'None' has no attribute 'gameversion' on the same line. I guess ChainableUndefined only works for properties, not methods?

Added {% if latest %} check in #415. Without a version, we will just return Mod.short_description.

HebaruSan · 2021-11-15T21:52:42Z

Or fix the upstream typeshed stubs, but who knows how long it will take until they get released and arrive in all the tools.

I started setting up to do this, but it appears that the typeshed stubs will be removed next month, so fixing them is unlikely to work out: python/typeshed#5423

However, that's not necessarily the end of the story for us. The reason they're being removed is that Flask 2 and Jinja2 3 have been released with type hints built-in, but we are currently pinned not to use them:

SpaceDock/requirements-backend.txt

Line 9 in 4dee33b

Flask<2 # Needs testing before upgrading

SpaceDock/requirements-backend.txt

Line 16 in 4dee33b

Jinja2<3 # See Flask

So I wonder whether the rest of our typing for those modules will break once the removal happens. If so, the linked issue also notes that the versions we're using are "no longer supported", so upgrading may need to be a high priority.

HebaruSan added Type: Bug Area: Backend Related to the Python code that runs inside gunicorn Priority: Low Status: Ready labels Jul 23, 2021

HebaruSan requested a review from DasSkelett July 23, 2021 19:37

This comment has been minimized.

Sign in to view

HebaruSan force-pushed the fix/null-defaultversion branch 4 times, most recently from 4d31a77 to a904e42 Compare July 26, 2021 17:15

DasSkelett reviewed Jul 28, 2021

View reviewed changes

templates/mod.html Outdated Show resolved Hide resolved

HebaruSan force-pushed the fix/null-defaultversion branch 2 times, most recently from 38e373d to 626d356 Compare July 29, 2021 01:39

Handle missing default mod version, don't close db before rendering t…

7b5e378

…emplates

HebaruSan force-pushed the fix/null-defaultversion branch from cc32420 to 7b5e378 Compare July 29, 2021 13:18

DasSkelett approved these changes Jul 29, 2021

View reviewed changes

HebaruSan merged commit acfc017 into KSP-SpaceDock:alpha Jul 29, 2021

HebaruSan deleted the fix/null-defaultversion branch July 29, 2021 13:38

This was referenced Oct 2, 2021

α → β ⑨ #395

Merged

β → 🜚🏭 ❹ #398

Merged

HebaruSan mentioned this pull request Nov 13, 2021

Escape user-edited fields in OpenGraph meta tags #415

Merged

HebaruSan mentioned this pull request Nov 16, 2021

Upgrade to Flask 2 #434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing default mod version, don't close db before rendering templates #378

Handle missing default mod version, don't close db before rendering templates #378

HebaruSan commented Jul 23, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HebaruSan commented Jul 26, 2021

dumblob commented Jul 26, 2021

HebaruSan commented Jul 26, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021 •

edited

Loading

HebaruSan commented Jul 29, 2021

HebaruSan commented Jul 29, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021 •

edited

Loading

HebaruSan commented Jul 29, 2021

DasSkelett left a comment

DasSkelett commented Oct 20, 2021

HebaruSan commented Nov 13, 2021 •

edited

Loading

HebaruSan commented Nov 15, 2021

Handle missing default mod version, don't close db before rendering templates #378

Handle missing default mod version, don't close db before rendering templates #378

Conversation

HebaruSan commented Jul 23, 2021 • edited Loading

Problem

Causes

Changes

Unsuccessful investigation

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HebaruSan commented Jul 26, 2021

dumblob commented Jul 26, 2021

HebaruSan commented Jul 26, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021 • edited Loading

HebaruSan commented Jul 29, 2021

HebaruSan commented Jul 29, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021

DasSkelett commented Jul 29, 2021 • edited Loading

HebaruSan commented Jul 29, 2021

DasSkelett left a comment

Choose a reason for hiding this comment

DasSkelett commented Oct 20, 2021

HebaruSan commented Nov 13, 2021 • edited Loading

HebaruSan commented Nov 15, 2021

HebaruSan commented Jul 23, 2021 •

edited

Loading

DasSkelett commented Jul 29, 2021 •

edited

Loading

DasSkelett commented Jul 29, 2021 •

edited

Loading

HebaruSan commented Nov 13, 2021 •

edited

Loading