Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch APIException in doc generation #5443

Merged
merged 1 commit into from
Sep 25, 2017
Merged

Conversation

sigvef
Copy link
Contributor

@sigvef sigvef commented Sep 22, 2017

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.

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

from rest_framework import serializers
from rest_framework import exceptions
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@carltongibson carltongibson left a comment

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
Copy link
Collaborator

@carltongibson carltongibson Sep 25, 2017

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

sigvef commented Sep 25, 2017

Updated, ready for review.

Copy link
Collaborator

@carltongibson carltongibson left a comment

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
Copy link
Collaborator

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

Thanks!

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
Copy link
Contributor Author

sigvef commented Sep 25, 2017

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

@carltongibson carltongibson merged commit bf0fbd5 into encode:master Sep 25, 2017
@sigvef
Copy link
Contributor Author

sigvef commented Sep 25, 2017

Thanks for a fast review by the way!

carltongibson added a commit that referenced this pull request Sep 26, 2017
carltongibson added a commit that referenced this pull request Sep 26, 2017
carltongibson added a commit that referenced this pull request Sep 27, 2017
carltongibson added a commit that referenced this pull request Sep 28, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson pushed a commit that referenced this pull request Oct 6, 2017
* Set version number for 3.7.0 release

* Rename release notes section

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

* Add release note for #5273

* Add release note for #5440

* Add release note for #5265

Strict JSON handling

* Add release note for #5250

* Add release notes for #5170

* Add release notes for #5443

* Add release notes for #5448

* Add release notes for #5452

* Add release not for #5342

* Add release notes for 5454

* Add release notes for #5058 & #5457

Remove Django 1.8 & 1.9 from README and setup.py

* Release notes for merged 3.6.5 milestone tickets

Tickets migrated to 3.7.0 milestone.

* Add release notes for #5469

* Add release notes from AM 2ndOct

* Add final changes to the release notes.

* Add date and milestone link

Move issue links back to bottom.

* Update translations from transifex

* Begin releae anouncement

* Add release note for #5482

* 3.7 release announcement & related docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants