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

Feature: make HTTPException non-abstract #4034

Open
atemate opened this issue Sep 3, 2019 · 5 comments
Open

Feature: make HTTPException non-abstract #4034

atemate opened this issue Sep 3, 2019 · 5 comments

Comments

@atemate
Copy link
Contributor

atemate commented Sep 3, 2019

Long story short

Use-case: a server's handler uses a client to make a request to another server. I would like to re-use the response status code in the first server's response (the code below does not work as HTTPException's constructor does not accept parameter status_code):

    try:
        ...
    except ClientResponseError as e:
        status = e.status or 500
        raise web.HTTPException(text=e.message, status_code=status)

Of course I can use return web.Response(text=e.message, status=status), but web.Response cannot be throwed, however I need it in my applicatoin.

Expected behaviour

I would expect web.HTTPException not to be abstract so that one can construct and raise it with a dynamic status code.

Actual behaviour

one cannot create an instance of web.HTTPException

Steps to reproduce

(not a bug so no steps to reproduce)

Your environment

(not a bug so no environment description)

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2019

Thanks for the interesting question.
Just raising HTTPException for status code 400 is not the best idea, the expected exception type, in this case, is HTTPBadRequest.

Python's OSError has a similar problem. OSError(errno.ENOENT, 'file zzz.txt not found') actually creates FileNotFound(errno.ENOENT, 'file zzz.txt not found').
It works but confusing, there are edge cases when exception type downcasting is not performed.

I prefer a factory function, e.g. make_exception(status_code, *args, **kwargs) that finds a proper exception type based on passed status_code and instantiate the exception with args / kwargs.

Feel free to make a pull request. I have a feeling that the feature priority is low; don't want to work on it personally but happy to help with the review.

@webknjaz
Copy link
Member

webknjaz commented Sep 6, 2019

I prefer a factory function

I'd even say a @classmethod...

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2019

Earlier I used to apply class methods often enough but now I prefer just a function usually.

It gives better decomposability but provides the same functionality. The taste question though.
In aiohttp we have several factories and 3 public class methods, all of them are creators of self class and never are used for polymorphic type downcasting.

@serhiy-storchaka
Copy link
Contributor

Some HTTPException subclasses require additional arguments. For example HTTPRequestEntityTooLarge requires max_size and actual_size. Passing them to constructors of other exceptions is an error. How to use the factory function in such case?

@asvetlov
Copy link
Member

I thought that the factory accepts *args and **kwargs and passes them into the selected class constructor.
Not ideal, we miss type checking here...

As an alternative, we can do nothing. The issue is useful only for writing some kind of proxy that passes HTTP codes from internal HTTP calls to its own caller as is.
Otherwise, the proxy already has some transformation code, it can build the factory on its own.

aiohttp is used for building microservices/proxies/aggregators often enough, maybe the idea has own fellows. I have no strong preference here (and don't want to invest my own time in the implementation but open for review).

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

No branches or pull requests

5 participants