Catch APIException in doc generation by sigvef · Pull Request #5443 · encode/django-rest-framework (original) (raw)

sigvef

The documentation generator calls view.get_serializer() in order to
inspect it for documentation generation. However, if get_serializer()
throws an APIException (e.g. PermissionDenied), it doesn't get caught at
the call site, but instead propagates up and aborts the entire view.
With the try/except in this commit, the documentation generator instead
gratiously ignores that particular view and moves on to the next one
instead. Practical concequences of this commit is that the docs no
longer break if any view's get_serializer(..) throws an APIException.

stianjensen

@@ -11,6 +11,7 @@
from django.utils.translation import ugettext_lazy as _
from rest_framework import serializers
from rest_framework import exceptions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this import one line up to resolve lint errors on python 2.7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

carltongibson

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'm inclined to take this.

There's just the point about the warning.

Good work. Thanks!

try:
serializer = view.get_serializer()
except exceptions.APIException:
serializer = None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a warning here. Something like

"""
{}.get_serializer() raised an exception during schema 
generation. 
Serializer fields will not be generated for {} {}.
""".format(view.cls.__name__, method, path)

(The formatting there is more for GitHub than the code.)

This way we'll have some kind of console feedback so users can address any issues.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, added a warning.

@sigvef

Updated, ready for review.

carltongibson

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think this is a good improvement. No-one wants an error-page. 🙂

Thanks for the input!

@carltongibson

@sigvef Could just run isort rest_framework/schemas/inspectors.py for me and push again please? (We've got that lint error again.)

Thanks!

@sigvef

The documentation generator calls view.get_serializer() in order to inspect it for documentation generation. However, if get_serializer() throws an APIException (e.g. PermissionDenied), it doesn't get caught at the call site, but instead propagates up and aborts the entire view. With the try/except in this commit, the documentation generator instead gratiously ignores that particular view and moves on to the next one instead. Practical concequences of this commit is that the docs no longer break if any view's get_serializer(..) throws an APIException.

@sigvef

Cool, I didn't know about isort. Updated again!

@sigvef

Thanks for a fast review by the way!

carltongibson pushed a commit that referenced this pull request

Oct 6, 2017

Moved issue links to top for easier access. (Can move back later)

Strict JSON handling

Remove Django 1.8 & 1.9 from README and setup.py

Tickets migrated to 3.7.0 milestone.

Move issue links back to bottom.