From 254dfa559bdfc1adddfb3e9d9af9de8b1f202852 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Tue, 3 Oct 2023 11:59:24 +0200 Subject: [PATCH 1/5] Add immediate project removal to fix auto-tests --- mergin/client.py | 56 ++++++++++++++++++++++++++++++++++++-- mergin/test/test_client.py | 8 +++--- mergin/utils.py | 23 ++++++++++++++++ 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/mergin/client.py b/mergin/client.py index ee241a5..372026a 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -28,7 +28,7 @@ ) from .client_pull import pull_project_async, pull_project_wait, pull_project_finalize from .client_push import push_project_async, push_project_wait, push_project_finalize -from .utils import DateTimeEncoder, get_versions_with_file_changes, int_version +from .utils import DateTimeEncoder, get_versions_with_file_changes, int_version, is_version_acceptable from .version import __version__ this_dir = os.path.dirname(os.path.realpath(__file__)) @@ -79,6 +79,7 @@ def __init__(self, url=None, auth_token=None, login=None, password=None, plugin_ self._auth_session = None self._user_info = None self._server_type = None + self._server_version = None self.client_version = "Python-client/" + __version__ if plugin_version is not None: # this could be e.g. "Plugin/2020.1 QGIS/3.14" self.client_version += " " + plugin_version @@ -383,6 +384,23 @@ def server_type(self): return self._server_type + def server_version(self): + """ + Returns version of the server + + :returns: Version string, e.g. "2023.5.0". For old servers (pre-2023) this may be empty string. + :rtype: str + """ + if self._server_version is None: + try: + resp = self.get("/config") + config = json.load(resp) + self._server_version = config["version"] + except (ClientError, KeyError): + self._server_version = "" + + return self._server_version + def workspaces_list(self): """ Find all available workspaces @@ -803,14 +821,48 @@ def clone_project(self, source_project_path, cloned_project_name, cloned_project request = urllib.request.Request(url, data=json.dumps(data).encode(), headers=json_headers, method="POST") self._do_request(request) + def delete_project_now(self, project_path): + """ + Delete project repository on server immediately. + + This should be typically in development, e.g. auto tests, where we + do not want scheduled project deletes as done by delete_project(). + + :param project_path: Project's full name (/) + :type project_path: String + + """ + # TODO: this version check should be replaced by checking against the list + # of endpoints that server publishes in /config (once implemented) + if not is_version_acceptable(self.server_version(), "2023.5"): + raise NotImplementedError("This needs server at version 2023.5 or later") + + project_info = self.project_info(project_path) + project_id = project_info["id"] + path = "/v2/projects/" + project_id + url = urllib.parse.urljoin(self.url, urllib.parse.quote(path)) + request = urllib.request.Request(url, method="DELETE") + self._do_request(request) + def delete_project(self, project_path): """ - Delete project repository on server. + Delete project repository on server. Newer servers since 2023 + will schedule deletion of the project, but not fully remove it immediately. + This is the preferred way when the removal is user initiated, so that + it is not possible to replace the project with another one with the same + name, which would confuse clients that do not use project IDs yet. + + There is also delete_project_now() on newer servers which deletes projects + immediately. :param project_path: Project's full name (/) :type project_path: String """ + # TODO: newer server version (since 2023.5.0) have a new endpoint + # (POST /v2/projects//scheduleDelete) that does the same thing, + # but using project ID instead of the name. At some point we may + # want to migrate to it. path = "/v1/project/%s" % project_path url = urllib.parse.urljoin(self.url, urllib.parse.quote(path)) request = urllib.request.Request(url, method="DELETE") diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index d250b5a..3ca0277 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -63,7 +63,7 @@ def create_workspace_for_client(mc): def cleanup(mc, project, dirs): # cleanup leftovers from previous test if needed such as remote project and local directories try: - mc.delete_project(project) + mc.delete_project_now(project) except ClientError: pass remove_folders(dirs) @@ -105,17 +105,17 @@ def test_create_delete_project(mc): assert any(p for p in projects if p["name"] == test_project and p["namespace"] == API_USER) # try again - with pytest.raises(ClientError, match=f"Project {test_project} already exists"): + with pytest.raises(ClientError, match=f"already exists"): mc.create_project(test_project) # remove project - mc.delete_project(API_USER + "/" + test_project) + mc.delete_project_now(API_USER + "/" + test_project) projects = mc.projects_list(flag="created") assert not any(p for p in projects if p["name"] == test_project and p["namespace"] == API_USER) # try again, nothing to delete with pytest.raises(ClientError): - mc.delete_project(API_USER + "/" + test_project) + mc.delete_project_now(API_USER + "/" + test_project) def test_create_remote_project_from_local(mc): diff --git a/mergin/utils.py b/mergin/utils.py index 0044560..a70f48b 100644 --- a/mergin/utils.py +++ b/mergin/utils.py @@ -232,3 +232,26 @@ def edit_conflict_file_name(path, user, version): ext = "".join(Path(tail).suffixes) file_name = tail.replace(ext, "") return os.path.join(head, file_name) + f" (edit conflict, {user} v{version}).json" + + +def is_version_acceptable(version, min_version): + """ + Checks whether given version is at least min_version or later (both given as strings). + + Versions are expected to be using syntax X.Y.Z + + Returns true if version >= min_version + """ + m = re.search("(\\d+)[.](\\d+)", min_version) + min_major, min_minor = m.group(1), m.group(2) + + if len(version) == 0: + return False # unknown version is assumed to be old + + m = re.search("(\\d+)[.](\\d+)", version) + if m is None: + return False + + major, minor = m.group(1), m.group(2) + + return major > min_major or (major == min_major and minor >= min_minor) From 28328fb14450ba2869e3362038d116c38d7c0d77 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Tue, 3 Oct 2023 13:52:05 +0200 Subject: [PATCH 2/5] skip some tests when sudo is not available --- mergin/test/test_client.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 3ca0277..55e603d 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -76,6 +76,11 @@ def remove_folders(dirs): shutil.rmtree(d) +def sudo_works(): + sudo_res = subprocess.run(["sudo", "echo", "test"]) + return sudo_res.returncode != 0 + + def test_login(mc): token = mc._auth_session["token"] assert MerginClient(mc.url, auth_token=token) @@ -1484,6 +1489,7 @@ def create_directory(root, data): open(os.path.join(root, file_name), "w").close() +@pytest.mark.skipif(sudo_works(), reason="needs working sudo") def test_unfinished_pull(mc): """ Checks whether a pull with failed rebase (due to remote DB schema change) @@ -1527,7 +1533,8 @@ def test_unfinished_pull(mc): # lock base file to emulate situation when we can't overwrite it, because # it is used by another process - subprocess.run(["sudo", "chattr", "+i", test_gpkg]) + sudo_res = subprocess.run(["sudo", "chattr", "+i", test_gpkg]) + assert sudo_res.returncode == 0 mc.pull_project(project_dir) @@ -1549,7 +1556,8 @@ def test_unfinished_pull(mc): assert _get_table_row_count(test_gpkg_unfinished_pull, "simple") == 3 # unlock base file, so we can apply changes from the unfinished pull - subprocess.run(["sudo", "chattr", "-i", test_gpkg]) + sudo_res = subprocess.run(["sudo", "chattr", "-i", test_gpkg]) + assert sudo_res.returncode == 0 mc.resolve_unfinished_pull(project_dir) @@ -1570,6 +1578,7 @@ def test_unfinished_pull(mc): assert _get_table_row_count(test_gpkg_conflict, "simple") == 4 +@pytest.mark.skipif(sudo_works(), reason="needs working sudo") def test_unfinished_pull_push(mc): """ Checks client behaviour when performing push and pull of the project @@ -1612,7 +1621,8 @@ def test_unfinished_pull_push(mc): # lock base file to emulate situation when we can't overwrite it, because # it is used by another process - subprocess.run(["sudo", "chattr", "+i", test_gpkg]) + sudo_res = subprocess.run(["sudo", "chattr", "+i", test_gpkg]) + assert sudo_res.returncode == 0 mc.pull_project(project_dir) @@ -1644,7 +1654,8 @@ def test_unfinished_pull_push(mc): mc.pull_project(project_dir) # unlock base file, so we can apply changes from the unfinished pull - subprocess.run(["sudo", "chattr", "-i", test_gpkg]) + sudo_res = subprocess.run(["sudo", "chattr", "-i", test_gpkg]) + assert sudo_res.returncode == 0 # perform pull. This should resolve unfinished pull first and then # collect data from the server From 7386971ae1634c9265fc96fe016f29e598f409f2 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Tue, 3 Oct 2023 13:52:41 +0200 Subject: [PATCH 3/5] close cursor+connection - maybe helps with test failures on CI? --- mergin/test/test_client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 55e603d..de99e78 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -1039,6 +1039,8 @@ def _use_wal(db_file): con = sqlite3.connect(db_file) cursor = con.cursor() cursor.execute("PRAGMA journal_mode=wal;") + cursor.close() + con.close() def _create_test_table(db_file): From 108ab9d1d3f47e3c9f00417f342f1ffae32365e4 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Tue, 3 Oct 2023 14:42:03 +0200 Subject: [PATCH 4/5] try to fix unfinished pull test failure on CI --- mergin/test/test_client.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index de99e78..ace151b 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -1076,18 +1076,18 @@ def _delete_spatial_table(db_file): def _check_test_table(db_file): """Checks whether the 'test' table exists and has one row - otherwise fails with an exception.""" - # con_verify = sqlite3.connect(db_file) - # cursor_verify = con_verify.cursor() - # cursor_verify.execute('select count(*) from test;') - # assert cursor_verify.fetchone()[0] == 1 assert _get_table_row_count(db_file, "test") == 1 def _get_table_row_count(db_file, table): - con_verify = sqlite3.connect(db_file) - cursor_verify = con_verify.cursor() - cursor_verify.execute("select count(*) from {};".format(table)) - return cursor_verify.fetchone()[0] + try: + con_verify = sqlite3.connect(db_file) + cursor_verify = con_verify.cursor() + cursor_verify.execute("select count(*) from {};".format(table)) + return cursor_verify.fetchone()[0] + finally: + cursor_verify.close() + con_verify.close() def _is_file_updated(filename, changes_dict): From 0f5264e06b3639e76f6d96a5b83431567943a656 Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Tue, 3 Oct 2023 14:51:11 +0200 Subject: [PATCH 5/5] fix code style --- mergin/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mergin/utils.py b/mergin/utils.py index a70f48b..8433f65 100644 --- a/mergin/utils.py +++ b/mergin/utils.py @@ -246,7 +246,7 @@ def is_version_acceptable(version, min_version): min_major, min_minor = m.group(1), m.group(2) if len(version) == 0: - return False # unknown version is assumed to be old + return False # unknown version is assumed to be old m = re.search("(\\d+)[.](\\d+)", version) if m is None: