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

Already on GitHub? Sign in to your account

Add `Freezer.error_handler_spec` for saving errors #17

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

singingwolfboy commented May 20, 2012

I needed to be able to generate a 404 page when freezing my Flask application, so I figured I'd add a generic way to make Frozen-Flask freeze any HTTP error page. The code works, and the unit test suite has been updated as well, but there is currently not documentation. I decided to mirror Flask.error_handler_spec as closely as possible, but I'm not certain that was the right decision, since it means that we have to use a nested dictionary due to blueprints providing their own exception handlers. I'm open to suggestions for other ways to implement this that make sense.

Add `Freezer.error_handler_spec` for saving errors
The structure of `Freezer.error_handler_spec` mimics that of
`Flask.error_handler_spec`:
    {blueprint_as_str: {status_code_as_int: url}}

If a user sets `Freezer.error_handler_spec` when freezing an
application, the URLs in the spec will be handled just like
the URLs from @freezer.register_generator functions, except
that the response objects will be checked to ensure that
they have the proper, non-200 status code.

If a URL is specified in both a register_generator function and in
the error_handler_spec, the register_generator function will win,
and the page will be tested for a 200 status code.
Collaborator

SimonSapin commented May 20, 2012

I’m not sure what this is trying to accomplish, nor what error_handler_spec represents. At least it seems not to be the same as in Flask.

What is the use case? If you want to have something saved at 404.html, why not have a "normal" route? The route would have a 200 status which is "wrong", but that only exists in Frozen-Flask. Or maybe do you use the same app both with Frozen-Flask and with "normal" Flask?

Contributor

singingwolfboy commented May 20, 2012

I would like to avoid "wrong" code when it's reasonably possible to do so, and serving a 404 handler with a 200 status code is "wrong" by my standards. My Flask app shouldn't know and shouldn't care that it will be frozen, and so shouldn't require workarounds for Frozen-Flask. I also don't want to have to provide a route in my Flask app for where the 404 page should end up: my using an errorhandler decorator, I can ensure that Flask will serve it for any unrouted URL; that way, if I decide to change where I want my static 404 file to live in the future, I only have to change one line in my freeze.py script, and I don't have to touch my Flask application. Finally, I also want to support an arbitrary framework for HTTP errors; maybe someone has their web server set up for HTTP Basic or Digest authentication, and wants to have a static 403 Forbidden page to serve for clients that fail the authentication test.

Collaborator

SimonSapin commented May 20, 2012

Ok about the idea, but I think that the execution in this first patch is backwards. In particular, it just sends a "normal" request to the app at some URI and expect it to trigger an error. This is easy for 404, but what about other status codes?

Instead, if we want the result of the error handlers, Frozen-Flask should just call the error handlers. Currently we only make "normal" requests at given URLs, and save to filenames derived from the URLs. Just like you can register functions that yield URLs to build, there would be a similar mechanism with (blueprint, status_code, filename). The first two identify an errror handler, the third value is where the result is saved.

Alternatively, hard-code the 404 status without blueprints, and only ask for the filename where to save. Do you really want other status codes?

I’m not sure of the details for a better replacement, but Freezer.error_handler_spec in its current form does not make sense.

it just sends a "normal" request to the app at some URI and expect it to trigger an error.

The goal is to render each error handler template and save it to an html file. (e.g. 404 → 404.html)

Using Frozen Flask, there should be a way to render:

@app.errorhandler(404)
def internal_error(error):
    return render_template('404.html'), 404

This is easy for 404, but what about other status codes?

Do you mean, sending /23ljkfkljsdf to render 404.html, but how to render 500? In fact, I don't think it's necessary to 'fake' a bad request. It's only necessary to call the error handler function to render the template.

Alternatively [to singingwolfboy's implementation], hard-code the 404 status without blueprints, and only ask for the filename where to save.

I think this would be a bad design decision. @singingwolfboy has a nearly-complete implementation which is not hardcoded.

Do you really want other status codes?

Yes. According to pingdom, the top 3 error codes:

  1. 500 (internal server error)
  2. 404 (not found)
  3. 403 (forbidden)

Frozen-Flask should just call the error handlers

completely agree. I think this is why @singingwolfboy created error_handler_spec property, so one can add more error handlers (e.g. 500, 403).

Freezer.error_handler_spec in its current form does not make sense.

I'm not a seasoned Python developer, so if you can explain maybe a bit more where the flaws are, maybe I could try to modify the pull request a bit.

Thinking about the main goal (render each error handler template and save it to an html file) , maybe it would be better to generate error handlers after and outside of the for url, endpoint in self._generate_all_urls(): loop. This loop is for valid urls, while error handlers seem to be technically for invalid urls.

(This could be why singingwolfboy's implementation seems to have a better replacement.)

[generate all urls] - current
[check endpoints] - current
[create error handler files] - addition

@SimonSapin , what do you think?

Collaborator

SimonSapin commented Dec 11, 2013

Hi @bergerjac. I think I agree with you on the design (call the error handlers directly rather than making "bad" requests to try and trigger them, do that out of the main loop). I don’t remember right now what was wrong with error_handler_spec (this was a long time ago.)

If you want to send another PR based on your design I’ll review that. Or let me know if you want me to comment further on what’s here so far, I’ll get back to it later.

@SimonSapin Flask has the ability to implement custom exceptions. I'm thinking that because Frozen Flask is primarily used for static site generation, that for the first iteration it's only necessary to implement functionality for standard http status codes (and NOT custom Flask custom exceptions).

I plan to use app.error_handlers which is a dict.

given the following code,

@app.errorhandler(404)
def page_not_found(error):
    return render_template('404.html'), 404

app.error_handlers would contain a key-value-pair:
404 : {page_not_found function}

# ... [generate all urls] - current
# ... [check endpoints] - current
for http_status_code, error_handler_function in self.app.error_handlers.iteritems():
   # ... sanity checks ...
   error_result = error_handler_function(http_status_code)
   # equates to: error_result = page_not_found(404)
   # ... save error_result to [http_status_code].html (e.g. 404.html) ...

please tell me what you think of the implementation overview.

Collaborator

SimonSapin commented Dec 12, 2013

The overview looks good, though you should at how Flask calls error handlers: how is the error argument created, and what is done with the return value. (The latter may involve make_response.)

Owner

tswast commented Sep 13, 2016

Closing because this PR has merge conflicts.

I think the idea of saving custom error pages is a good one, though, if you want to rebase the PR on latest master branch, hook into the Flask error handlers, and reopen.

@tswast tswast closed this Sep 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment