refactor: diverse refactorings (DEV-2559) (#480)

docs/cli-commands.md

@@ -61,7 +61,7 @@ The following options are available:
 - `-V` | `--validate-only` (optional): validate the JSON file without creating it on the DSP server
 - `-l` | `--lists-only` (optional): create only the lists (prerequisite: the project exists on the server)
 - `-v` | `--verbose` (optional): print more information about the progress to the console
-- `-d` | `--dump` (optional): dump test files for DSP-API requests
+- `-d` | `--dump` (optional): write every request to DSP-API into a file
 
 The defaults are intended for local testing: 
 

pyproject.toml

@@ -91,12 +91,33 @@ addopts = ["--import-mode=importlib"]
 
 
 [tool.mypy]
-ignore_missing_imports = true              # TODO: deactivate this
+ignore_missing_imports = true                     # TODO: deactivate this
 show_column_numbers = true
 strict = true
 exclude = [
-    "src/dsp_tools/models",                # TODO: activate this
-    "src/dsp_tools/import_scripts",        # TODO: activate this
+    "src/dsp_tools/import_scripts",               # TODO: activate this
+    "src/dsp_tools/models/connection.py",         # TODO: activate this
+    "src/dsp_tools/models/exceptions.py",         # TODO: activate this
+    "src/dsp_tools/models/group.py",              # TODO: activate this
+    "src/dsp_tools/models/helpers.py ",           # TODO: activate this
+    "src/dsp_tools/models/langstring.py",         # TODO: activate this
+    "src/dsp_tools/models/listnode.py",           # TODO: activate this
+    "src/dsp_tools/models/model.py",              # TODO: activate this
+    "src/dsp_tools/models/ontology.py",           # TODO: activate this
+    "src/dsp_tools/models/permission.py",         # TODO: activate this
+    "src/dsp_tools/models/project.py",            # TODO: activate this
+    "src/dsp_tools/models/projectContext.py",     # TODO: activate this
+    "src/dsp_tools/models/propertyclass.py",      # TODO: activate this
+    "src/dsp_tools/models/propertyelement.py",    # TODO: activate this
+    "src/dsp_tools/models/resource.py",           # TODO: activate this
+    "src/dsp_tools/models/resourceclass.py",      # TODO: activate this
+    "src/dsp_tools/models/user.py",               # TODO: activate this
+    "src/dsp_tools/models/value.py",              # TODO: activate this
+    "src/dsp_tools/models/xmlbitstream.py",       # TODO: activate this
+    "src/dsp_tools/models/xmlpermission.py",      # TODO: activate this
+    "src/dsp_tools/models/xmlproperty.py",        # TODO: activate this
+    "src/dsp_tools/models/xmlresource.py",        # TODO: activate this
+    "src/dsp_tools/models/xmlvalue.py",           # TODO: activate this
 ]
 
 
@@ -133,6 +154,7 @@ disable = [
     "no-else-return",                      # would be less readable
     "no-else-raise",                       # would be less readable
     "logging-fstring-interpolation",       # would be less readable
+    "duplicate-code",                      # too many false positives
     "invalid-name",                        # TODO: activate this
     "too-many-arguments",                  # TODO: activate this
     "too-many-branches",                   # TODO: activate this

src/dsp_tools/cli.py

@@ -85,7 +85,7 @@ def _make_parser(
         help="create only the lists (prerequisite: the project exists on the server)",
     )
     parser_create.add_argument("-v", "--verbose", action="store_true", help=verbose_text)
-    parser_create.add_argument("-d", "--dump", action="store_true", help="dump test files for DSP-API requests")
+    parser_create.add_argument("-d", "--dump", action="store_true", help="write every request to DSP-API into a file")
     parser_create.add_argument("project_definition", help="path to the JSON project file")
 
     # get

src/dsp_tools/models/bitstream.py

@@ -5,7 +5,7 @@
 from dsp_tools.models.permission import Permissions
 
 
-@dataclass
+@dataclass(frozen=True)
 class Bitstream:
     """
     Represents a bitstream object (file) which is attached to a resource.

src/dsp_tools/models/helpers.py

@@ -13,7 +13,7 @@
 #
 
 
-@dataclass
+@dataclass(frozen=True)
 class OntoIri:
     """
     Holds an ontology IRI

src/dsp_tools/models/propertyclass.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-class-docstring,missing-function-docstring,duplicate-code
+# pylint: disable=missing-class-docstring,missing-function-docstring
 
 import json
 from typing import Any, Optional, Sequence, Union

src/dsp_tools/models/resource.py

@@ -49,7 +49,7 @@ def default(self, o) -> str:
         return json.JSONEncoder.default(self, o)
 
 
-@dataclass
+@dataclass(frozen=True)
 class Propinfo:
     valtype: Type
     cardinality: Cardinality

src/dsp_tools/models/resourceclass.py

@@ -5,7 +5,7 @@
     * "ResourceClass" is the main class representing a DSP resource class.
 """
 
-# pylint: disable=missing-class-docstring,missing-function-docstring,too-many-instance-attributes,duplicate-code
+# pylint: disable=missing-class-docstring,missing-function-docstring,too-many-instance-attributes
 
 import json
 from enum import Enum

src/dsp_tools/models/set_encoder.py

@@ -1,12 +1,13 @@
 import json
+from typing import Any, Union
 
 from dsp_tools.models.helpers import Context, OntoIri
 
 
 class SetEncoder(json.JSONEncoder):
     """Encoder used to serialize objects to JSON that would by default not be serializable"""
 
-    def default(self, o):
+    def default(self, o: Union[set[Any], Context, OntoIri]) -> Any:
         if isinstance(o, set):
             return list(o)
         elif isinstance(o, Context):

src/dsp_tools/models/sipi.py

@@ -1,21 +1,38 @@
-# pylint: disable=too-few-public-methods
-
+from dataclasses import dataclass
+from datetime import datetime
+import json
 import os
+from pathlib import Path
 from typing import Any
 
 import requests
 
 from dsp_tools.models.connection import check_for_api_error
 
-from dsp_tools.utils.shared import http_call_with_retry
-
 
+@dataclass(frozen=True)
 class Sipi:
-    """Represents the Sipi instance"""
+    """
+    A Sipi instance represents a connection to a SIPI server.
+
+    Attributes:
+        sipi_server: address of the server, e.g https://iiif.dasch.swiss
+        token: session token received by the server after login
+        dump: if True, every request is written into a file
+        dump_directory: directory where the HTTP requests are written
+    """
 
-    def __init__(self, sipi_server: str, token: str):
-        self.sipi_server = sipi_server
-        self.token = token
+    sipi_server: str
+    token: str
+    dump: bool = False
+    dump_directory = Path("HTTP requests")
+
+    def __post_init__(self) -> None:
+        """
+        Create dumping directory (if applicable)
+        """
+        if self.dump:
+            self.dump_directory.mkdir(exist_ok=True)
 
     def upload_bitstream(self, filepath: str) -> dict[Any, Any]:
         """
@@ -28,16 +45,65 @@ def upload_bitstream(self, filepath: str) -> dict[Any, Any]:
             API response
         """
         with open(filepath, "rb") as bitstream_file:
-            files = {
-                "file": (os.path.basename(filepath), bitstream_file),
-            }
-            response = http_call_with_retry(
-                action=requests.post,
-                initial_timeout=5 * 60,
-                url=self.sipi_server + "/upload",
-                headers={"Authorization": "Bearer " + self.token},
+            files = {"file": (os.path.basename(filepath), bitstream_file)}
+            url = self.sipi_server + "/upload"
+            headers = {"Authorization": "Bearer " + self.token}
+            timeout = 5 * 60
+            response = requests.post(
+                url=url,
+                headers=headers,
                 files=files,
+                timeout=timeout,
             )
+            if self.dump:
+                self.write_request_to_file(
+                    method="POST",
+                    url=url,
+                    headers=headers,
+                    filepath=filepath,
+                    timeout=timeout,
+                    response=response,
+                )
         check_for_api_error(response)
         res: dict[Any, Any] = response.json()
         return res
+
+    def write_request_to_file(
+        self,
+        method: str,
+        url: str,
+        headers: dict[str, str],
+        filepath: str,
+        timeout: int,
+        response: requests.Response,
+    ) -> None:
+        """
+        Write the request and response to a file.
+
+        Args:
+            method: HTTP method of the request (GET, POST, PUT, DELETE)
+            url: complete URL (server + route of SIPI) that was called
+            headers: headers of the HTTP request
+            filepath: path to the file that was uploaded
+            timeout: timeout of the HTTP request
+            response: response of the server
+        """
+        if response.status_code == 200:
+            _return = response.json()
+        else:
+            _return = {"status": response.status_code, "message": response.text}
+        dumpobj = {
+            "SIPI server": self.sipi_server,
+            "url": url,
+            "method": method,
+            "filepath": filepath,
+            "timeout": timeout,
+            "headers": headers,
+            "return-headers": dict(response.headers),
+            "return": _return,
+        }
+        timestamp = datetime.now().strftime("%Y-%m-%d %H.%M.%S.%f")
+        route_for_filename = url.replace(self.sipi_server, "").replace("/", "_")
+        filename = f"{timestamp} {method} SIPI {route_for_filename} {filepath.replace('/', '_')}.json"
+        with open(self.dump_directory / filename, "w", encoding="utf-8") as f:
+            json.dump(dumpobj, f, indent=4)

src/dsp_tools/models/xmlallow.py

@@ -10,7 +10,7 @@ class XmlAllow:
     _group: str
     _permission: str
 
-    def __init__(self, node: etree.Element, project_context: ProjectContext) -> None:
+    def __init__(self, node: etree._Element, project_context: ProjectContext) -> None:
         """
         Constructor which parses the XML DOM allow element
 
@@ -28,9 +28,10 @@ def __init__(self, node: etree.Element, project_context: ProjectContext) -> None
                 if tmp[0] == "knora-admin" and tmp[1] in sysgroups:
                     self._group = node.attrib["group"]
                 else:
-                    self._group = project_context.group_map.get(node.attrib["group"])
-                    if self._group is None:
+                    _group = project_context.group_map.get(node.attrib["group"])
+                    if _group is None:
                         raise XmlError(f'Group "{node.attrib["group"]}" is not known: Cannot find project!')
+                    self._group = _group
             else:
                 if project_context.project_name is None:
                     raise XmlError("Project shortcode has not been set in ProjectContext")
@@ -40,6 +41,8 @@ def __init__(self, node: etree.Element, project_context: ProjectContext) -> None
                 self._group = "knora-admin:" + node.attrib["group"]
             else:
                 raise XmlError(f'Group "{node.attrib["group"]}" is not known: ')
+        if not node.text:
+            raise XmlError("No permission set specified")
         self._permission = node.text
 
     @property

src/dsp_tools/utils/project_create.py

@@ -975,8 +975,8 @@ def create_project(
     server: str,
     user_mail: str,
     password: str,
-    verbose: bool,
-    dump: bool,
+    verbose: bool = False,
+    dump: bool = False,
 ) -> bool:
     """
     Creates a project from a JSON project file on a DSP server.
@@ -991,7 +991,7 @@ def create_project(
         user_mail: a username (e-mail) who has the permission to create a project
         password: the user's password
         verbose: prints more information if set to True
-        dump: dumps test files (JSON) for DSP API requests if set to True
+        dump: if True, write every request to DSP-API into a file
 
     Raises:
         UserError:

src/dsp_tools/utils/project_create_lists.py

@@ -154,7 +154,7 @@ def create_lists(
         server: URL of the DSP server
         user: Username (e-mail) for the DSP server, must have the permissions to create a project
         password: Password of the user
-        dump: if True, the request is dumped as JSON (used for testing)
+        dump: if True, write every request to DSP-API into a file
 
     Raises:
         UserError:

src/dsp_tools/utils/project_get.py

@@ -18,7 +18,7 @@ def get_project(
     server: str,
     user: str,
     password: str,
-    verbose: bool,
+    verbose: bool = False,
 ) -> bool:
     """
     This function writes a project from a DSP server into a JSON file.

src/dsp_tools/utils/shared.py

@@ -144,7 +144,7 @@ def try_network_action(
                 return action(*args, **kwargs)
             else:
                 return action()
-        except (TimeoutError, ReadTimeout):
+        except (TimeoutError, ReadTimeout, ReadTimeoutError):
             msg = f"Timeout Error: Try reconnecting to DSP server, next attempt in {2 ** i} seconds..."
             print(f"{datetime.now().isoformat()}: {msg}")
             logger.error(f"{msg} {action_as_str} (retry-counter i={i})", exc_info=True)

src/dsp_tools/utils/stack_handling.py

@@ -18,7 +18,7 @@
 logger = get_logger(__name__)
 
 
-@dataclass
+@dataclass(frozen=True)
 class StackConfiguration:
     """
     Groups together configuration information for the StackHandler.

src/dsp_tools/utils/xml_upload.py

@@ -488,10 +488,10 @@ def xml_upload(
     password: str,
     imgdir: str,
     sipi: str,
-    verbose: bool,
-    incremental: bool,
-    save_metrics: bool,
-    preprocessing_done: bool,
+    verbose: bool = False,
+    incremental: bool = False,
+    save_metrics: bool = False,
+    preprocessing_done: bool = False,
 ) -> bool:
     """
     This function reads an XML file and imports the data described in it onto the DSP server.
@@ -537,14 +537,16 @@ def xml_upload(
     metrics: list[MetricRecord] = []
     preparation_start = datetime.now()
 
-    # Connect to the DaSCH Service Platform API and get the project context
+    # establish connection to DSP server
     con = login(server=server, user=user, password=password)
+    sipi_server = Sipi(sipi, con.get_token())
+
+    # get the project context
     try:
         proj_context = try_network_action(lambda: ProjectContext(con=con))
     except BaseError:
         logger.error("Unable to retrieve project context from DSP server", exc_info=True)
         raise UserError("Unable to retrieve project context from DSP server") from None
-    sipi_server = Sipi(sipi, con.get_token())
 
     # make Python object representations of the XML file
     resources: list[XMLResource] = []

test/e2e/test_00A1_import_scripts.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-class-docstring,duplicate-code
+# pylint: disable=missing-class-docstring
 
 import os
 import subprocess
@@ -16,7 +16,8 @@
 class TestImportScripts(unittest.TestCase):
     def tearDown(self) -> None:
         """
-        Remove generated data. This method is executed after every test method.
+        Remove generated data.
+        For each test method, a new TestCase instance is created, so tearDown() is executed after each test method.
         """
         if os.path.isfile("src/dsp_tools/import_scripts/data-processed.xml"):
             os.remove("src/dsp_tools/import_scripts/data-processed.xml")

test/e2e/test_cli.py

@@ -4,7 +4,7 @@
 can be called with the basic configuration that is available via CLI. More thorough testing of each method is done in
 separate unit tests/e2e tests."""
 
-# pylint: disable=missing-class-docstring,missing-function-docstring,duplicate-code
+# pylint: disable=missing-class-docstring,missing-function-docstring
 
 import copy
 import json