Fix empty str(exception) when initialized with kwargs by BoWuGit · Pull Request #2181 · Kludex/starlette

BoWuGit · 2023-06-11T09:21:08Z

Add a super().__init__ call to sub class of Python standard Exception, for two reasons:

It's convention to call it for sub class
Like the title says, when HTTPException is initialized with kwargs, str(HTTPException) is empty, which is just the case I met in this pytest error case.

Thanks for your attention.

zanieb · 2023-06-11T14:57:52Z

This makes sense to me after reading the upstream issue, thanks for contributing!

It might be nice to just add a new test case for this instead of changing the existing repr test case.

zanieb · 2023-06-11T14:58:21Z

tests/test_exceptions.py

+    detail = "Not Found: foo"
+    exception = HTTPException(status_code=404, detail=detail)
+    assert str(exception) == detail
+    assert repr(exception) == (f"HTTPException(status_code=404, detail='{detail}')")


Suggested change

assert repr(exception) == (f"HTTPException(status_code=404, detail='{detail}')")

assert repr(exception) == f"HTTPException(status_code=404, detail='{detail}')"

zanieb · 2023-06-11T14:58:30Z

tests/test_exceptions.py

+    reason = "Policy Violation"
+    exception = WebSocketException(code=1008, reason=reason)
+    assert str(exception) == reason
+    assert repr(exception) == (f"WebSocketException(code=1008, reason='{reason}')")


Suggested change

assert repr(exception) == (f"WebSocketException(code=1008, reason='{reason}')")

assert repr(exception) == f"WebSocketException(code=1008, reason='{reason}')"

Kludex

Please implement the __str__ instead of super().__init__(self.detail).

Also, on the __str__, I think we can have the same implementation as the __repr__.

On DRF, they use only the detail: https://github.com/encode/django-rest-framework/blob/376a5cbbba3f8df9c9db8c03a7c8fa2a6e6c05f4/rest_framework/exceptions.py#L99C1-L117

On flask, they use all the information: https://github.com/pallets/werkzeug/blob/82b5ac433c54b8d03cb0de9fdc16a4c58b935860/src/werkzeug/exceptions.py#L65-L85

zanieb · 2023-06-11T16:51:57Z

@Kludex for what it's worth, I don't think that __str__ and __repr__ implementations should match if you want standard behavior

❯ python -c "print(str(ValueError('test')))"
test
❯ python -c "print(repr(ValueError('test')))"
ValueError('test')

Using the __repr__ implementation would break expectations in downstream libraries that format exceptions e.g. https://github.com/PrefectHQ/prefect/blob/92e3e6de417bf542c9fca201fd396dba7301665c/src/prefect/states.py#L121 would display the exception type twice

The linked Flask code omits the type from the __str__ https://github.com/pallets/werkzeug/blob/82b5ac433c54b8d03cb0de9fdc16a4c58b935860/src/werkzeug/exceptions.py#L164-L170

Kludex · 2023-06-11T17:52:59Z

The linked Flask code omits the type from the str pallets/werkzeug@82b5ac4/src/werkzeug/exceptions.py#L164-L170

Yeah, I pasted the wrong part of the implementation. By "they use all the information" I mean that the __str__ uses all the arguments from the constructor.

Using the repr implementation would break expectations in downstream libraries that format exceptions e.g. PrefectHQ/prefect@92e3e6d/src/prefect/states.py#L121 would display the exception type twice

Fair enough.

Is there any literature that has a recommendation on how the __str__ should look like, or should we just copy flask? 🤔

BoWuGit · 2023-06-12T00:47:28Z

@Kludex Is there any literature that has a recommendation on how the str should look like, or should we just copy flask? 🤔

For sub class of Python Exception, str(Exception) defaults to str(Exception.args) if you don't call super().__init__ and don't override __str__ method, for example:

>>> str(HTTPException(404, "Not found"))
"(404, 'Not found')"

On DRF, they use only the detail: encode/django-rest-framework@376a5cb/rest_framework/exceptions.py#L99C1-L117

Yes, str(Exception) only displays the detail, and they also have a get_full_details method, so for simplicity consideration, I think we can refer Flask's implemention.

BoWuGit · 2023-06-18T04:05:52Z

I have re-implemented it referring Flask, https://github.com/pallets/werkzeug/blob/main/src/werkzeug/exceptions.py#L164. And I think it is somehow backward compatible in this way, because str(Exception) defaults to str(Exception.args) if you don't call super().__init__ and don't override __str__ method, so both code and detail should be outputted.

