- tests for uploads

- fixed issues found via tests
- disallow uploads for banned maps, temporary maps, and legacy maps

Author: alexlambson

kirovy/constants/api_codes.py

@@ -26,3 +26,8 @@ class LegacyUploadApiCodes(enum.StrEnum):
 class FileUploadApiCodes(enum.StrEnum):
     MISSING_FOREIGN_ID = "missing-foreign-id"
     INVALID = "file-failed-validation"
+    UNSUPPORTED = "parent-does-not-support-this-upload"
+    """attr: Raised when the parent object for the file does not allow this upload.
+
+    e.g. a temporary map does not support custom image uploads.
+    """

kirovy/models/cnc_map.py

@@ -292,7 +292,7 @@ def generate_upload_to(instance: "CncMapImageFile", filename: str) -> pathlib.Pa
             Path to upload map to relative to :attr:`~kirovy.settings.base.MEDIA_ROOT`.
         """
         filename = pathlib.Path(filename)
-        final_file_name = f"{instance.created.isoformat('-')}_{instance.id.hex}{filename.suffix}"
+        final_file_name = f"{instance.created.date().isoformat()}_{instance.id.hex}{filename.suffix}"
 
         # e.g. "yr/map_images/CNC_NET_MAP_ID_HEX/2020-01-01_somelongid.jpg
         return pathlib.Path(instance.cnc_map.get_map_directory_path(instance.UPLOAD_TYPE), final_file_name)

kirovy/urls.py

@@ -108,7 +108,7 @@ def _get_url_patterns() -> list[_DjangoPath]:
     path("<uuid:pk>/", cnc_map_views.MapRetrieveUpdateView.as_view()),
     path("delete/<uuid:pk>/", cnc_map_views.MapDeleteView.as_view()),
     path("search/", cnc_map_views.MapListView.as_view()),
-    path("img", cnc_map_views.MapImageFileUploadView.as_view()),
+    path("img/", cnc_map_views.MapImageFileUploadView.as_view()),
     # path("img/<uuid:map_id>/", ...),
     # path("search/")
 ]

kirovy/views/base_views.py

@@ -194,6 +194,7 @@ def get_parent_object(self, request: KirovyRequest) -> GameScopedUserOwnedModel:
     def post(self, request: KirovyRequest, format=None) -> KirovyResponse[ui_objects.ResultResponseData]:
         uploaded_file: UploadedFile = request.data["file"]
         parent_object = self.get_parent_object(request)
+        self.extra_verification(request, uploaded_file, parent_object)
 
         extension_id = CncFileExtension.get_extension_id_for_upload(
             uploaded_file,
@@ -206,7 +207,7 @@ def post(self, request: KirovyRequest, format=None) -> KirovyResponse[ui_objects
             data={
                 "cnc_game_id": parent_object.cnc_game_id,
                 self.file_parent_attr_name: parent_object.id,
-                "name": request.get("name"),
+                "name": uploaded_file.name,
                 "file": uploaded_file,
                 "file_extension_id": extension_id,
                 **self.extra_serializer_data(request, uploaded_file, parent_object),
@@ -223,13 +224,25 @@ def post(self, request: KirovyRequest, format=None) -> KirovyResponse[ui_objects
             ui_objects.ResultResponseData(
                 message=self.success_message,
                 result={
+                    "file_id": saved.id,
                     "file_url": saved.file.url,
                     "parent_object_id": parent_object.id,
                 },
-            )
+            ),
+            status=status.HTTP_201_CREATED,
         )
 
     def extra_serializer_data(
         self, request: KirovyRequest, uploaded_file: UploadedFile, parent_object: GameScopedUserOwnedModel
     ) -> t.Dict[str, t.Any]:
         raise NotImplementedError()
+
+    def extra_verification(
+        self, request: KirovyRequest, uploaded_file: UploadedFile, parent_object: GameScopedUserOwnedModel
+    ) -> None:
+        """Any extra verification that the file needs.
+
+        :raises kirovy.exceptions.view_exceptions.KirovyValidationError:
+            Raised for any issues.
+        """
+        raise NotImplementedError()

kirovy/views/cnc_map_views.py

@@ -328,8 +328,14 @@ class MapImageFileUploadView(base_views.FileUploadBaseView):
     success_message = "Map image uploaded successfully."
 
     def extra_serializer_data(
-        self, request: KirovyRequest, uploaded_file: UploadedFile, parent_object: GameScopedUserOwnedModel
+        self, request: KirovyRequest, uploaded_file: UploadedFile, parent_object: CncMap
     ) -> t.Dict[str, t.Any]:
+        latest_image: CncMapImageFile | None = (
+            CncMapImageFile.objects.filter(cnc_map_id=parent_object.id)
+            .order_by("-image_order")
+            .only("image_order")
+            .first()
+        )
         try:
             with Image.open(uploaded_file) as image:
                 height = image.height
@@ -340,4 +346,15 @@ def extra_serializer_data(
                 {"user_id": request.user.id, "username": request.user.username, "e": str(e)},
             )
             raise KirovyValidationError("Image is invalid", code=api_codes.FileUploadApiCodes.INVALID)
-        return {"height": height, "width": width}
+        return {
+            "height": height,
+            "width": width,
+            "is_extracted": False,
+            "image_order": latest_image.image_order if latest_image else 0,
+        }
+
+    def extra_verification(self, request: KirovyRequest, uploaded_file: UploadedFile, parent_object: CncMap) -> None:
+        if parent_object.is_legacy or parent_object.is_temporary:
+            raise KirovyValidationError(
+                "Map type does not support custom preview images", code=api_codes.FileUploadApiCodes.UNSUPPORTED
+            )

tests/fixtures/file_fixtures.py

@@ -188,3 +188,40 @@ def _inner(file: File | io.BytesIO) -> t.Tuple[ContentFile, ZipContentsSha1]:
         return ContentFile(zip_bytes.read(), f"{file_sha1}.zip"), file_sha1
 
     return _inner
+
+
+@pytest.fixture
+def file_map_image(load_test_file) -> Generator[File, Any, None]:
+    """Return a valid map preview image."""
+    file = load_test_file("test_images/2_across_the_frost.png")
+    yield file
+    file.close()
+
+
+@pytest.fixture
+def file_map_image_jpg(load_test_file) -> Generator[File, Any, None]:
+    """Return a valid map preview image in jpg."""
+    file = load_test_file("test_images/uptown.jpg")
+    yield file
+    file.close()
+
+
+@pytest.fixture
+def get_file_path_for_uploaded_file_url(settings, tmp_media_root):
+    """Returns a function to convert an uploaded file's URL to the actual file path in the tmp folder.
+
+    Used to check that an uploaded file actually exists on disk after uploading in tests.
+    """
+
+    def _inner(uploaded_file_url: str) -> pathlib.Path:
+        """Convert an uploaded file's URL to its physical filepath in the tmp directory.
+
+        :param uploaded_file_url:
+            The URL path for an uploaded file.
+        :return:
+            The file path to the uploaded file.
+        """
+        strip_media_url = f"/{settings.MEDIA_URL}"
+        return pathlib.Path(tmp_media_root) / uploaded_file_url.lstrip(strip_media_url)
+
+    return _inner

tests/test_data/test_images/2_across_the_frost.png

@@ -2,22 +2,26 @@
 import zipfile
 import io
 
+import pytest
 from django.core.files.uploadedfile import UploadedFile
 from rest_framework import status
 
 from kirovy import settings, typing as t
-from kirovy.constants.api_codes import UploadApiCodes
+from kirovy.constants.api_codes import UploadApiCodes, FileUploadApiCodes
 from kirovy.models.cnc_map import CncMapImageFile
 from kirovy.utils import file_utils
 from kirovy.models import CncMap, CncMapFile, MapCategory
 from kirovy.response import KirovyResponse
 from kirovy.services.cnc_gen_2_services import CncGen2MapParser
+from kirovy.views.cnc_map_views import MapImageFileUploadView
 
 _UPLOAD_URL = "/maps/upload/"
 _CLIENT_URL = "/maps/client/upload/"
 
 
-def test_map_file_upload_happy_path(client_user, file_map_desert, game_yuri, extension_map, tmp_media_root):
+def test_map_file_upload_happy_path(
+    client_user, file_map_desert, game_yuri, extension_map, tmp_media_root, get_file_path_for_uploaded_file_url
+):
     response: KirovyResponse = client_user.post(
         _UPLOAD_URL,
         {"file": file_map_desert, "game_id": str(game_yuri.id)},
@@ -30,11 +34,9 @@ def test_map_file_upload_happy_path(client_user, file_map_desert, game_yuri, ext
     uploaded_file_url: str = response.data["result"]["cnc_map_file"]
     uploaded_image_url: str = response.data["result"]["extracted_preview_file"]
 
-    # We need to strip the url path off of the files,
-    # then check the tmp directory to make sure the uploaded files were saved
-    strip_media_url = f"/{settings.MEDIA_URL}"
-    uploaded_file_path = pathlib.Path(tmp_media_root) / uploaded_file_url.lstrip(strip_media_url)
-    uploaded_image = pathlib.Path(tmp_media_root) / uploaded_image_url.lstrip(strip_media_url)
+    # We need to check the tmp directory to make sure the uploaded files were saved
+    uploaded_file_path = get_file_path_for_uploaded_file_url(uploaded_file_url)
+    uploaded_image = get_file_path_for_uploaded_file_url(uploaded_image_url)
     assert uploaded_file_path.exists()
     assert uploaded_image.exists()
 
@@ -115,3 +117,108 @@ def test_map_file_upload_banned_map(banned_cheat_map, file_map_unfair, client_an
     assert response.status_code == status.HTTP_400_BAD_REQUEST
     assert response.data["code"] == UploadApiCodes.DUPLICATE_MAP
     assert response.data["additional"]["existing_map_id"] == str(banned_cheat_map.id)
+
+
+def test_map_image_upload__happy_path(create_cnc_map, file_map_image, client_user, get_file_path_for_uploaded_file_url):
+    """Test that we can upload an image as a verified user, who has created a map"""
+    cnc_map = create_cnc_map(user_id=client_user.kirovy_user.id, is_legacy=False, is_published=True, is_temporary=False)
+    original_image_count = cnc_map.cncmapimagefile_set.select_related().count()
+    response = client_user.post(
+        "/maps/img/",
+        {"file": file_map_image, "cnc_map_id": str(cnc_map.id)},
+        format="multipart",
+        content_type=None,
+    )
+
+    assert response.status_code == status.HTTP_201_CREATED
+    assert response.data["message"] == MapImageFileUploadView.success_message
+    saved_file = CncMapImageFile.objects.get(id=response.data["result"]["file_id"])
+    image_url: str = response.data["result"]["file_url"]
+    parent_id: str = response.data["result"]["parent_object_id"]
+
+    assert parent_id == cnc_map.id
+    expected_date = saved_file.created.date().isoformat()
+    assert image_url == f"/silo/yr/map_images/{cnc_map.id.hex}/{expected_date}_{saved_file.id.hex}.png"
+
+    assert get_file_path_for_uploaded_file_url(image_url).exists()
+
+    assert cnc_map.cncmapimagefile_set.select_related().count() == original_image_count + 1
+    # Image order starts at 0, then gets incremented, so image_order should be current_count - 1
+    assert saved_file.image_order == original_image_count
+    assert saved_file.name == pathlib.Path(file_map_image.name).name
+    assert saved_file.file_extension.extension == "png"
+    # Width and height are from the image itself.
+    assert saved_file.width == 768
+    assert saved_file.height == 494
+
+
+def test_map_image_upload__jpg(create_cnc_map, file_map_image_jpg, client_user, get_file_path_for_uploaded_file_url):
+    """Test that we can upload a jpg."""
+    cnc_map = create_cnc_map(user_id=client_user.kirovy_user.id, is_legacy=False, is_published=True, is_temporary=False)
+    response = client_user.post(
+        "/maps/img/",
+        {"file": file_map_image_jpg, "cnc_map_id": str(cnc_map.id)},
+        format="multipart",
+        content_type=None,
+    )
+
+    assert response.status_code == status.HTTP_201_CREATED
+    assert response.data["message"] == MapImageFileUploadView.success_message
+    saved_file = CncMapImageFile.objects.get(id=response.data["result"]["file_id"])
+    assert saved_file.name == pathlib.Path(file_map_image_jpg.name).name
+    assert saved_file.file_extension.extension == "jpg"
+
+    # Width and height are from the image itself.
+    assert saved_file.width == 993
+    assert saved_file.height == 740
+
+
+@pytest.mark.parametrize("map_kwargs", [{"is_legacy": True}, {"is_temporary": True}])
+def test_map_image_upload__unsupported_map(create_cnc_map, file_map_image, client_user, map_kwargs):
+    """Test that map image uploads fail for legacy and temporary maps."""
+    cnc_map = create_cnc_map(user_id=client_user.kirovy_user.id, **map_kwargs)
+    original_image_count = cnc_map.cncmapimagefile_set.select_related().count()
+    response = client_user.post(
+        "/maps/img/",
+        {"file": file_map_image, "cnc_map_id": str(cnc_map.id)},
+        format="multipart",
+        content_type=None,
+    )
+
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.data["message"] == "Map type does not support custom preview images"
+    assert response.data["code"] == FileUploadApiCodes.UNSUPPORTED
+
+    assert cnc_map.cncmapimagefile_set.select_related().count() == original_image_count
+
+
+def test_map_image_upload__user_is_banned(create_cnc_map, file_map_image, client_banned):
+    """Test that map image uploads fail for banned users."""
+    cnc_map = create_cnc_map(user_id=client_banned.kirovy_user.id)
+    original_image_count = cnc_map.cncmapimagefile_set.select_related().count()
+    response = client_banned.post(
+        "/maps/img/",
+        {"file": file_map_image, "cnc_map_id": str(cnc_map.id)},
+        format="multipart",
+        content_type=None,
+    )
+
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+    assert cnc_map.cncmapimagefile_set.select_related().count() == original_image_count
+
+
+def test_map_image_upload__map_is_banned(create_cnc_map, file_map_image, client_user):
+    """Test that map image uploads fail for banned maps."""
+    cnc_map = create_cnc_map(user_id=client_user.kirovy_user.id, is_banned=True)
+    original_image_count = cnc_map.cncmapimagefile_set.select_related().count()
+    response = client_user.post(
+        "/maps/img/",
+        {"file": file_map_image, "cnc_map_id": str(cnc_map.id)},
+        format="multipart",
+        content_type=None,
+    )
+
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+    assert cnc_map.cncmapimagefile_set.select_related().count() == original_image_count