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

Fixing behavior issue when route raising Exception #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

staytime
Copy link

Background

Consider following code

@server.route("/", methods=("GET", ))
def landing_page(request):
  # some error
  foo = 0
  foo.set()
  return "OK?"

Presently _handle_request will return not thing,
Leading client hang until timeout.

Change

This pull request change are following

  1. Fixing undesired behavior by return a error page.
  2. Adding error message.
  3. Adding decorator "errorhandler" for customizing in user code.

staytime and others added 3 commits November 23, 2022 01:19
1. Adding Exception handling when processing route.
2. Adding decorator "errorhandler" for customizing in user code.
@ccrighton
Copy link

ccrighton commented Jun 28, 2024

@staytime @Gadgetoid This PR is heading in the right direction. However, Phew is a framework that is used by other applications. The behaviour in this PR while an improvement does not give control into the hands of the application developer.

Note that I'ved used my branch of Phew ccrighton/phew in the examples below.

Ideally, we would:

  1. Provide default unhandled exception handler that returns a 500 Internal Server Error (much like this PR)

  2. Provide a decorator that allows the developer to create their own handlers for different exceptions and provide their own default exception handler. This PR provides the decorator but does not support handling different exceptions with different handlers. It's possible that we just start with the generic case and expand it. However, it would be better to support the parameter in the errorhandler from the outset even if we just do something like @phew_app.errorhandler(Exception)

phew_app = server.Phew()

@phew_app.errorhandler(HTTPException)
def handle_exception(e):
   return render_template("500_generic.html", e=e), 500
  1. Support API errors as JSON. Could also be an extension to initial capability.

  2. Support a set of exception classes that can be raised in route handlers that will automatically generate a response object.

Again could be done after initial implementation without breaking anything.

class ImATeapot(HTTPException):
    code = 418
    description = 'I'm a teapot'

@phew_app.route("/", methods=["GET"])
def index(request)
    ...
    raise ImATeapot("an error occured")    
  1. Documentation and examples. Very important we add this with the initial implementation.

  2. Some other stuff that will come out in the wash when implementation is done :-).

It would be worth considering adding an abort(code) method that raises an HTTPException based on the given status code. This is supported by Flask. They also support code or exception e.g. errorhandler(404) and errorhandler(NotFound) are the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants