diff --git a/lean/components/api/file_client.py b/lean/components/api/file_client.py index cb22194af..65b0ae7b5 100644 --- a/lean/components/api/file_client.py +++ b/lean/components/api/file_client.py @@ -11,7 +11,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import List +from typing import List, Optional from lean.components.api.api_client import * from lean.models.api import QCFullFile, QCMinimalFile @@ -53,39 +53,49 @@ def get_all(self, project_id: int) -> List[QCFullFile]: return [QCFullFile(**file) for file in data["files"]] - def create(self, project_id: int, file_name: str, content: str) -> None: + def create(self, project_id: int, file_name: str, content: str, code_source_id: Optional[str] = None) -> None: """Creates a new file. :param project_id: the id of the project to create a file for :param file_name: the name of the file to create :param content: the content of the file to create + :param code_source_id: the source of the code changes (e.g., "cli") """ - data = self._api.post("files/create", { + self._api.post("files/create", self._with_code_source_id({ "projectId": project_id, "name": file_name, "content": content - }) + }, code_source_id)) - def update(self, project_id: int, file_name: str, content: str) -> None: + def update(self, project_id: int, file_name: str, content: str, code_source_id: Optional[str] = None) -> None: """Updates an existing file. :param project_id: the id of the project to update a file in :param file_name: the name of the file to update :param content: the new content of the file + :param code_source_id: the source of the code changes (e.g., "cli") """ - data = self._api.post("files/update", { + self._api.post("files/update", self._with_code_source_id({ "projectId": project_id, "name": file_name, "content": content - }) + }, code_source_id)) - def delete(self, project_id: int, file_name: str) -> None: + def delete(self, project_id: int, file_name: str, code_source_id: Optional[str] = None) -> None: """Deletes an existing file. :param project_id: the id of the project the file belongs to :param file_name: the name of the file to delete + :param code_source_id: the source of the code changes (e.g., "cli") """ - self._api.post("files/delete", { + self._api.post("files/delete", self._with_code_source_id({ "projectId": project_id, "name": file_name - }) + }, code_source_id)) + + @staticmethod + def _with_code_source_id(request_parameters: dict, code_source_id: Optional[str]) -> dict: + # The API only checks the project's collaboration lock when codeSourceId is sent + if code_source_id is not None: + request_parameters["codeSourceId"] = code_source_id + return request_parameters diff --git a/lean/components/cloud/push_manager.py b/lean/components/cloud/push_manager.py index d3df19bcd..cb9a76e59 100644 --- a/lean/components/cloud/push_manager.py +++ b/lean/components/cloud/push_manager.py @@ -12,7 +12,8 @@ # limitations under the License. from pathlib import Path -from typing import List, Dict, Optional +from functools import partial +from typing import Any, List, Dict, Optional from lean.components.api.api_client import APIClient from lean.components.config.project_config_manager import ProjectConfigManager @@ -22,6 +23,7 @@ from lean.models.api import QCLanguage, QCProject from lean.models.utils import LeanLibraryReference from lean.models.encryption import ActionType +from lean.models.errors import PartialPushError, RequestFailedError class PushManager: """The PushManager class is responsible for synchronizing local projects to the cloud.""" @@ -69,6 +71,7 @@ def push_projects(self, projects_to_push: List[Path], associated_libraries_to_pu return organization_id = self._organization_manager.try_get_working_organization_id() + partly_pushed_projects = [] for index, path in enumerate(all_projects_to_push, start=1): @@ -85,6 +88,14 @@ def push_projects(self, projects_to_push: List[Path], associated_libraries_to_pu self._logger.warn(f"Cannot push '{relative_path}': {ex}") if "write permission" in str(ex).lower(): self._logger.info("Please pull any required changes and push with --force") + if isinstance(ex, PartialPushError): + partly_pushed_projects.append(relative_path.as_posix()) + + # A partly pushed project must not be compiled, backtested or deployed by the command that pushed it + if len(partly_pushed_projects) > 0: + raise PartialPushError(f"Some projects were only partly pushed: " + f"{', '.join(repr(name) for name in partly_pushed_projects)}. " + f"Push again after fixing the errors above") def _get_local_libraries_cloud_ids(self, project_dir: Path) -> List[int]: project_config = self._project_config_manager.get_project_config(project_dir) @@ -259,7 +270,12 @@ def _push_metadata(self, project: Path, cloud_project: QCProject, encryption_act if not force: update_args["code_source_id"] = "cli" if update_args != {}: - self._api_client.projects.update(cloud_project.projectId, **update_args) + try: + self._api_client.projects.update(cloud_project.projectId, **update_args) + except RequestFailedError as error: + if error.response.status_code != 413: + raise + self._push_one_by_one(cloud_project, update_args) if "encryption_key" in update_args: del update_args["encryption_key"] @@ -274,6 +290,56 @@ def _push_metadata(self, project: Path, cloud_project: QCProject, encryption_act self._logger.info(f"Successfully updated {updated_keys_str} for '{cloud_project.name}'") + def _push_one_by_one(self, cloud_project: QCProject, update_args: Dict[str, Any]) -> None: + """Pushes a project that is too large for a single projects/update request, with one request per change. + + The files go first and the other metadata last. The API checks the collaboration lock on files/* requests + that carry codeSourceId, but not on a projects/update without files, so a lock held by someone else stops + the push before any file is written. + + :param cloud_project: the cloud project to push to + :param update_args: the arguments of the projects/update request that was too large + """ + project_id = cloud_project.projectId + code_source_id = update_args.get("code_source_id") + local_files = update_args["files"] + local_file_names = {file["name"] for file in local_files} + self._logger.info(f"'{cloud_project.name}' is too large to push in one request, pushing its changed files one by one") + cloud_files = {file.name: file for file in self._api_client.files.get_all(project_id)} + + # Deletes go first, so a rename in a project at its file quota doesn't go over the quota + requests = [(f"deleting '{cloud_file.name}'", + partial(self._api_client.files.delete, project_id, cloud_file.name, code_source_id=code_source_id)) + for cloud_file in cloud_files.values() + if not cloud_file.isLibrary and cloud_file.name not in local_file_names] + for file in local_files: + cloud_file = cloud_files.get(file["name"]) + if cloud_file is None: + requests.append((f"creating '{file['name']}'", + partial(self._api_client.files.create, project_id, file["name"], file["content"], + code_source_id=code_source_id))) + # Trailing whitespace can't change the code, leading whitespace can (Python indentation) + elif cloud_file.content.rstrip() != file["content"].rstrip(): + requests.append((f"updating '{file['name']}'", + partial(self._api_client.files.update, project_id, file["name"], file["content"], + code_source_id=code_source_id))) + requests.append(("updating the project details", + partial(self._api_client.projects.update, project_id, + **{key: value for key, value in update_args.items() if key != "files"}))) + + for index, (description, request) in enumerate(requests): + try: + request() + except Exception as error: + if index == 0: + raise RuntimeError(f"{description} failed, nothing was pushed: {error}") from error + if index == len(requests) - 1: + raise PartialPushError(f"All files of '{cloud_project.name}' were pushed, but updating the project " + f"details failed. Fix the problem and push again. Error: {error}") from error + raise PartialPushError(f"Only part of '{cloud_project.name}' was pushed: {description} failed, so the " + f"cloud project now mixes old and new code. Fix the problem and push again. " + f"Error: {error}") from error + def _get_cloud_project(self, project_id: int, organization_id: str) -> QCProject: project = next(iter(p for p in self._cloud_projects if p.projectId == project_id), None) if project is None: diff --git a/lean/models/errors.py b/lean/models/errors.py index cb477a129..ca28de15f 100644 --- a/lean/models/errors.py +++ b/lean/models/errors.py @@ -34,6 +34,10 @@ def __init__(self, response, message: Optional[str] = None) -> None: self.response = response +class PartialPushError(Exception): + """A PartialPushError indicates that a push stopped after some of the project's changes were written to the cloud.""" + + class MoreInfoError(Exception): """An error which consists of a message and a link to documentation with more information.""" diff --git a/tests/components/api/test_file_client.py b/tests/components/api/test_file_client.py new file mode 100644 index 000000000..f780d15dd --- /dev/null +++ b/tests/components/api/test_file_client.py @@ -0,0 +1,36 @@ +# QUANTCONNECT.COM - Democratizing Finance, Empowering Individuals. +# Lean CLI v1.0. Copyright 2021 QuantConnect Corporation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import Optional +from unittest import mock + +import pytest + +from lean.components.api.file_client import FileClient + + +@pytest.mark.parametrize("method, args, endpoint, expected_parameters", [ + ("create", (1, "main.py", "content"), "files/create", {"projectId": 1, "name": "main.py", "content": "content"}), + ("update", (1, "main.py", "content"), "files/update", {"projectId": 1, "name": "main.py", "content": "content"}), + ("delete", (1, "main.py"), "files/delete", {"projectId": 1, "name": "main.py"}) +]) +@pytest.mark.parametrize("code_source_id", [None, "cli"]) +def test_file_client_sends_code_source_id_only_when_given(method: str, args: tuple, endpoint: str, + expected_parameters: dict, code_source_id: Optional[str]) -> None: + api_client = mock.Mock() + + getattr(FileClient(api_client), method)(*args, code_source_id=code_source_id) + + if code_source_id is not None: + expected_parameters = {**expected_parameters, "codeSourceId": code_source_id} + api_client.post.assert_called_once_with(endpoint, expected_parameters) diff --git a/tests/components/cloud/test_cloud_project_manager.py b/tests/components/cloud/test_cloud_project_manager.py index fbf0fa573..0813915af 100644 --- a/tests/components/cloud/test_cloud_project_manager.py +++ b/tests/components/cloud/test_cloud_project_manager.py @@ -14,7 +14,10 @@ from pathlib import Path from unittest import mock +import pytest + from lean.container import container +from lean.models.errors import PartialPushError from tests.test_helpers import create_api_project, create_fake_lean_cli_directory from tests.conftest import initialize_container @@ -49,3 +52,19 @@ def test_get_cloud_project_pushing_new_project(): api_client.projects.get.assert_called_with(cloud_project.projectId, "abc") push_manager.push_projects.assert_called_once_with([Path.cwd() / "Python Project"]) + + +def test_get_cloud_project_does_not_return_a_partly_pushed_project(): + create_fake_lean_cli_directory() + + api_client = mock.Mock() + push_manager = mock.Mock() + push_manager.push_projects = mock.Mock(side_effect=PartialPushError("Some projects were only partly pushed")) + + initialize_container(api_client_to_use=api_client, push_manager_to_use=push_manager) + + # Commands like "lean cloud backtest --push" must not run on a mix of old and new cloud code + with pytest.raises(PartialPushError): + container.cloud_project_manager.get_cloud_project("Python Project", push=True) + + api_client.projects.get.assert_not_called() diff --git a/tests/components/util/test_push_manager.py b/tests/components/util/test_push_manager.py index f3090d85a..14968ed9f 100644 --- a/tests/components/util/test_push_manager.py +++ b/tests/components/util/test_push_manager.py @@ -11,14 +11,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +from datetime import datetime from pathlib import Path -from typing import List +from typing import List, Optional from unittest import mock import pytest import platform from lean.components.cloud.push_manager import PushManager from lean.container import container -from lean.models.api import QCLanguage, QCProject +from lean.models.api import QCFullFile, QCLanguage, QCProject +from lean.models.errors import PartialPushError, RequestFailedError from tests.test_helpers import create_fake_lean_cli_directory, create_api_project, create_lean_environments from tests.test_helpers import create_fake_lean_cli_project from lean.components import forbidden_characters @@ -527,4 +529,129 @@ def test_push_projects_updates_name_locally_if_updated_by_cloud() -> None: push_manager.push_projects([project_path]) assert not (Path.cwd() / project_path).exists() - assert (Path.cwd() / project_name_renamed_by_cloud).exists() \ No newline at end of file + assert (Path.cwd() / project_name_renamed_by_cloud).exists() + +def _create_too_large_project_push_manager(api_client: mock.Mock) -> PushManager: + create_fake_lean_cli_directory() + + project_path = Path.cwd() / "Python Project" + (project_path / "new_file.py").write_text("# new file", encoding="utf-8") + container.project_config_manager.get_project_config(project_path).set("cloud-id", 1000) + + def get_local_project_path(project_name, *args): + return Path.cwd() / project_name + + project_manager = mock.Mock() + project_manager.get_project_libraries = mock.MagicMock(return_value=[]) + project_manager.get_source_files = mock.MagicMock(return_value=[project_path / "main.py", + project_path / "research.ipynb", + project_path / "new_file.py"]) + project_manager.get_local_project_path = mock.MagicMock(side_effect=get_local_project_path) + + api_client.projects.get = mock.MagicMock(return_value=create_api_project(1000, project_path.name)) + return _create_push_manager(api_client, project_manager) + + +def _create_cloud_file(name: str, content: str, is_library: bool = False) -> QCFullFile: + return QCFullFile(name=name, content=content, modified=datetime.now(), isLibrary=is_library) + + +@pytest.mark.parametrize("force, expected_code_source_id", [(False, "cli"), (True, None)]) +def test_push_projects_pushes_files_one_by_one_when_the_project_is_too_large(force: bool, + expected_code_source_id: Optional[str]) -> None: + api_client = mock.Mock() + api_client.projects.update = mock.MagicMock(side_effect=[RequestFailedError(mock.Mock(status_code=413)), None]) + push_manager = _create_too_large_project_push_manager(api_client) + + project_path = Path.cwd() / "Python Project" + main_content = (project_path / "main.py").read_text(encoding="utf-8") + research_content = (project_path / "research.ipynb").read_text(encoding="utf-8") + api_client.files.get_all = mock.MagicMock(return_value=[ + # Extra trailing whitespace can't change the code, extra leading whitespace can + _create_cloud_file("main.py", main_content + "\n\n"), + _create_cloud_file("research.ipynb", " " + research_content), + _create_cloud_file("removed.py", "# removed locally"), + _create_cloud_file("Library/library.py", "# library file", is_library=True) + ]) + + push_manager.push_projects([project_path], force=force) + + assert api_client.projects.update.call_count == 2 + first_call_kwargs = api_client.projects.update.call_args_list[0].kwargs + second_call_args, second_call_kwargs = api_client.projects.update.call_args_list[1] + assert "files" in first_call_kwargs + assert first_call_kwargs.get("code_source_id") == expected_code_source_id + assert second_call_args[0] == 1000 + assert second_call_kwargs == {key: value for key, value in first_call_kwargs.items() if key != "files"} + + api_client.files.create.assert_called_once_with(1000, "new_file.py", "# new file", + code_source_id=expected_code_source_id) + api_client.files.update.assert_called_once_with(1000, "research.ipynb", research_content, + code_source_id=expected_code_source_id) + api_client.files.delete.assert_called_once_with(1000, "removed.py", code_source_id=expected_code_source_id) + + # Deletes go first and the metadata last, so a collaboration lock stops the push before anything is written + write_calls = [name for name, _, _ in api_client.mock_calls + if name in ["projects.update", "files.delete", "files.create", "files.update"]] + assert write_calls == ["projects.update", "files.delete", "files.update", "files.create", "projects.update"] + + +def test_push_projects_leaves_the_project_unchanged_when_the_first_file_push_fails() -> None: + api_client = mock.Mock() + api_client.projects.update = mock.MagicMock(side_effect=[RequestFailedError(mock.Mock(status_code=413)), None]) + api_client.files.get_all = mock.MagicMock(return_value=[]) + api_client.files.create = mock.MagicMock(side_effect=RequestFailedError(mock.Mock(), "You do not have write permission")) + push_manager = _create_too_large_project_push_manager(api_client) + + push_manager.push_projects([Path.cwd() / "Python Project"]) + + api_client.files.create.assert_called_once() + api_client.projects.update.assert_called_once() + push_manager._logger.warn.assert_called_once_with("Cannot push 'Python Project': creating 'main.py' failed, " + "nothing was pushed: You do not have write permission") + push_manager._logger.info.assert_any_call("Please pull any required changes and push with --force") + + +def test_push_projects_raises_when_a_file_push_fails_after_other_files_were_pushed() -> None: + api_client = mock.Mock() + api_client.projects.update = mock.MagicMock(side_effect=[RequestFailedError(mock.Mock(status_code=413)), None]) + api_client.files.get_all = mock.MagicMock(return_value=[]) + api_client.files.create = mock.MagicMock(side_effect=[None, RequestFailedError(mock.Mock(), "Internal error")]) + push_manager = _create_too_large_project_push_manager(api_client) + + with pytest.raises(PartialPushError, match="Some projects were only partly pushed: 'Python Project'"): + push_manager.push_projects([Path.cwd() / "Python Project"]) + + assert api_client.files.create.call_count == 2 + api_client.projects.update.assert_called_once() + warning = push_manager._logger.warn.call_args.args[0] + assert "Only part of 'Python Project' was pushed: creating 'research.ipynb' failed" in warning + assert "Internal error" in warning + + +def test_push_projects_raises_when_only_the_project_details_fail_after_the_files_were_pushed() -> None: + api_client = mock.Mock() + api_client.projects.update = mock.MagicMock(side_effect=[RequestFailedError(mock.Mock(status_code=413)), + RequestFailedError(mock.Mock(), "Name already taken")]) + api_client.files.get_all = mock.MagicMock(return_value=[]) + push_manager = _create_too_large_project_push_manager(api_client) + + with pytest.raises(PartialPushError): + push_manager.push_projects([Path.cwd() / "Python Project"]) + + assert api_client.files.create.call_count == 3 + warning = push_manager._logger.warn.call_args.args[0] + assert "All files of 'Python Project' were pushed, but updating the project details failed" in warning + assert "Name already taken" in warning + + +def test_push_projects_does_not_push_files_one_by_one_on_other_errors() -> None: + api_client = mock.Mock() + api_client.projects.update = mock.MagicMock(side_effect=RequestFailedError(mock.Mock(status_code=400))) + push_manager = _create_too_large_project_push_manager(api_client) + + push_manager.push_projects([Path.cwd() / "Python Project"]) + + api_client.projects.update.assert_called_once() + api_client.files.get_all.assert_not_called() + api_client.files.create.assert_not_called()