Skip to content

Stop hiding cause of last exception #224

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.

Sign up for GitHub

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

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

Stop hiding cause of last exception #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56d9b6e
Stop hiding cause of last exception
pquentin Jan 30, 2025
30e13b4
Add sphinx.configuration
pquentin Jan 30, 2025
1322048
Fix Sphinx conf.py path
pquentin Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .readthedocs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ python:
- develop

sphinx:
configuration: docs/sphinx/conf.py
fail_on_warning: true
18 changes: 0 additions & 18 deletions elastic_transport/_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ def __repr__(self) -> str:
parts.append(f"errors={self.errors!r}")
return "{}({})".format(self.__class__.__name__, ", ".join(parts))

def __str__(self) -> str:
return str(self.message)

Comment on lines -53 to -55
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since message is passed in super.__init__(message) and we removed the custom __str__ implementation below, this is already the default implementation.

In Python, __str__ isn't helpful. If you need to know the class name, then __repr__ is required, and our __repr__ implementation contains all information about the current exception, including errors.

>>> e = ValueError("abandon ship!")
>>> str(e)
'abandon ship!'
>>> repr(e)
"ValueError('abandon ship!')"


class SniffingError(TransportError):
"""Error that occurs during the sniffing of nodes"""
Expand All @@ -67,29 +64,14 @@ class SerializationError(TransportError):
class ConnectionError(TransportError):
"""Error raised by the HTTP connection"""

def __str__(self) -> str:
if self.errors:
return f"Connection error caused by: {self.errors[0].__class__.__name__}({self.errors[0]})"
return "Connection error"

Comment on lines -70 to -74
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This __str__ implementation tries to be helpful but hides the current exception to focus on errors, which are past attempts. This comment applies to the other subclasses as well.


class TlsError(ConnectionError):
"""Error raised by during the TLS handshake"""

def __str__(self) -> str:
if self.errors:
return f"TLS error caused by: {self.errors[0].__class__.__name__}({self.errors[0]})"
return "TLS error"


class ConnectionTimeout(TransportError):
"""Connection timed out during an operation"""

def __str__(self) -> str:
if self.errors:
return f"Connection timeout caused by: {self.errors[0].__class__.__name__}({self.errors[0]})"
return "Connection timed out"


class ApiError(Exception):
"""Base-class for clients that raise errors due to a response such as '404 Not Found'"""
Expand Down
100 changes: 87 additions & 13 deletions tests/async_/test_async_transport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@
from tests.conftest import AsyncDummyNode


def exception_to_dict(exc: TransportError) -> dict:
return {
"type": exc.__class__.__name__,
"message": exc.message,
"errors": [exception_to_dict(e) for e in exc.errors],
}
Comment on lines +48 to +53
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all the information exposed by __repr__, but in a more structured way which is nicer to test for.



@pytest.mark.asyncio
async def test_async_transport_httpbin(httpbin_node_config):
t = AsyncTransport([httpbin_node_config], meta_header=False)
Expand Down Expand Up @@ -139,14 +147,39 @@ async def test_request_will_fail_after_x_retries():
)
],
node_class=AsyncDummyNode,
max_retries=0,
)

with pytest.raises(ConnectionError) as e:
await t.perform_request("GET", "/")

assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
}

# max_retries=2
with pytest.raises(ConnectionError) as e:
await t.perform_request("GET", "/", max_retries=2)

assert 4 == len(t.node_pool.get().calls)
assert len(e.value.errors) == 3
assert all(isinstance(error, ConnectionError) for error in e.value.errors)
assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
],
}


@pytest.mark.parametrize("retry_on_timeout", [True, False])
Expand Down Expand Up @@ -174,15 +207,30 @@ async def test_retry_on_timeout(retry_on_timeout):
)

if retry_on_timeout:
with pytest.raises(ConnectionError) as e:
with pytest.raises(TransportError) as e:
await t.perform_request("GET", "/")
assert len(e.value.errors) == 1
assert isinstance(e.value.errors[0], ConnectionTimeout)

assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "error!",
"errors": [
{
"type": "ConnectionTimeout",
"message": "abandon ship",
"errors": [],
}
],
}

else:
with pytest.raises(ConnectionTimeout) as e:
with pytest.raises(TransportError) as e:
await t.perform_request("GET", "/")
assert len(e.value.errors) == 0

assert exception_to_dict(e.value) == {
"type": "ConnectionTimeout",
"message": "abandon ship",
"errors": [],
}


@pytest.mark.asyncio
Expand Down Expand Up @@ -254,8 +302,27 @@ async def test_failed_connection_will_be_marked_as_dead():
await t.perform_request("GET", "/")
assert 0 == len(t.node_pool._alive_nodes)
assert 2 == len(t.node_pool._dead_nodes.queue)
assert len(e.value.errors) == 3
assert all(isinstance(error, ConnectionError) for error in e.value.errors)
assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
],
}


@pytest.mark.asyncio
Expand Down Expand Up @@ -602,7 +669,12 @@ def sniff_error(*_):

with pytest.raises(TransportError) as e:
await t.perform_request("GET", "/")
assert str(e.value) == "This is an error!"

assert exception_to_dict(e.value) == {
"type": "TransportError",
"message": "This is an error!",
"errors": [],
}

