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

chore: add PermanentConnectionError to make BaseError slimmer (DEV-3192) #741

Merged
merged 30 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fc086f8
separate PermanentConnectionError from BaseError
jnussbaum Jan 16, 2024
bafb5aa
tidy up PermanentConnectionError and BaseError
jnussbaum Jan 16, 2024
f30d454
tidy up usages of BaseError
jnussbaum Jan 16, 2024
e6e2e1e
working version?
jnussbaum Jan 16, 2024
f6083a4
Merge branch 'main' into wip/fix-request-logging
jnussbaum Jan 16, 2024
f5c978e
beautify diff
jnussbaum Jan 16, 2024
d632381
fix e2e
jnussbaum Jan 16, 2024
b1cb58d
edit
jnussbaum Jan 16, 2024
8900253
fix docstring
jnussbaum Jan 16, 2024
f8a6a1b
refactor _try_network_action
jnussbaum Jan 16, 2024
dc814d9
fix darglint
jnussbaum Jan 16, 2024
ed3c735
_log_response(): log status code; make serialization clearer
jnussbaum Jan 16, 2024
4de415f
distinguish messages
jnussbaum Jan 16, 2024
47917a7
edit
jnussbaum Jan 16, 2024
8b5f528
fix: if session is reset, it needs the token
jnussbaum Jan 16, 2024
b69ba76
add _anonymize() method
jnussbaum Jan 17, 2024
ad00309
improve content logging of response
jnussbaum Jan 17, 2024
b5dd4f3
temporarily use builtin logging
jnussbaum Jan 17, 2024
b609aa0
don't print/log before raising PermanentConnectionError
jnussbaum Jan 17, 2024
bc1c318
Revert "temporarily use builtin logging"
jnussbaum Jan 17, 2024
8990651
Merge branch 'main' into wip/fix-request-logging
jnussbaum Jan 17, 2024
0d260fb
Merge branch 'main' into wip/fix-request-logging
jnussbaum Jan 17, 2024
a40a69a
clean up merge conflicts
jnussbaum Jan 17, 2024
b3aad00
remove unnecessary fields
jnussbaum Jan 17, 2024
848fc9c
revert unwanted changes
jnussbaum Jan 17, 2024
d32a326
test for error type, not for message
jnussbaum Jan 18, 2024
88d0704
be more precise in docstrings
jnussbaum Jan 18, 2024
b6fa4e6
- don't log pw when login fails
jnussbaum Jan 18, 2024
7689bf3
fix
jnussbaum Jan 18, 2024
142f6c0
explicit exception chaining: better link between the two tracebacks i…
jnussbaum Jan 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/dsp_tools/commands/project/create/project_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from dsp_tools.commands.project.models.propertyclass import PropertyClass
from dsp_tools.commands.project.models.resourceclass import ResourceClass
from dsp_tools.commands.project.models.user import User
from dsp_tools.models.exceptions import BaseError, UserError
from dsp_tools.models.exceptions import BaseError, PermanentConnectionError, UserError
from dsp_tools.models.helpers import Cardinality, Context, DateTimeStamp
from dsp_tools.models.langstring import LangString
from dsp_tools.utils.connection import Connection
Expand Down Expand Up @@ -55,7 +55,7 @@ def _create_project_on_server(
Returns:
a tuple of the remote project and the success status (True if everything went smoothly, False otherwise)
"""
with contextlib.suppress(BaseError):
with contextlib.suppress(PermanentConnectionError):
# the normal, expected case is that this block fails
project_local = Project(con=con, shortcode=shortcode)
project_remote: Project = project_local.read()
Expand Down
3 changes: 1 addition & 2 deletions src/dsp_tools/commands/xmlupload/resource_multimedia.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,8 @@ def _handle_media_upload(
logger.info(msg)
return resource_bitstream
except BaseError as err:
err_msg = err.orig_err_msg_from_api or err.message
msg = f"Unable to upload file '{bitstream.value}' of resource '{resource.label}' ({resource.res_id})"
print(f"{datetime.now()}: WARNING: {msg}: {err_msg}")
print(f"{datetime.now()}: WARNING: {msg}: {err.message}")
logger.warning(msg, exc_info=True)
return None

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def _upload_stash_item(
try:
con.post(route="/v2/values", data=payload)
except BaseError as err:
_log_unable_to_upload_link_value(err.orig_err_msg_from_api or err.message, stash.res_id, stash.prop_name)
_log_unable_to_upload_link_value(err.message, stash.res_id, stash.prop_name)
return False
logger.debug(f' Successfully uploaded resptr links of "{stash.prop_name}"')
return True
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,11 @@ def _log_unable_to_retrieve_resource(
# print the message to keep track of the cause for the failure
# apart from that; no action is necessary:
# this resource will remain in nonapplied_xml_texts, which will be handled by the caller
orig_err_msg = received_error.orig_err_msg_from_api or received_error.message
err_msg = (
f"Unable to upload XML texts of resource '{resource}', "
"because the resource cannot be retrieved from the DSP server."
)
print(f"{datetime.now()}: WARNING: {err_msg} Original error message: {orig_err_msg}")
print(f"{datetime.now()}: WARNING: {err_msg} Original error message: {received_error.message}")
logger.warning(err_msg, exc_info=True)


Expand All @@ -53,9 +52,8 @@ def _log_unable_to_upload_xml_resource(
# print the message to keep track of the cause for the failure
# apart from that; no action is necessary:
# this resource will remain in nonapplied_xml_texts, which will be handled by the caller
orig_err_msg = received_error.orig_err_msg_from_api or received_error.message
err_msg = f"Unable to upload the xml text of '{prop_name}' of resource '{stashed_resource_id}'."
print(f"{datetime.now()}: WARNING: {err_msg} Original error message: {orig_err_msg}")
print(f"{datetime.now()}: WARNING: {err_msg} Original error message: {received_error.message}")
logger.warning(err_msg, exc_info=True)


Expand Down
2 changes: 1 addition & 1 deletion src/dsp_tools/commands/xmlupload/xmlupload.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ def _create_resource(
try:
return resource_create_client.create_resource(resource, bitstream_information)
except BaseError as err:
err_msg = err.orig_err_msg_from_api or err.message
err_msg = err.message
print(f"{datetime.now()}: WARNING: Unable to create resource '{resource.label}' ({resource.res_id}): {err_msg}")
log_msg = (
f"Unable to create resource '{resource.label}' ({resource.res_id})\n"
Expand Down
61 changes: 13 additions & 48 deletions src/dsp_tools/models/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,63 +1,17 @@
import contextlib
import json
from dataclasses import dataclass
from pathlib import Path
from typing import Optional

import regex


@dataclass
class BaseError(Exception):
"""
A basic error class for DSP-TOOLS.

Attributes:

message: A message that describes the error
status_code: HTTP status code of the response from DSP-API (only applicable if error comes from DSP-API)
json_content_of_api_response: The message that DSP-API returns (only applicable if error comes from DSP-API)
orig_err_msg_from_api: Original error message that DSP-API returns (only applicable if error comes from DSP-API)
reason_from_api: Reason for the failure that DSP-API returns (only applicable if error comes from DSP-API)
api_route: The route that was called (only applicable if the error comes from DSP-API)
"""

message: str
status_code: Optional[int]
json_content_of_api_response: Optional[str]
orig_err_msg_from_api: Optional[str] = None
reason_from_api: Optional[str]
api_route: Optional[str]

def __init__(
self,
message: str,
status_code: Optional[int] = None,
json_content_of_api_response: Optional[str] = None,
reason_from_api: Optional[str] = None,
api_route: Optional[str] = None,
) -> None:
"""
A basic error class for DSP-TOOLS.

Args:
message: A message that describes the error
status_code: HTTP status code of the response from DSP-API (only applicable if error comes from DSP-API)
json_content_of_api_response: The message that DSP-API returns (only applicable if error comes from DSP-API)
reason_from_api: Reason for the failure that DSP-API returns (only applicable if error comes from DSP-API)
api_route: The route that was called (only applicable if the error comes from DSP-API)
"""
super().__init__()
self.message = message
self.status_code = status_code
if json_content_of_api_response:
self.json_content_of_api_response = json_content_of_api_response
with contextlib.suppress(json.JSONDecodeError):
parsed_json = json.loads(json_content_of_api_response)
if "knora-api:error" in parsed_json:
knora_api_error = parsed_json["knora-api:error"]
knora_api_error = regex.sub(r"^dsp\.errors\.[A-Za-z]+?: ?", "", knora_api_error)
self.orig_err_msg_from_api = knora_api_error
self.reason_from_api = reason_from_api
self.api_route = api_route

jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
def __str__(self) -> str:
return self.message
Expand Down Expand Up @@ -128,6 +82,17 @@ class UserError(BaseError):
"""


class PermanentConnectionError(BaseError):
"""
This error is raised when all attempts to reconnect to DSP have failed.

Attributes:
message: A message that describes the error
"""

message: str


class XmlUploadError(Exception):
"""
jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
Represents an error raised in the context of the XML import.
Expand Down
48 changes: 27 additions & 21 deletions src/dsp_tools/utils/connection_live.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from requests import JSONDecodeError, ReadTimeout, RequestException, Response, Session
from urllib3.exceptions import ReadTimeoutError

from dsp_tools.models.exceptions import BaseError
from dsp_tools.models.exceptions import BaseError, PermanentConnectionError, UserError
from dsp_tools.utils.create_logger import get_logger
from dsp_tools.utils.set_encoder import SetEncoder

Expand Down Expand Up @@ -51,19 +51,18 @@ def login(self, email: str, password: str) -> None:
password: password of the user

Raises:
BaseError: if DSP-API returns no token with the provided user credentials
PermanentConnectionError: if DSP-API returns no token with the provided user credentials
"""
response = self.post(
route="/v2/authentication",
data={"email": email, "password": password},
)
if not response.get("token"):
raise BaseError(
f"Error when trying to login with user '{email}' and password '{password} "
f"on server '{self.server}'",
json_content_of_api_response=json.dumps(response),
api_route="/v2/authentication",
jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
err_msg = f"Username and/or password are not valid on server '{self.server}'"
try:
response = self.post(
route="/v2/authentication",
data={"email": email, "password": password},
)
except PermanentConnectionError:
raise UserError(err_msg)
if not response.get("token"):
raise UserError(err_msg)
jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
self.token = response["token"]
self.session.headers["Authorization"] = f"Bearer {self.token}"

Expand Down Expand Up @@ -134,6 +133,9 @@ def post(

Returns:
response from server

Raises:
PermanentConnectionError: if the server returns a permanent error
"""
if not route.startswith("/"):
route = f"/{route}"
Expand Down Expand Up @@ -178,6 +180,9 @@ def get(

Returns:
response from server

Raises:
PermanentConnectionError: if the server returns a permanent error
"""
if not route.startswith("/"):
route = f"/{route}"
Expand Down Expand Up @@ -219,6 +224,9 @@ def put(

Returns:
response from server

Raises:
PermanentConnectionError: if the server returns a permanent error
"""
if not route.startswith("/"):
route = f"/{route}"
Expand Down Expand Up @@ -264,6 +272,9 @@ def delete(

Returns:
response from server

Raises:
PermanentConnectionError: if the server returns a permanent error
"""
if not route.startswith("/"):
route = f"/{route}"
Expand Down Expand Up @@ -309,7 +320,7 @@ def _try_network_action(self, action: Callable[[], Response]) -> Response:
action: a lambda with the code to be executed, or a function

Raises:
BaseError: if the action fails permanently
PermanentConnectionError: if the server returns a permanent error
unexpected exceptions: if the action fails with an unexpected exception

Returns:
Expand All @@ -325,7 +336,7 @@ def _try_network_action(self, action: Callable[[], Response]) -> Response:
self.session.close()
self.session = Session()
self.session.headers["Authorization"] = f"Bearer {self.token}"
self._log_and_sleep(reason="Network Error", retry_counter=i)
self._log_and_sleep(reason="Connection Error raised", retry_counter=i)
continue

self._log_response(response)
Expand All @@ -336,13 +347,8 @@ def _try_network_action(self, action: Callable[[], Response]) -> Response:
time.sleep(2**i)
continue
elif response.status_code != HTTP_OK:
raise BaseError(
message="Permanently unable to execute the network action. See logs for more details.",
status_code=response.status_code,
json_content_of_api_response=response.text,
reason_from_api=response.reason,
api_route=response.url,
jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
)
msg = "Permanently unable to execute the network action. See logs for more details."
raise PermanentConnectionError(msg)
else:
return response

Expand Down
7 changes: 3 additions & 4 deletions test/e2e/utils/test_connection_live.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import pytest

from dsp_tools.models.exceptions import BaseError
from dsp_tools.models.exceptions import BaseError, PermanentConnectionError
from dsp_tools.utils.connection_live import ConnectionLive

# ruff: noqa: PT009 (pytest-unittest-assertion) (remove this line when pytest is used instead of unittest)
Expand Down Expand Up @@ -38,9 +38,8 @@ def test_log_in_and_out(self) -> None:
self.assertIsNotNone(con.token)
con.logout()
self.assertIsNone(con.token)
self.assertRaisesRegex(
BaseError, "Permanently unable to execute the network action", con.login, "invalid", "invalid"
)
with self.assertRaises(PermanentConnectionError):
con.login("invalid", "invalid")

def test_get(self) -> None:
res = self.con.get("/ontology/0001/anything/simple/v2")
Expand Down