Kludex · 2023-06-19T07:49:35Z

Why do we need super().__init__()?

BoWuGit · 2023-06-19T12:30:52Z

I think it's desired practice in Python to always call super().__init__, and this pytest error case I met with is related with it more or less. Giving two links to explain:

Is it necessary to call super().init() explicitly in python?

If you override the init method of the superclass, then the init method of the subclass needs to explicitly call it if that is the intended behavior, yes.

Is calling the superclass constructor in a subclass really important?

If you add variables to self in the constructor of Class and don't call Class.init() in the constructor of Subclass, then these variables will not be in your Subclass object.

Kludex · 2023-06-20T05:54:18Z

this pytest-dev/pytest#11073 I met with is related with it more or less

Doesn't the __str__ already solves that?

Kludex · 2023-06-20T18:35:53Z

Thanks @BoWuGit 👍

BoWuGit · 2023-06-21T00:04:24Z

Happy to be merged, thanks again.

Co-authored-by: Marcelo Trylesinski <[email protected]>

This commit upgrades FastAPI to the newest version, adding an odd piece of code to our exception handling to enable the upgrade. The odd piece of code is needed because of FastAPI 0.108.0, which upgrades Starlette to a newer version than 0.29.0, which introduces a custom `__str__` method to `HTTPException`, which breaks our exception interface. See: Kludex/starlette#2181 for details. The odd piece of code simply circumvents the `__str__` implementation, and uses the old `Exception.__str__` behavior instead, ensuring backwards compatibility and thus ensuring that the upgrade is not a breaking change.

This commit upgrades FastAPI to the newest version, adding an odd piece of code to our exception handling to enable the upgrade. The odd piece of code is needed because of FastAPI 0.108.0, which upgrades Starlette to a newer version than 0.29.0, which introduces a custom `__str__` method to `HTTPException`, which breaks our exception interface. See: Kludex/starlette#2181 for details. The odd piece of code simply circumvents the `__str__` implementation, and uses the old `Exception.__str__` behavior instead, ensuring backwards compatibility and thus ensuring that the upgrade is not a breaking change. The newer version of FastAPI also required an bugfix upgrade of RAMQP, to version 9.1.1 as prior versions of RAMQP does not work with newer versions of FastAPI.

Fix empty str(exception) when initialized with kwargs

c7a3a24

BoWuGit marked this pull request as ready for review June 11, 2023 09:25

zanieb reviewed Jun 11, 2023

View reviewed changes

Kludex requested changes Jun 11, 2023

View reviewed changes

Kludex mentioned this pull request Jun 11, 2023

add __str__ method to HTTPException fastapi/fastapi#5879

Closed

Fix empty str(exception) when initialized with kwargs, review change

e2314dc

Kludex added 3 commits June 20, 2023 20:06

Merge branch 'master' into feature/support-kwargs-initialized-exception

ab10bd9

Remove the super().__init__() and refactor tests

5518508

Remove unused import

cfe4729

Kludex approved these changes Jun 20, 2023

View reviewed changes

Kludex enabled auto-merge (squash) June 20, 2023 18:34

Kludex merged commit 2168e47 into Kludex:master Jun 20, 2023

Kludex added a commit that referenced this pull request Jul 7, 2023

Fix empty str(exception) when initialized with kwargs (#2181)

c0fa36d

Co-authored-by: Marcelo Trylesinski <[email protected]>

	assert repr(exception) == (f"HTTPException(status_code=404, detail='{detail}')")
	assert repr(exception) == f"HTTPException(status_code=404, detail='{detail}')"

	assert repr(exception) == (f"WebSocketException(code=1008, reason='{reason}')")
	assert repr(exception) == f"WebSocketException(code=1008, reason='{reason}')"

Uh oh!

Conversation

BoWuGit commented Jun 11, 2023

Uh oh!

zanieb commented Jun 11, 2023

Uh oh!

zanieb Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

zanieb Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

Kludex left a comment

Choose a reason for hiding this comment

Uh oh!

zanieb commented Jun 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kludex commented Jun 11, 2023

Uh oh!

BoWuGit commented Jun 12, 2023

Uh oh!

BoWuGit commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kludex commented Jun 19, 2023

Uh oh!

BoWuGit commented Jun 19, 2023

Uh oh!

Kludex commented Jun 20, 2023

Uh oh!

Kludex commented Jun 20, 2023

Uh oh!

BoWuGit commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zanieb commented Jun 11, 2023 •

edited

Loading

BoWuGit commented Jun 18, 2023 •

edited

Loading