-
Notifications
You must be signed in to change notification settings - Fork 11
Initial version of v2 pull integration + project info v2 #282
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
base: v2-pull-integration
Are you sure you want to change the base?
Conversation
+ tests for both v2 and v1 features + projectinfo v2 compatibility test
wonder-sk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's first do a bit of refactoring... the idea is that:
- we introduce a new class that captures project delta (added/updated/updated_diff/deleted) files
- the project delta class would be returned by get_pull_changes and get_push_changes
- there would a function that can combine server changes (from get_pull_changes) and local changes (from get_push_changes) and return a list of tasks to do at the end of the pull (e.g. copy, copy conflict, apply diff, delete - like PullTask in mobile app's merginapi)
- apply_pull_changes() would just go through the list of "pull tasks" and apply them, rather than again getting local changes and doing logic based on that
- models.py for some classes without specific location - clenup of non-necssary functions from previous version - refactor of pull handling using PullActions
+ descriptions + tests for appl_pull_actions + restore get_pull_changes for backward compatibility with project status
| self.size_check = size_check # whether we want to do merged file size check | ||
|
|
||
| def merge(self): | ||
| with open(self.dest_file, "wb") as final: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to create intermediate dirs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was done before. I updated this to create also directory.
| resp = mc.get( | ||
| f"/v2/projects/{mp.project_id()}/raw/diff/{self.file_path}", | ||
| ) | ||
| if resp.status in [200, 206]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always download diff in single request? If so, let's state it explicitly somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comments ⬇️
| (DeltaChangeType.UPDATE_DIFF, DeltaChangeType.UPDATE): PullActionType.COPY_CONFLICT, | ||
| (DeltaChangeType.UPDATE_DIFF, DeltaChangeType.DELETE): PullActionType.COPY, | ||
| (DeltaChangeType.UPDATE_DIFF, DeltaChangeType.UPDATE_DIFF): PullActionType.APPLY_DIFF, # rebase | ||
| (DeltaChangeType.DELETE, None): PullActionType.DELETE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why pull action type delete here and not also for other use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean here. better to discuss @varmar05
Pull Request Test Coverage Report for Build 21143761159Details
💛 - Coveralls |
| resp = self.get(f"/v2/projects/{project_id}", params) | ||
| resp_json = json.load(resp) | ||
| project_workspace = resp_json.get("workspace") | ||
| return ProjectResponse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to ProjectInfo
| :type since: String | ||
| :param to: Optional version to track history of files to, if not given latest version is used | ||
| :type since: String | ||
| :rtype: Dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Implements the initial v2 pull integration using the /delta endpoint (with a v1 fallback), and adds v2 project info support plus related integration/unit tests.
Changes:
- Add v2 pull flow in
pull_project_asyncbased on server feature flag (v2_pull_enabled) and project delta items. - Introduce typed models for delta items, pull actions, and v2 project info responses.
- Expand test coverage for pull actions/delta calculation and project info v2 compatibility.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
mergin/client_pull.py |
Refactors pull pipeline around ProjectDelta + PullAction, adds v2 diff download path. |
mergin/client.py |
Adds project_info_v2() and get_project_delta() API wrappers and a server-version capability check. |
mergin/merginproject.py |
Adds delta/local-delta computation and applies pull via action-based execution. |
mergin/models.py |
Adds dataclasses for delta items, pull actions, and v2 project info response typing. |
mergin/common.py |
Adds enums for delta change and pull action types. |
mergin/test/test_client.py |
Updates/extends integration tests for v2 pull and v2 project info. |
mergin/test/test_mergin_project.py |
Adds unit tests for pull-action selection, delta calculation, local delta, and applying actions. |
mergin/test/test_client_pull.py |
Adds unit tests for download item creation helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.is_versioned_file(path): | ||
| os.remove(basefile) |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the DELETE action, the basefile is only removed when the live file exists. If the live file was already removed locally but the .mergin basefile still exists, this leaves stale metadata behind. Match the previous behavior by removing the basefile whenever it exists (and optionally log when the live file is already missing).
| if self.is_versioned_file(path): | |
| os.remove(basefile) | |
| # always try to remove basefile for versioned files to avoid stale metadata | |
| if self.is_versioned_file(path) and os.path.exists(basefile): | |
| os.remove(basefile) |
| from typing import List, Dict, Any | ||
| from dataclasses import dataclass, field |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports were added (List, Dict, Any, dataclass, field) but aren’t used in this module. Consider removing them to avoid linting issues.
| from typing import List, Dict, Any | |
| from dataclasses import dataclass, field |
| from .utils import cleanup_tmp_dir, int_version, save_to_file | ||
| from typing import List, Optional, Tuple |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports: int_version and Tuple are imported but not used in this module. Remove them to keep imports minimal and avoid lint failures.
| from .utils import cleanup_tmp_dir, int_version, save_to_file | |
| from typing import List, Optional, Tuple | |
| from .utils import cleanup_tmp_dir, save_to_file | |
| from typing import List, Optional |
| total_size = 0 | ||
| download_list = [] | ||
| for file_to_merge in files_to_merge: | ||
| download_list.extend(file_to_merge.downloaded_items) | ||
| download_queue_items = [] | ||
| # Diff files downloaded without chunks (downloaded as a whole, do need to be merged) | ||
| for diff_file in diff_files: | ||
| download_queue_items.append(diff_file) | ||
| total_size += diff_file.size | ||
| for file_to_merge in download_files: |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total_size is computed using DownloadDiffQueueItem.size, but these items start with size = 0 and only learn their real size after download. This makes job.total_size under-report (progress can exceed 100%, and total_size == transferred_size can be wrong). Populate diff sizes up-front (e.g., parse size from the delta response into ProjectDeltaItemDiff and set it on DownloadDiffQueueItem) or adjust progress reporting to account for unknown sizes.
| resp_parsed = json.load(resp) | ||
| return ProjectDelta( | ||
| to_version=resp_parsed.get("to_version"), | ||
| items=[ | ||
| ProjectDeltaItem( | ||
| path=item["path"], | ||
| size=item.get("size"), | ||
| checksum=item.get("checksum"), | ||
| version=item.get("version"), | ||
| change=item.get("change"), | ||
| diffs=( | ||
| [ | ||
| ProjectDeltaItemDiff( | ||
| id=diff.get("id"), | ||
| ) | ||
| for diff in item.get("diffs", []) | ||
| ] | ||
| ), | ||
| ) |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_project_delta() drops diff metadata (size, version) and may set size/checksum/version to None if the server omits fields. This makes it impossible to precompute download sizes for diff items and risks runtime errors where numeric sizes are expected. Parse size/version from each diff entry (and provide safe defaults like size=0, checksum="") when constructing ProjectDeltaItem/ProjectDeltaItemDiff.
| import pytest | ||
| from mergin.common import DeltaChangeType, CHUNK_SIZE | ||
| from mergin.models import ProjectDeltaItem, ProjectDeltaItemDiff | ||
| from mergin.client_pull import get_download_diff_files, get_download_diff_files, get_download_items |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate import of get_download_diff_files in the same import statement. Remove the duplicate to avoid lint failures and keep imports clean.
| from mergin.client_pull import get_download_diff_files, get_download_diff_files, get_download_items | |
| from mergin.client_pull import get_download_diff_files, get_download_items |
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| mp = MerginProject(tmp_dir) | ||
|
|
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MerginProject is created inside the TemporaryDirectory() context but used after the context exits. Because MerginProject sets up a file logger in that directory, this can break cleanup on Windows (open file handles prevent temp dir removal) and makes the test fragile. Keep the assertions inside the with block (or explicitly close/remove the log handler).
| project_info = mc.project_info_v2(mp.project_id(), mp.version()) | ||
| assert project_info.version == "v3" | ||
| assert project_info.id == mp.project_id() | ||
| f_remote = next((f for f in project_info.files if f.path == f_updated), None) |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable f_remote is not used.
| f_remote = next((f for f in project_info.files if f.path == f_updated), None) |
| import os | ||
| import re | ||
| import shutil | ||
| from typing import List, Dict, Any, Optional |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Dict' is not used.
Import of 'Any' is not used.
| from typing import List, Dict, Any, Optional | |
| from typing import List, Optional |
| self.log.warning("failed to create changeset for " + path) | ||
| # probably the database schema has been modified if geodiff cannot create changeset. | ||
| # we will need to do full upload of the file | ||
| pass |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass |
|
|
||
| for change in added: | ||
| result.append( | ||
| ProjectDeltaItem( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to ProjectDeltaChange with type arg
| """ | ||
|
|
||
| type: PullActionType | ||
| pull_delta_item: ProjectDeltaItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add just necessary values.
Resolved #280
/deltaendpoint, where pull changes between current version and server are calculated from delta itemsFixes
Pull logic updates
in
pull_project_asyncis check if server supports v2_pullIf supports: get pull delta from server
If not: calculate pull delta from files and projects
config.jsonThen calculate delta items between local files and server (local delta) - delta needs to be applied to server
Compare pull_delta and local_delta ->
PullActionwith actions needs to be done after files are downloaded from a server defined inPullActionTypeenumStarting download
After download is finished -> call
pull_project_finalizeMethod
apply_pull_actionsis then responsible to finalizePullActionbased on type