Sanic: Any plan to include 401?

Created on 19 May 2017  路  8Comments  路  Source: sanic-org/sanic

I am working on a simple authentication extension, I need to throw an exception of 401 unauthorized, what is the proper way to do that?

Should I subclass sanic.exceptions.SanicException as:

class Unauthorized(SanicException):
    status_code = 401

in my package, or is it a good idea to include that seemingly common HTTP status code in Sanic?

Further, to the best of my knowledge, there is no abort function like in Flask and bottle in Sanic, should Sanic pre-define all possible common status codes? Maybe an abort function is better than this.

Thanks.

Most helpful comment

I agree, we should consider changing the order of the parameters in SanicException, and assuming the message will be inferred from the code. However that would be a breaking change. I did add an abort function to the PR, which I think would be nice to have.

All 8 comments

I can draft a PR to either implement those status code as individual exceptions, or a generic abort function which raises specified status code with an SanicException.

In the latter case, we will need a pre-defined mapping for existing exceptions, to speed up the look up[1] time, It is actually not going every well with the current implementation.

Thinking more about that, concrete exception for each HTTP error code is not as flexible as the way Flask and bottle doing it (using integer), they both can work on the error code as plain old int, especially the error handling part 1, which allows user to use and catch any (im)possbie codes without restriction.

@pyx does this seem like an apt solution to you? https://github.com/channelcat/sanic/pull/740/files

Then you can do:

from sanic import Sanic
from sanic import response
from sanic.exceptions import SanicException

app = Sanic(__name__)

@app.route("/")
async def test(request):
    raise SanicException(status_code=401)
    return response.json({"test": True})


if __name__ == '__main__':
    app.run(host="0.0.0.0", port=8000)

@r0fls Thanks for the quick PR.

Raising a SanicException with status code is what I am going to use as of right now.

But the part that worries me is style and consistency between Sanic, extensions and end user code.

Let's say that, right now, I am using SanicException with 401 in my extension, to catch that, users of my extension will have to use a generic error handler, and tests status code inside that. To make it more elegant, I can provide an Unauthorized exception, so that my users can use:

@app.exception(my_extension.Unauthorized)
...

instead.
Which if I understand correctly, is the purpose of using concrete exceptions for each individual error code.
All seems good except, my implementation is not canonical, what If the user is using another extension with their implementation of the same error code? If later on, Sanic decide to have it's Unautherized exception, then there will be a mess within all three parties (Sanic, the extension, the end user) because they were using different implementations for the same thing.

Edit: and using interger based error handling does not have this problem, because we catch on a specific value (error code as int), not a specific type, which should have canonical implementation to avoid future breakage.

Edit2: my point is, if we are going the concrete exceptions route, defining all supported/possible error code exceptions will be necessary to avoid problems I mention above. Or we could just use plain old int as error code and provide abort. I am okay with either way.

BTW, I personally prefer using plain old integer:

  1. the HTTP error codes are standardized, there should be one on one mapping between the code and the error, so an integer is enough to identify a specific error.
  2. testing against integer is faster than matching on specific Exception type, and uses less memory, but I am only guessing here.
  3. it's future-proof without much code addition in Sanic's code base.

But there is a huge drawback, this change will break existing code. :(

I agree, we should consider changing the order of the parameters in SanicException, and assuming the message will be inferred from the code. However that would be a breaking change. I did add an abort function to the PR, which I think would be nice to have.

@r0fls thanks, I commented on that change.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

rainyear picture rainyear  路  3Comments

geekpy picture geekpy  路  4Comments

olalonde picture olalonde  路  3Comments

jasonab picture jasonab  路  3Comments

ZeeRoc picture ZeeRoc  路  3Comments