refactor

renku/core/dataset/dataset.py

@@ -15,6 +15,7 @@
 # limitations under the License.
 """Dataset business logic."""
 
+import imghdr
 import os
 import shutil
 import urllib
@@ -50,6 +51,7 @@
     get_absolute_path,
     get_file_size,
     get_files,
+    get_relative_path,
     get_safe_relative_path,
     hash_file,
     is_path_empty,
@@ -875,15 +877,30 @@ def set_dataset_images(dataset: Dataset, images: Optional[List[ImageObjectReques
     dataset.images = []
     images_updated = False
     for img in images:
+        image_folder = project_context.dataset_images_path / dataset.initial_identifier
         try:
-            img_object = img.to_image_object(
-                image_folder=project_context.dataset_images_path / dataset.initial_identifier, owner_id=dataset.id
-            )
+            img_object = img.to_image_object(owner_id=dataset.id)
         except errors.ImageError as e:
             raise errors.DatasetImageError(e) from e
 
-        if not img_object:
-            continue
+        path = img_object.content_url
+
+        if not img_object.is_remote:
+            # NOTE: only copy dataset image if it's not in .renku/datasets/<id>/images/ already
+            if not path.startswith(str(image_folder)):
+                image_type = imghdr.what(path)
+                if image_type:
+                    ext = f".{image_type}"
+                else:
+                    _, ext = os.path.splitext(path)
+                target_image_path: Union[Path, str] = image_folder / f"{img_object.position}{ext}"
+
+                image_folder.parent.mkdir(parents=True, exist_ok=True)
+                shutil.copy(path, target_image_path)
+            else:
+                target_image_path = path
+
+            img_object.content_url = get_relative_path(target_image_path, base=project_context.path)  # type: ignore
 
         if any(i.position == img_object.position for i in dataset.images):
             raise errors.DatasetImageError(f"Duplicate dataset image specified for position {img_object.position}")

renku/core/image.py

@@ -21,7 +21,6 @@
 
 import imghdr
 import os
-import shutil
 import urllib
 from pathlib import Path
 from typing import List, Optional, Union, cast
@@ -50,69 +49,7 @@ def __init__(
         self.mirror_locally = mirror_locally
         self.safe_image_paths: List[Union[str, Path]] = cast(List[Union[str, Path]], safe_image_paths) or []
 
-    def to_image_object(self, image_folder: Path, owner_id: str) -> ImageObject:
-        """Convert request model to ``ImageObject``."""
-        image_type = None
-        self.safe_image_paths.append(project_context.path)
-
-        image_folder.mkdir(exist_ok=True, parents=True)
-
-        if urllib.parse.urlparse(self.content_url).netloc:
-            # NOTE: absolute url
-            if not self.mirror_locally:
-                return ImageObject(
-                    content_url=self.content_url,
-                    position=self.position,
-                    id=ImageObject.generate_id(owner_id=owner_id, position=self.position),
-                )
-
-            # NOTE: mirror the image locally
-            try:
-                path, _ = urlretrieve(self.content_url)
-            except urllib.error.URLError as e:
-                raise errors.ImageError(f"Image with url {self.content_url} couldn't be mirrored") from e
-
-            image_type = imghdr.what(path)
-            if image_type:
-                image_type = f".{image_type}"
-
-            self.content_url = path
-            self.safe_image_paths.append(Path(path).parent)
-
-        path = self.content_url
-        if not os.path.isabs(path):
-            path = os.path.normpath(os.path.join(project_context.path, path))
-
-        if not os.path.exists(path) or not any(
-            os.path.commonprefix([path, p]) == str(p) for p in self.safe_image_paths
-        ):
-            # NOTE: make sure files exists and prevent path traversal
-            raise errors.ImageError(f"Image with relative path {self.content_url} not found")
-
-        # TODO: Delete the old image since it might have a different extension and won't get copied over
-
-        if not path.startswith(str(image_folder)):
-            # NOTE: only copy dataset image if it's not in .renku/datasets/<id>/images/ already
-            if image_type:
-                ext = image_type
-            else:
-                _, ext = os.path.splitext(self.content_url)
-
-            img_path = image_folder / f"{self.position}{ext}"
-            shutil.copy(path, img_path)
-        else:
-            img_path = Path(path)
-
-        return ImageObject(
-            content_url=str(img_path.relative_to(project_context.path)),
-            position=self.position,
-            id=ImageObject.generate_id(owner_id=owner_id, position=self.position),
-        )
-
-    def download_image(self, owner_id: str) -> ImageObject:
-        """Download the image and save it to a temporary file."""
-
-        # def to_image_object(self, image_folder: Path, owner_id: str) -> ImageObject:
+    def to_image_object(self, owner_id: str) -> ImageObject:
         """Convert request model to ``ImageObject``."""
         self.safe_image_paths.append(project_context.path)
 
@@ -131,7 +68,6 @@ def download_image(self, owner_id: str) -> ImageObject:
                 raise errors.ImageError(f"Cannot download image with url {self.content_url}: {e}") from e
 
             path = Path(tmp_path)
-            self.safe_image_paths.append(Path(path).parent)
         else:
             path = Path(self.content_url).resolve()
 

renku/core/project.py

@@ -117,7 +117,7 @@ def set_project_image(image_request: Optional[ImageObjectRequest]) -> None:
     if image_request is None:
         return
 
-    image_object = image_request.download_image(owner_id=project_context.project.id)
+    image_object = image_request.to_image_object(owner_id=project_context.project.id)
 
     project_image = project_context.project_image_pathname
 
@@ -132,6 +132,8 @@ def set_project_image(image_request: Optional[ImageObjectRequest]) -> None:
 
             image_object.content_url = get_relative_path(project_image, base=project_context.path)  # type: ignore
 
+    image_object.position = 0
+
     project_context.project.image = image_object
 
 

renku/ui/service/views/error_handlers.py

@@ -360,7 +360,7 @@ def decorated_function(*args, **kwargs):
             error_message = str(e)
             if "Duplicate dataset image" in error_message:
                 raise UserDatasetsMultipleImagesError(e)
-            elif "couldn't be mirrored" in error_message:
+            elif "Cannot download image with url" in error_message:
                 raise UserDatasetsUnreachableImageError(e)
             raise
         except ValidationError as e:

tests/cli/test_init.py

@@ -483,7 +483,11 @@ def test_init_with_image(isolated_runner, template, image):
 
     assert 0 == result.exit_code, format_result_exception(result)
 
+    project = Database.from_path(Path("new-project") / ".renku" / "metadata").get("project")
+
     assert (Path("new-project") / ".renku" / "images" / "project" / "0.png").exists()
+    assert ".renku/images/project/0.png" == project.image.content_url
+    assert 0 == project.image.position
 
 
 @pytest.mark.parametrize(

tests/cli/test_project.py

@@ -104,6 +104,9 @@ def test_project_edit(runner, project, subdirectory, with_injection):
     assert "Renku Version:" in result.output
     assert "Keywords:" in result.output
 
+    assert ".renku/images/project/0.png" == project.image.content_url
+    assert 0 == project.image.position
+
     result = runner.invoke(cli, ["graph", "export", "--format", "json-ld", "--strict"])
     assert 0 == result.exit_code, format_result_exception(result)
 
@@ -196,6 +199,8 @@ def test_project_edit_unset(runner, project, subdirectory, with_injection):
     assert "Renku Version:" in result.output
     assert "Keywords:" in result.output
 
+    assert project.image is None
+
     result = runner.invoke(cli, ["graph", "export", "--format", "json-ld", "--strict"])
     assert 0 == result.exit_code, format_result_exception(result)
 

tests/core/fixtures/core_database.py

@@ -78,7 +78,7 @@ def database() -> Iterator[Tuple["Database", DummyStorage]]:
 
 @pytest.fixture
 def with_injection():
-    """Factory fixture for test injections manager."""
+    """Factory fixture for test injection manager."""
     from renku.command.command_builder.command import inject, remove_injector
     from renku.domain_model.project_context import project_context