diff --git a/mergin/client.py b/mergin/client.py index 50c9b6f..d949a46 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -763,7 +763,7 @@ def set_project_access(self, project_path, access): :param project_path: project full name (/) :param access: dict -> list of str username we want to give access to (editorsnames are only supported on servers at version 2024.4.0 or later) """ - if "editorsnames" in access and not is_version_acceptable(self.server_version(), "2024.4"): + if "editorsnames" in access and not self.has_editor_support(): raise NotImplementedError("Editors are only supported on servers at version 2024.4.0 or later") if not self._user_info: @@ -790,7 +790,7 @@ def add_user_permissions_to_project(self, project_path, usernames, permission_le Editor permission_level is only supported on servers at version 2024.4.0 or later. """ if permission_level not in ["owner", "reader", "writer", "editor"] or ( - permission_level == "editor" and not is_version_acceptable(self.server_version(), "2024.4") + permission_level == "editor" and not self.has_editor_support() ): raise ClientError("Unsupported permission level") @@ -1235,3 +1235,9 @@ def download_files( job = download_files_async(self, project_dir, file_paths, output_paths, version=version) pull_project_wait(job) download_files_finalize(job) + + def has_editor_support(self): + """ + Returns whether the server version is acceptable for editor support. + """ + return is_version_acceptable(self.server_version(), "2024.4.0") diff --git a/mergin/client_pull.py b/mergin/client_pull.py index 05a2dd6..db4fa94 100644 --- a/mergin/client_pull.py +++ b/mergin/client_pull.py @@ -22,7 +22,7 @@ from .common import CHUNK_SIZE, ClientError from .merginproject import MerginProject -from .utils import save_to_file, get_versions_with_file_changes +from .utils import save_to_file # status = download_project_async(...) @@ -340,7 +340,7 @@ def __init__( mp, project_info, basefiles_to_patch, - user_name, + mc, ): self.project_path = project_path self.pull_changes = ( @@ -358,7 +358,7 @@ def __init__( self.basefiles_to_patch = ( basefiles_to_patch # list of tuples (relative path within project, list of diff files in temp dir to apply) ) - self.user_name = user_name + self.mc = mc def dump(self): print("--- JOB ---", self.total_size, "bytes") @@ -494,7 +494,7 @@ def pull_project_async(mc, directory): mp, server_info, basefiles_to_patch, - mc.username(), + mc, ) # start download @@ -570,7 +570,7 @@ def merge(self): raise ClientError("Download of file {} failed. Please try it again.".format(self.dest_file)) -def pull_project_finalize(job): +def pull_project_finalize(job: PullJob): """ To be called when pull in the background is finished and we need to do the finalization (merge chunks etc.) @@ -623,7 +623,7 @@ def pull_project_finalize(job): raise ClientError("Cannot patch basefile {}! Please try syncing again.".format(basefile)) try: - conflicts = job.mp.apply_pull_changes(job.pull_changes, job.temp_dir, job.user_name) + conflicts = job.mp.apply_pull_changes(job.pull_changes, job.temp_dir, job.project_info, job.mc) except Exception as e: job.mp.log.error("Failed to apply pull changes: " + str(e)) job.mp.log.info("--- pull aborted") @@ -721,7 +721,7 @@ def download_diffs_async(mc, project_directory, file_path, versions): mp, server_info, {}, - mc.username(), + mc, ) # start download diff --git a/mergin/client_push.py b/mergin/client_push.py index de48fc1..58b17fb 100644 --- a/mergin/client_push.py +++ b/mergin/client_push.py @@ -18,6 +18,7 @@ from .common import UPLOAD_CHUNK_SIZE, ClientError from .merginproject import MerginProject +from .editor import filter_changes class UploadJob: @@ -66,7 +67,11 @@ def upload_blocking(self, mc, mp): mp.log.debug(f"Uploading {self.file_path} part={self.chunk_index}") headers = {"Content-Type": "application/octet-stream"} - resp = mc.post("/v1/project/push/chunk/{}/{}".format(self.transaction_id, self.chunk_id), data, headers) + resp = mc.post( + "/v1/project/push/chunk/{}/{}".format(self.transaction_id, self.chunk_id), + data, + headers, + ) resp_dict = json.load(resp) mp.log.debug(f"Upload finished: {self.file_path}") if not (resp_dict["size"] == len(data) and resp_dict["checksum"] == checksum.hexdigest()): @@ -91,12 +96,12 @@ def push_project_async(mc, directory): mp.log.info(f"--- start push {project_path}") try: - server_info = mc.project_info(project_path) + project_info = mc.project_info(project_path) except ClientError as err: mp.log.error("Error getting project info: " + str(err)) mp.log.info("--- push aborted") raise - server_version = server_info["version"] if server_info["version"] else "v0" + server_version = project_info["version"] if project_info["version"] else "v0" mp.log.info(f"got project info: local version {local_version} / server version {server_version}") @@ -116,6 +121,7 @@ def push_project_async(mc, directory): ) changes = mp.get_push_changes() + changes = filter_changes(mc, project_info, changes) mp.log.debug("push changes:\n" + pprint.pformat(changes)) tmp_dir = tempfile.TemporaryDirectory(prefix="mergin-py-client-") @@ -143,7 +149,11 @@ def push_project_async(mc, directory): data = {"version": local_version, "changes": changes} try: - resp = mc.post(f"/v1/project/push/{project_path}", data, {"Content-Type": "application/json"}) + resp = mc.post( + f"/v1/project/push/{project_path}", + data, + {"Content-Type": "application/json"}, + ) except ClientError as err: mp.log.error("Error starting transaction: " + str(err)) mp.log.info("--- push aborted") diff --git a/mergin/editor.py b/mergin/editor.py new file mode 100644 index 0000000..4195b3f --- /dev/null +++ b/mergin/editor.py @@ -0,0 +1,109 @@ +from itertools import filterfalse +from typing import Callable + +from .utils import is_mergin_config, is_qgis_file, is_versioned_file + +EDITOR_ROLE_NAME = "editor" + +""" +Determines whether a given file change should be disallowed based on the file path. + +Returns: + bool: True if the file change should be disallowed, False otherwise. +""" +disallowed_added_changes: Callable[[dict], bool] = lambda change: is_qgis_file(change["path"]) or is_mergin_config( + change["path"] +) +""" +Determines whether a given file change should be disallowed from being updated. + +The function checks the following conditions: +- If the file path matches a QGIS file +- If the file path matches a Mergin configuration file +- If the file is versioned and the change does not have a diff + +Returns: + bool: True if the change should be disallowed, False otherwise. +""" +_disallowed_updated_changes: Callable[[dict], bool] = ( + lambda change: is_qgis_file(change["path"]) + or is_mergin_config(change["path"]) + or (is_versioned_file(change["path"]) and change.get("diff") is None) +) +""" +Determines whether a given file change should be disallowed from being removed. + +The function checks if the file path of the change matches any of the following conditions: +- The file is a QGIS file (e.g. .qgs, .qgz) +- The file is a Mergin configuration file (mergin-config.json) +- The file is a versioned file (.gpkg, .sqlite) + +If any of these conditions are met, the change is considered disallowed from being removed. +""" +_disallowed_removed_changes: Callable[[dict], bool] = ( + lambda change: is_qgis_file(change["path"]) or is_mergin_config(change["path"]) or is_versioned_file(change["path"]) +) + + +def is_editor_enabled(mc, project_info: dict) -> bool: + """ + The function checks if the server supports editor access, and if the current user's project role matches the expected role name for editors. + """ + server_support = mc.has_editor_support() + project_role = project_info.get("role") + + return server_support and project_role == EDITOR_ROLE_NAME + + +def _apply_editor_filters(changes: dict[str, list[dict]]) -> dict[str, list[dict]]: + """ + Applies editor-specific filters to the changes dictionary, removing any changes to files that are not in the editor's list of allowed files. + + Args: + changes (dict[str, list[dict]]): A dictionary containing the added, updated, and removed changes. + + Returns: + dict[str, list[dict]]: The filtered changes dictionary. + """ + added = changes.get("added", []) + updated = changes.get("updated", []) + removed = changes.get("removed", []) + + # filter out files that are not in the editor's list of allowed files + changes["added"] = list(filterfalse(disallowed_added_changes, added)) + changes["updated"] = list(filterfalse(_disallowed_updated_changes, updated)) + changes["removed"] = list(filterfalse(_disallowed_removed_changes, removed)) + return changes + + +def filter_changes(mc, project_info: dict, changes: dict[str, list[dict]]) -> dict[str, list[dict]]: + """ + Filters the given changes dictionary based on the editor's enabled state. + + If the editor is not enabled, the changes dictionary is returned as-is. Otherwise, the changes are passed through the `_apply_editor_filters` method to apply any configured filters. + + Args: + changes (dict[str, list[dict]]): A dictionary mapping file paths to lists of change dictionaries. + + Returns: + dict[str, list[dict]]: The filtered changes dictionary. + """ + if not is_editor_enabled(mc, project_info): + return changes + return _apply_editor_filters(changes) + + +def prevent_conflicted_copy(path: str, mc, project_info: dict) -> bool: + """ + Decides whether a file path should be blocked from creating a conflicted copy. + Note: This is used when the editor is active and attempting to modify files (e.g., .ggs) that are also updated on the server, preventing unnecessary conflict files creation. + + Args: + path (str): The file path to check. + mc: The Mergin client object. + project_info (dict): Information about the Mergin project from server. + + Returns: + bool: True if the file path should be prevented from ceating conflicted copy, False otherwise. + """ + return is_editor_enabled(mc, project_info) and any([is_qgis_file(path), is_mergin_config(path)]) diff --git a/mergin/merginproject.py b/mergin/merginproject.py index f82fbe5..0c44e66 100644 --- a/mergin/merginproject.py +++ b/mergin/merginproject.py @@ -9,12 +9,13 @@ from datetime import datetime from dateutil.tz import tzlocal +from .editor import prevent_conflicted_copy + from .common import UPLOAD_CHUNK_SIZE, InvalidProject, ClientError from .utils import ( generate_checksum, - move_file, + is_versioned_file, int_version, - find, do_sqlite_checkpoint, unique_path_name, conflicted_copy_file_name, @@ -138,8 +139,7 @@ def fpath_cache(self, file, version=None): def project_full_name(self) -> str: """Returns fully qualified project name: /""" - if self._metadata is None: - self._read_metadata() + self._read_metadata() if self.is_old_metadata: return self._metadata["name"] else: @@ -164,8 +164,7 @@ def project_id(self) -> str: only happen with projects downloaded with old client, before February 2023, see https://github.com/MerginMaps/mergin-py-client/pull/154 """ - if self._metadata is None: - self._read_metadata() + self._read_metadata() # "id" or "project_id" may not exist in projects downloaded with old client version if self.is_old_metadata: @@ -182,8 +181,7 @@ def workspace_id(self) -> int: """Returns ID of the workspace where the project belongs""" # unfortunately we currently do not have information about workspace ID # in project's metadata... - if self._metadata is None: - self._read_metadata() + self._read_metadata() # "workspace_id" does not exist in projects downloaded with old client version if self.is_old_metadata: @@ -195,14 +193,12 @@ def workspace_id(self) -> int: def version(self) -> str: """Returns project version (e.g. "v123")""" - if self._metadata is None: - self._read_metadata() + self._read_metadata() return self._metadata["version"] def files(self) -> list: """Returns project's list of files (each file being a dictionary)""" - if self._metadata is None: - self._read_metadata() + self._read_metadata() return self._metadata["files"] @property @@ -213,12 +209,13 @@ def metadata(self) -> dict: from warnings import warn warn("MerginProject.metadata getter should not be used anymore", DeprecationWarning) - if self._metadata is None: - self._read_metadata() + self._read_metadata() return self._metadata - def _read_metadata(self): + def _read_metadata(self) -> None: """Loads the project's metadata from JSON""" + if self._metadata is not None: + return if not os.path.exists(self.fpath_meta("mergin.json")): raise InvalidProject("Project metadata has not been created yet") with open(self.fpath_meta("mergin.json"), "r") as file: @@ -254,9 +251,7 @@ def is_versioned_file(self, file): :returns: if file is compatible with geodiff lib :rtype: bool """ - diff_extensions = [".gpkg", ".sqlite"] - f_extension = os.path.splitext(file)[1] - return f_extension in diff_extensions + return is_versioned_file(file) def is_gpkg_open(self, path): """ @@ -504,7 +499,7 @@ def get_list_of_push_changes(self, push_changes): pass return changes - def apply_pull_changes(self, changes, temp_dir, user_name): + def apply_pull_changes(self, changes, temp_dir, server_project, mc): """ Apply changes pulled from server. @@ -517,14 +512,18 @@ def apply_pull_changes(self, changes, temp_dir, user_name): :type changes: dict[str, list[dict]] :param temp_dir: directory with downloaded files from server :type temp_dir: str - :returns: files where conflicts were found + :param user_name: name of the user that is pulling the changes + :type user_name: str + :param server_project: project metadata from the server + :type server_project: dict + :param mc: mergin client + :type mc: mergin.client.MerginClient + :returns: list of files with conflicts :rtype: list[str] """ conflicts = [] local_changes = self.get_push_changes() - modified = {} - for f in local_changes["added"] + local_changes["updated"]: - modified.update({f["path"]: f}) + modified_local_paths = [f["path"] for f in local_changes.get("added", []) + local_changes.get("updated", [])] local_files_map = {} for f in self.inspect_files(): @@ -540,8 +539,8 @@ def apply_pull_changes(self, changes, temp_dir, user_name): # special care is needed for geodiff files # 'src' here is server version of file and 'dest' is locally modified if self.is_versioned_file(path) and k == "updated": - if path in modified: - conflict = self.update_with_rebase(path, src, dest, basefile, temp_dir, user_name) + if path in modified_local_paths: + conflict = self.update_with_rebase(path, src, dest, basefile, temp_dir, mc.username()) if conflict: conflicts.append(conflict) else: @@ -549,9 +548,13 @@ def apply_pull_changes(self, changes, temp_dir, user_name): # We just apply the diff between our copy and server to both the local copy and its basefile self.update_without_rebase(path, src, dest, basefile, temp_dir) else: - # backup if needed - if path in modified and item["checksum"] != local_files_map[path]["checksum"]: - conflict = self.create_conflicted_copy(path, user_name) + # creating conflicted copy if both server and local changes are present on the files + if ( + path in modified_local_paths + and item["checksum"] != local_files_map[path]["checksum"] + and not prevent_conflicted_copy(path, mc, server_project) + ): + conflict = self.create_conflicted_copy(path, mc.username()) conflicts.append(conflict) if k == "removed": diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index fb70216..00d22ef 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -9,7 +9,6 @@ import pytz import sqlite3 import glob -import time from .. import InvalidProject from ..client import ( @@ -39,6 +38,7 @@ ) from ..merginproject import pygeodiff from ..report import create_report +from ..editor import EDITOR_ROLE_NAME, filter_changes, is_editor_enabled SERVER_URL = os.environ.get("TEST_MERGIN_URL") @@ -113,7 +113,12 @@ def server_has_editor_support(mc, access): Returns: bool: True if the server has editor support, False otherwise. """ - return "editorsnames" in access and is_version_acceptable(mc.server_version(), "2024.4") + return "editorsnames" in access and mc.has_editor_support() + + +def test_client_instance(mc, mc2): + assert isinstance(mc, MerginClient) + assert isinstance(mc2, MerginClient) def test_login(mc): @@ -1191,24 +1196,18 @@ def test_download_diffs(mc): def test_modify_project_permissions(mc): test_project = "test_project" - project = API_USER + "/" + test_project + test_project_fullname = API_USER + "/" + test_project project_dir = os.path.join(TMP_DIR, test_project) download_dir = os.path.join(TMP_DIR, "download", test_project) - cleanup(mc, project, [project_dir, download_dir]) + cleanup(mc, test_project_fullname, [project_dir, download_dir]) # prepare local project shutil.copytree(TEST_DATA_DIR, project_dir) # create remote project - mc.create_project_and_push(project, directory=project_dir) - - # check basic metadata about created project - project_info = mc.project_info(project) - assert project_info["version"] == "v1" - assert project_info["name"] == test_project - assert project_info["namespace"] == API_USER + mc.create_project_and_push(test_project_fullname, directory=project_dir) - permissions = mc.project_user_permissions(project) + permissions = mc.project_user_permissions(test_project_fullname) assert permissions["owners"] == [API_USER] assert permissions["writers"] == [API_USER] assert permissions["readers"] == [API_USER] @@ -1216,8 +1215,8 @@ def test_modify_project_permissions(mc): if editor_support: assert permissions["editors"] == [API_USER] - mc.add_user_permissions_to_project(project, [API_USER2], "writer") - permissions = mc.project_user_permissions(project) + mc.add_user_permissions_to_project(test_project_fullname, [API_USER2], "writer") + permissions = mc.project_user_permissions(test_project_fullname) assert set(permissions["owners"]) == {API_USER} assert set(permissions["writers"]) == {API_USER, API_USER2} assert set(permissions["readers"]) == {API_USER, API_USER2} @@ -1225,8 +1224,8 @@ def test_modify_project_permissions(mc): if editor_support: assert set(permissions["editors"]) == {API_USER, API_USER2} - mc.remove_user_permissions_from_project(project, [API_USER2]) - permissions = mc.project_user_permissions(project) + mc.remove_user_permissions_from_project(test_project_fullname, [API_USER2]) + permissions = mc.project_user_permissions(test_project_fullname) assert permissions["owners"] == [API_USER] assert permissions["writers"] == [API_USER] assert permissions["readers"] == [API_USER] @@ -2524,3 +2523,132 @@ def test_download_failure(mc): with open(job.failure_log_file, "r", encoding="utf-8") as f: content = f.read() assert "Traceback" in content + + +# TODO: consider to use separate test_editor.py file for tests that require editor +def test_editor(mc: MerginClient): + """Test editor handler class and push with editor""" + + project_info = {"role": "editor"} + if not mc.has_editor_support(): + assert is_editor_enabled(mc, project_info) is False + return + + # mock that user is editor + project_info["role"] = EDITOR_ROLE_NAME + assert is_editor_enabled(mc, project_info) is True + + # unit test for editor methods + qgs_changeset = { + "added": [{"path": "/folder/project.new.Qgz"}], + "updated": [{"path": "/folder/project.updated.Qgs"}], + "removed": [{"path": "/folder/project.removed.qgs"}], + } + qgs_changeset = filter_changes(mc, project_info, qgs_changeset) + assert sum(len(v) for v in qgs_changeset.values()) == 0 + + mergin_config_changeset = { + "added": [{"path": "/.mergin/mergin-config.json"}], + "updated": [{"path": "/.mergin/mergin-config.json"}], + "removed": [{"path": "/.mergin/mergin-config.json"}], + } + mergin_config_changeset = filter_changes(mc, project_info, mergin_config_changeset) + assert sum(len(v) for v in mergin_config_changeset.values()) == 0 + + gpkg_changeset = { + "added": [{"path": "/.mergin/data.gpkg"}], + "updated": [ + {"path": "/.mergin/conflict-data.gpkg"}, + {"path": "/.mergin/data.gpkg", "diff": {}}, + ], + "removed": [{"path": "/.mergin/data.gpkg"}], + } + gpkg_changeset = filter_changes(mc, project_info, gpkg_changeset) + assert sum(len(v) for v in gpkg_changeset.values()) == 2 + assert gpkg_changeset["added"][0]["path"] == "/.mergin/data.gpkg" + assert gpkg_changeset["updated"][0]["path"] == "/.mergin/data.gpkg" + + +def test_editor_push(mc: MerginClient, mc2: MerginClient): + """Test push with editor""" + if not mc.has_editor_support(): + return + test_project = "test_editor_push" + test_project_fullname = API_USER + "/" + test_project + project = API_USER + "/" + test_project + project_dir = os.path.join(TMP_DIR, test_project) + project_dir2 = os.path.join(TMP_DIR, test_project + "_2") + cleanup(mc, project, [project_dir, project_dir2]) + + # create new (empty) project on server + # TODO: return project_info from create project, don't use project_full name for project info, instead returned id of project + mc.create_project(test_project) + mc.add_user_permissions_to_project(project, [API_USER2], "editor") + # download empty project + mc2.download_project(test_project_fullname, project_dir) + + # editor is starting to adding qgis files and "normal" file + qgs_file_name = "test.qgs" + txt_file_name = "test.txt" + gpkg_file_name = "base.gpkg" + files_to_push = [qgs_file_name, txt_file_name, gpkg_file_name] + for file in files_to_push: + shutil.copy(os.path.join(TEST_DATA_DIR, file), project_dir) + # it's possible to push allowed files if editor + mc2.push_project(project_dir) + project_info = mc2.project_info(test_project_fullname) + assert len(project_info.get("files")) == len(files_to_push) - 1 # ggs is not pushed + # find pushed files in server + assert any(file["path"] == qgs_file_name for file in project_info.get("files")) is False + assert any(file["path"] == txt_file_name for file in project_info.get("files")) is True + assert any(file["path"] == gpkg_file_name for file in project_info.get("files")) is True + pull_changes, push_changes, push_changes_summary = mc.project_status(project_dir) + assert not sum(len(v) for v in pull_changes.values()) + assert sum(len(v) for v in push_changes.values()) == 1 + # ggs is still waiting to push + assert any(file["path"] == qgs_file_name for file in push_changes.get("added")) is True + + # editor is trying to psuh row to gpkg file -> it's possible + shutil.copy( + os.path.join(TEST_DATA_DIR, "inserted_1_A.gpkg"), + os.path.join(project_dir, gpkg_file_name), + ) + mc2.push_project(project_dir) + project_info = mc2.project_info(test_project_fullname) + pull_changes, push_changes, push_changes_summary = mc.project_status(project_dir) + assert any(file["path"] == gpkg_file_name for file in project_info.get("files")) is True + assert any(file["path"] == gpkg_file_name for file in push_changes.get("updated")) is False + + # editor is trying to insert tables to gpkg file + shutil.copy( + os.path.join(TEST_DATA_DIR, "two_tables.gpkg"), + os.path.join(project_dir, gpkg_file_name), + ) + mc2.push_project(project_dir) + pull_changes, push_changes, push_changes_summary = mc.project_status(project_dir) + assert not sum(len(v) for v in pull_changes.values()) + # gpkg was filter by editor_handler in push_project, because new tables added + assert sum(len(v) for v in push_changes.values()) == 2 + # ggs and gpkg are still waiting to push + assert any(file["path"] == qgs_file_name for file in push_changes.get("added")) is True + assert any(file["path"] == gpkg_file_name for file in push_changes.get("updated")) is True + # push as owner do cleanup local changes and preparation to conflicited copy simulate + mc.push_project(project_dir) + + # simulate conflicting copy of qgis file + # Push from dir2 changes to qgis file + mc.download_project(test_project_fullname, project_dir2) + with open(os.path.join(project_dir2, qgs_file_name), "a") as f: + f.write("C server version") + mc.push_project(project_dir2) + # editor is editing qgs file too in dir + with open(os.path.join(project_dir, qgs_file_name), "a") as f: + f.write("B local version") + mc2.pull_project(project_dir) + conflicted_file = None + for project_file in os.listdir(project_dir): + _, ext = os.path.splitext(project_file) + if ext == ".qgs~": + conflicted_file = project_file + # There is no conflicted qgs file + assert conflicted_file is None diff --git a/mergin/utils.py b/mergin/utils.py index 8433f65..0848a29 100644 --- a/mergin/utils.py +++ b/mergin/utils.py @@ -205,7 +205,7 @@ def conflicted_copy_file_name(path, user, version): # to avoid having several QGIS project files inside Mergin Maps project. # See https://github.com/lutraconsulting/qgis-mergin-plugin/issues/382 # for more details - if ext.lower() in (".qgz", ".qgs"): + if is_qgis_file(path): ext += "~" return os.path.join(head, file_name) + f" (conflicted copy, {user} v{version}){ext}" @@ -255,3 +255,23 @@ def is_version_acceptable(version, min_version): major, minor = m.group(1), m.group(2) return major > min_major or (major == min_major and minor >= min_minor) + + +def is_versioned_file(path: str) -> bool: + diff_extensions = [".gpkg", ".sqlite"] + f_extension = os.path.splitext(path)[1] + return f_extension.lower() in diff_extensions + + +def is_qgis_file(path: str) -> bool: + """ + Check if file is a QGIS project file. + """ + f_extension = os.path.splitext(path)[1] + return f_extension.lower() in [".qgs", ".qgz"] + + +def is_mergin_config(path: str) -> bool: + """Check if the given path is for file mergin-config.json""" + filename = os.path.basename(path).lower() + return filename == "mergin-config.json"