fix: catch network interruptions during onto creation (DEV-1073) #210

jnussbaum · 2022-08-05T17:04:02Z

- improve console output during the onto creation - thorough refactoring of the architecture - it is now more clear what the methods do - no more doubling, e.g. the onto isn't validated two times any more

jnussbaum · 2022-08-05T17:46:41Z

docs/dsp-tools-usage.md

+- `-V` | `--validate-only`: If set, only the validation of the JSON file is performed.
+- `-l` | `--lists-only`: If set, only the lists are created. Please note that in this case the project must already exist.


small change to make it more clear

jnussbaum · 2022-08-05T17:47:01Z

docs/dsp-tools-usage.md

+- `-V` | `--validate-only`: If set, only the validation of the JSON file is performed.
+- `-l` | `--lists-only`: If set, only the lists are created. Please note that in this case the project must already exist.
+- `-v` | `--verbose`: If set, more information about the progress is printed to the console.
+- `-d` | `--dump`: If set, dump test files for DSP-API requests.


This option already existed in the code, but was not documented

jnussbaum · 2022-08-05T17:51:07Z

knora/dsp_tools.py

+                if validate_project(args.datamodelfile):
+                    print('Data model is syntactically correct and passed validation.')
                    exit(0)
-                else:
-                    exit(1)


The behaviour of validate_project() changed: It returns True or raises an Error with detailed explanation. So, it's not necessary any more to exit().

jnussbaum · 2022-08-05T17:52:38Z

knora/dsplib/models/group.py

-        except BaseError:
-            # return None if no groups are found or an error happened
-            return None


I want the error to escalate, so that I can handle it in the higher-level methods

jnussbaum · 2022-08-05T17:57:04Z

knora/dsplib/utils/excel_to_json_lists.py