assert t._last_sniffed_at == last_sniffed_at
assert t._sniffing_task.done()
Expand All @@ -628,9 +700,11 @@ async def test_sniff_on_start_no_results_errors(sniff_callback):
with pytest.raises(SniffingError) as e:
await t._async_call()

assert (
str(e.value) == "No viable nodes were discovered on the initial sniff attempt"
)
assert exception_to_dict(e.value) == {
"type": "SniffingError",
"message": "No viable nodes were discovered on the initial sniff attempt",
"errors": [],
}


@pytest.mark.parametrize("pool_size", [1, 8])
Expand Down
7 changes: 5 additions & 2 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.

import copy
import hashlib
import logging
import socket
Expand All @@ -40,7 +41,8 @@ def __init__(self, config: NodeConfig):
def perform_request(self, *args, **kwargs):
self.calls.append((args, kwargs))
if self.exception:
raise self.exception
# Raising the same exception can cause recursion errors when exceptions are linked together
raise copy.deepcopy(self.exception)
meta = ApiResponseMeta(
node=self.config,
duration=0.0,
Expand All @@ -55,7 +57,8 @@ class AsyncDummyNode(DummyNode):
async def perform_request(self, *args, **kwargs):
self.calls.append((args, kwargs))
if self.exception:
raise self.exception
# Raising the same exception can cause recursion errors when exceptions are linked together
raise copy.deepcopy(self.exception)
meta = ApiResponseMeta(
node=self.config,
duration=0.0,
Expand Down
115 changes: 80 additions & 35 deletions tests/test_transport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@
from tests.conftest import DummyNode


def exception_to_dict(exc: TransportError) -> dict:
return {
"type": exc.__class__.__name__,
"message": exc.message,
"errors": [exception_to_dict(e) for e in exc.errors],
}


def test_transport_close_node_pool():
t = Transport([NodeConfig("http", "localhost", 443)])
with mock.patch.object(t.node_pool.all()[0], "close") as node_close:
Expand Down Expand Up @@ -138,37 +146,33 @@ def test_request_will_fail_after_x_retries():
with pytest.raises(ConnectionError) as e:
t.perform_request("GET", "/")

assert 1 == len(t.node_pool.get().calls)
assert len(e.value.errors) == 0

# max_retries=3
t = Transport(
[
NodeConfig(
"http",
"localhost",
80,
_extras={"exception": ConnectionError("abandon ship")},
)
],
node_class=DummyNode,
max_retries=3,
)

with pytest.raises(ConnectionError) as e:
t.perform_request("GET", "/")

assert 4 == len(t.node_pool.get().calls)
assert len(e.value.errors) == 3
assert all(isinstance(error, ConnectionError) for error in e.value.errors)
assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
}

# max_retries=2 in perform_request()
with pytest.raises(ConnectionError) as e:
t.perform_request("GET", "/", max_retries=2)

assert 7 == len(t.node_pool.get().calls)
assert len(e.value.errors) == 2
assert all(isinstance(error, ConnectionError) for error in e.value.errors)
assert 4 == len(t.node_pool.get().calls)
assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
],
}


@pytest.mark.parametrize("retry_on_timeout", [True, False])
Expand Down Expand Up @@ -197,13 +201,28 @@ def test_retry_on_timeout(retry_on_timeout):
if retry_on_timeout:
with pytest.raises(ConnectionError) as e:
t.perform_request("GET", "/")
assert len(e.value.errors) == 1
assert isinstance(e.value.errors[0], ConnectionTimeout)

assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "error!",
"errors": [
{
"type": "ConnectionTimeout",
"message": "abandon ship",
"errors": [],
}
],
}

else:
with pytest.raises(ConnectionTimeout) as e:
t.perform_request("GET", "/")
assert len(e.value.errors) == 0

assert exception_to_dict(e.value) == {
"type": "ConnectionTimeout",
"message": "abandon ship",
"errors": [],
}


def test_retry_on_status():
Expand Down Expand Up @@ -273,8 +292,27 @@ def test_failed_connection_will_be_marked_as_dead():
t.perform_request("GET", "/")
assert 0 == len(t.node_pool._alive_nodes)
assert 2 == len(t.node_pool._dead_nodes.queue)
assert len(e.value.errors) == 3
assert all(isinstance(error, ConnectionError) for error in e.value.errors)
assert exception_to_dict(e.value) == {
"type": "ConnectionError",
"message": "abandon ship",
"errors": [
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
{
"type": "ConnectionError",
"message": "abandon ship",
"errors": [],
},
],
}


def test_resurrected_connection_will_be_marked_as_live_on_success():
Expand Down Expand Up @@ -603,7 +641,12 @@ def sniff_error(*_):

with pytest.raises(TransportError) as e:
t.perform_request("GET", "/")
assert str(e.value) == "This is an error!"

assert exception_to_dict(e.value) == {
"type": "TransportError",
"message": "This is an error!",
"errors": [],
}

assert t._last_sniffed_at == last_sniffed_at
assert t._sniffing_lock.locked() is False
Expand All @@ -620,9 +663,11 @@ def test_sniff_on_start_no_results_errors():
sniff_callback=lambda *_: [],
)

assert (
str(e.value) == "No viable nodes were discovered on the initial sniff attempt"
)
assert exception_to_dict(e.value) == {
"type": "SniffingError",
"message": "No viable nodes were discovered on the initial sniff attempt",
"errors": [],
}


@pytest.mark.parametrize("pool_size", [1, 8])
Expand Down