+def expand_lists_from_excel(


I moved this method here, because it thematically belongs here. Before, it was in the file expand_all_lists.py, which contained nothing else than just this method. I think it makes more sense to move it here and delete the other file.

jnussbaum · 2022-08-05T18:01:11Z

knora/dsplib/utils/excel_to_json_lists.py

    excelfiles: dict[str, Worksheet],
    base_file: dict[str, Worksheet],
    parentnode: dict[str, Any],
    row: int,
    col: int,
-    preval: list[str]
+    preval: list[str],
+    verbose: bool = False


This method is used

by the create command, where it shouldn't print too much by default

during the excel command, where it should print every created node by default

The verbose switch seemed to be a good solution for this.

jnussbaum · 2022-08-08T07:00:08Z

knora/dsplib/utils/excel_to_json_lists.py

-    print('Found the following files:')
-    for file in excel_files:
-        print(file)


This will be printed in the higher-level methods, because depending on the caller, a different print is needed.

jnussbaum · 2022-08-08T07:35:59Z

knora/dsplib/utils/onto_create_ontology.py



-def login(server: str, user: str, password: str) -> Connection:


I moved the method "login()" to "shared_methods.py"

jnussbaum · 2022-08-08T07:40:21Z

knora/dsplib/utils/onto_create_ontology.py

-        # check if status is defined, set default value if not
-        group_status: Optional[str] = group.get("status")
-        group_status_bool = True
-        if isinstance(group_status, str):
-            group_status_bool = json.loads(group_status.lower())  # lower() converts string to boolean
-
-        # check if selfjoin is defined, set default value if not
-        group_selfjoin: Optional[str] = group.get("selfjoin")
-        group_selfjoin_bool = False
-        if isinstance(group_selfjoin, str):
-            group_selfjoin_bool = json.loads(group_selfjoin.lower())  # lower() converts string to boolean
-


I simplified these two radically: They are now directly in the constructor call below, as one-liners

jnussbaum · 2022-08-08T07:51:00Z

knora/dsplib/utils/onto_create_ontology.py


-        sysadmin = False


From here on, GitHub's diff renderling is a bit hard to understand. In PyCharm, it was more understandable. Basically, there are 4 things done inside the big loop for user in users:

skip the user if he already exists

if "groups" is provided, add user to the group(s)

if "projects" is provided, add user to the project(s)

create the user

I tried to simplify these four steps, to minimize the indentation depth, and to improve code understandability.

The diff might be hard to understand, but the code should be better understandable than before.

jnussbaum · 2022-08-08T07:55:22Z

knora/dsplib/utils/onto_create_ontology.py

+        project_remote: Project = try_network_action(
+            object=project_local,
+            method="read",
+            error_message_on_failure=""
+        )


If the project can't be read, it raises a BaseError without message --> create the project.
If the reading suceeds without error --> update project

jnussbaum · 2022-08-08T08:35:48Z

knora/dsplib/utils/shared_methods.py

+            if re.search(r'try again later', err.message) or re.search(r'status code=5\d\d', err.message):
+                print(f'{datetime.now().isoformat()}: Try reconnecting to DSP server, next attempt in {2 ** i} seconds...')
+                time.sleep(2 ** i)
+                continue


These lines are new: If the API's response says that it is a temporary error, it is worth retrying. All other base errors are permanent and will escalate.

jnussbaum · 2022-08-08T08:47:51Z

knora/dsplib/utils/xml_upload.py

@@ -5,14 +5,12 @@
 import json


Why did I change this file?

I moved _try_network_action() to shared_methods.py

I changed the behaviour of _try_network_action(): If the connectivity issues remain, it won't return None anymore, but it will escalate the error that it received from the method that failed. For this reasons, I have to embed all usages of _try_network_action() into a try-except block

jnussbaum · 2022-08-08T08:48:52Z

test/e2e/test_tools.py

@@ -21,8 +20,8 @@ class TestTools(unittest.TestCase):
    password = 'test'
    imgdir = '.'
    sipi = 'http://0.0.0.0:1024'
-    test_onto_file = 'testdata/test-onto.json'
-    test_list_file = 'testdata/test-list.json'


I got rid of the lists.json output file

jnussbaum · 2022-08-08T08:50:30Z

test/e2e/test_tools.py

@@ -68,23 +67,23 @@ def test_get(self) -> None:
        group_descriptions_received = []
        group_selfjoin_received = []
        group_status_received = []
-        for group in groups_expected:
+        for group in sorted(groups_expected, key=lambda x: x["name"]):


I added some groups with more diverse characteristics. It is now necessary to sort the groups by name

jnussbaum · 2022-08-08T08:52:10Z

test/e2e/test_tools.py

+    test_project_file = 'testdata/test-project-systematic.json'
+    test_project_minimal_file = 'testdata/test-project-minimal.json'


It is not only important to have a systematic test project, but also to have a minimal one. During my work, I accidentally spotted a bug which was never detected. With the new minimal test project, it would have been detected.

BalduinLandolt

Not an easy one to review... but I think you improved a lot. I hope I didn't miss anything important.

knora/dsplib/utils/excel_to_json_lists.py

knora/dsplib/utils/onto_create_lists.py

knora/dsplib/utils/onto_create_ontology.py

knora/dsplib/utils/shared_methods.py

sonarcloud · 2022-08-09T08:45:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
25 Code Smells

No Coverage information
0.0% Duplication

- catch network interruptions during the onto creation process

75c095f

- improve console output during the onto creation - thorough refactoring of the architecture - it is now more clear what the methods do - no more doubling, e.g. the onto isn't validated two times any more

jnussbaum self-assigned this Aug 5, 2022

fix

1c06a74

jnussbaum commented Aug 5, 2022

View reviewed changes

handle errors during list expansion

22ad5e7

jnussbaum commented Aug 8, 2022

View reviewed changes

jnussbaum requested a review from BalduinLandolt August 8, 2022 08:53

improve docstring

cb51399

BalduinLandolt approved these changes Aug 8, 2022

View reviewed changes

apply reviewer's feedback

e0826cf

jnussbaum merged commit ab0e3b2 into main Aug 9, 2022
6 checks passed

jnussbaum deleted the wip/dev-1073-catch-network-interruption-while-creating-ontology branch August 9, 2022 09:03

fix: catch network interruptions during onto creation (DEV-1073) #210

fix: catch network interruptions during onto creation (DEV-1073) #210

jnussbaum commented Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 5, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

jnussbaum Aug 8, 2022

BalduinLandolt left a comment

sonarcloud bot commented Aug 9, 2022 •

edited

		- `-V` \| `--validate-only`: If set, only the validation of the JSON file is performed.
		- `-l` \| `--lists-only`: If set, only the lists are created. Please note that in this case the project must already exist.



		def login(server: str, user: str, password: str) -> Connection:

		test_project_file = 'testdata/test-project-systematic.json'
		test_project_minimal_file = 'testdata/test-project-minimal.json'

fix: catch network interruptions during onto creation (DEV-1073) #210

fix: catch network interruptions during onto creation (DEV-1073) #210

Conversation

jnussbaum commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalduinLandolt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 9, 2022 • edited

sonarcloud bot commented Aug 9, 2022 •

edited