Watermark exception #3019

akhilsrivatsa · 2023-09-12T18:28:56Z

Fixes

Fixes #1234 by @obulat

Description

Testing Instructions

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

akhilsrivatsa · 2023-09-13T15:49:17Z

@obulat @krysal

obulat

Thank you for your contribution, @akhilsrivatsa, it's a great start. We would also probably need a test for this change.

To help the reviewers, please fill in the PR description and the testing instructions. You can see what we usually write there in other PRs.

api/api/utils/watermark.py

obulat · 2023-09-19T11:47:54Z

@akhilsrivatsa, the CI check logs show the following errors that need to be fixed:

api/api/utils/watermark.py:27:34: F821 Undefined name `APIException`
api/api/utils/watermark.py:28:19: F821 Undefined name `status`
api/api/utils/watermark.py:29:89: E501 Line too long (89 > 88 characters)
Found 3 errors.

You can also lint the code locally using just lint.

akhilsrivatsa · 2023-09-19T14:56:11Z

@obulat I'm working on fixing the bugs

krysal · 2023-09-19T15:34:07Z

@akhilsrivatsa I converted your PR to a draft by the moment, let us know when it is ready!

akhilsrivatsa · 2023-09-21T04:34:43Z

@obulat I've fixed the issues. There's some check that's still failing in CI + CD/ lint files. I'm not sure why.

obulat

There is another exception that should be handled here, UnidentifiedImageError. It is often caused by SVG files because SVG is not supported by Pillow.

Here's the patch of changes that can be added to fix it:

diff --git a/api/api/utils/watermark.py b/api/api/utils/watermark.py
--- a/api/api/utils/watermark.py	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/api/api/utils/watermark.py	(date 1695537703291)
@@ -7,7 +7,8 @@
 from django.conf import settings
 
 import requests
-from PIL import Image, ImageDraw, ImageFont
+from PIL import Image, ImageDraw, ImageFont, UnidentifiedImageError
+from requests import RequestException
 from sentry_sdk import capture_exception
 
 from rest_framework import status
@@ -183,10 +184,10 @@
         response.raise_for_status()
         img_bytes = BytesIO(response.content)
         img = Image.open(img_bytes)
-    except requests.exceptions.RequestException as e:
+    except (RequestException, UnidentifiedImageError) as e:
         capture_exception(e)
         logger.error(f"Error loading image data: {e}")
-        raise UpstreamWatermarkException(f"Error: {e}")
+        raise UpstreamWatermarkException(f"{e}")
 
     return img, img.getexif()

We also should add tests, at least for the 404 response and for images that raise the UnidentifiedImageError. Here's a patch of tests that could be used:

diff --git a/api/test/unit/views/test_image_views.py b/api/test/unit/views/test_image_views.py
--- a/api/test/unit/views/test_image_views.py	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/api/test/unit/views/test_image_views.py	(date 1695537748677)
@@ -2,6 +2,8 @@
 from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
+from PIL import UnidentifiedImageError
+
 from test.factory.models.image import ImageFactory
 from unittest.mock import ANY, patch
 
@@ -79,3 +81,28 @@
         thumb_call.return_value = mock_response
         api_client.get(f"/v1/images/{image.identifier}/thumb/")
     thumb_call.assert_called_once_with(ANY, image, expected_thumb_url)
+
+
+@pytest.mark.django_db
+def test_watermark_raises_424_for_invalid_image(api_client):
+    image = ImageFactory.create()
+    expected_error_message = "cannot identify image file <_io.BytesIO object at 0xffff86d8fec0>"
+
+    with patch("PIL.Image.open") as mock_open:
+        mock_open.side_effect = UnidentifiedImageError(expected_error_message)
+        res = api_client.get(f"/v1/images/{image.identifier}/watermark/")
+    assert res.status_code == 424
+    assert res.data["detail"] == expected_error_message
+
+@pytest.mark.django_db
+def test_watermark_raises_424_for_404_image(api_client):
+    image = ImageFactory.create()
+
+    with patch("requests.get") as mock_get:
+        mock_get.return_value = Response()
+        mock_get.return_value.status_code = 404
+        mock_get.return_value.url = image.url
+        mock_get.return_value.reason = "Not Found"
+        res = api_client.get(f"/v1/images/{image.identifier}/watermark/")
+    assert res.status_code == 424
+    assert res.data["detail"] == f"404 Client Error: Not Found for url: {image.url}"

To test the changes locally, you can also update the sample_image.csv to replace the URLs for the first two images, as in this diff:

diff --git a/sample_data/sample_image.csv b/sample_data/sample_image.csv
--- a/sample_data/sample_image.csv	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/sample_data/sample_image.csv	(date 1695535546283)
@@ -1,6 +1,6 @@
 identifier,created_on,updated_on,ingestion_type,provider,source,foreign_identifier,foreign_landing_url,url,thumbnail,width,height,filesize,license,license_version,creator,creator_url,title,meta_data,tags,watermarked,last_synced_with_source,removed_from_source,filetype,category,standardized_popularity
-0e3315c5-3328-4a99-80ab-567ac32f685f,2022-12-21 17:29:54.000000+00,2022-12-21 17:29:54.000000+00,provider_api,flickr,flickr,51745822704,https://www.flickr.com/photos/54633257@N04/51745822704,https://live.staticflickr.com/65535/51745822704_ae97226e20_b.jpg,https://live.staticflickr.com/65535/51745822704_ae97226e20_m.jpg,433,1024,97150,by-nc-sa,2.0,mexicofist3,https://www.flickr.com/photos/54633257@N04,my dress is not very short,"{""views"": ""5991"", ""pub_date"": ""1639443671"", ""date_taken"": ""2021-07-20 18:19:25"", ""description"": ""dressed"", ""license_url"": ""https://creativecommons.org/licenses/by-nc-sa/2.0/"", ""raw_license_url"": null}",,f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
-b840de61-fb9d-4ec5-9572-8d778875869f,2022-03-08 14:32:25.000000+00,2022-03-08 14:32:25.000000+00,provider_api,flickr,flickr,51745349583,https://www.flickr.com/photos/129684398@N07/51745349583,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_b.jpg,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_m.jpg,1024,713,171956,by-nc,2.0,clairetresse,https://www.flickr.com/photos/129684398@N07,Barranco de las bodegas,"{""views"": ""4156"", ""pub_date"": ""1639440673"", ""date_taken"": ""2021-10-28 10:54:43"", ""description"": ""Descente dans un des Barranco, descente prudente parce que ça glisse. L’érosion des sols est extrêmement puissante, les reliefs sont sculptés par les forces de la nature telles que le vente les pluies . Descent in one of the Barranco, careful descent because it slips. The erosion of the soil is extremely powerful, the reliefs are sculpted by the forces of nature such as the rains."", ""license_url"": ""https://creativecommons.org/licenses/by-nc/2.0/"", ""raw_license_url"": null}","[{""name"": ""bardenas"", ""provider"": ""flickr""}, {""name"": ""barranco"", ""provider"": ""flickr""}, {""name"": ""desert"", ""provider"": ""flickr""}, {""name"": ""espagne"", ""provider"": ""flickr""}, {""name"": ""glaise"", ""provider"": ""flickr""}, {""name"": ""navarre"", ""provider"": ""flickr""}, {""name"": ""pyrennees"", ""provider"": ""flickr""}, {""name"": ""ravin"", ""provider"": ""flickr""}, {""name"": ""roches"", ""provider"": ""flickr""}, {""name"": ""water"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
+0e3315c5-3328-4a99-80ab-567ac32f685f,2022-12-21 17:29:54.000000+00,2022-12-21 17:29:54.000000+00,provider_api,flickr,flickr,51745822704,https://www.flickr.com/photos/54633257@N04/51745822704,https://live.staticflickr.com/2914/14425817605_87df10788d_b.jpg,https://live.staticflickr.com/65535/51745822704_ae97226e20_m.jpg,433,1024,97150,by-nc-sa,2.0,mexicofist3,https://www.flickr.com/photos/54633257@N04,my dress is not very short,"{""views"": ""5991"", ""pub_date"": ""1639443671"", ""date_taken"": ""2021-07-20 18:19:25"", ""description"": ""dressed"", ""license_url"": ""https://creativecommons.org/licenses/by-nc-sa/2.0/"", ""raw_license_url"": null}",,f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
+b840de61-fb9d-4ec5-9572-8d778875869f,2022-03-08 14:32:25.000000+00,2022-03-08 14:32:25.000000+00,provider_api,flickr,flickr,51745349583,https://www.flickr.com/photos/129684398@N07/51745349583,https://raw.githubusercontent.com/WordPress/openverse/main/frontend/src/assets/svg/raw/caret-down.svg,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_m.jpg,1024,713,171956,by-nc,2.0,clairetresse,https://www.flickr.com/photos/129684398@N07,Barranco de las bodegas,"{""views"": ""4156"", ""pub_date"": ""1639440673"", ""date_taken"": ""2021-10-28 10:54:43"", ""description"": ""Descente dans un des Barranco, descente prudente parce que ça glisse. L’érosion des sols est extrêmement puissante, les reliefs sont sculptés par les forces de la nature telles que le vente les pluies . Descent in one of the Barranco, careful descent because it slips. The erosion of the soil is extremely powerful, the reliefs are sculpted by the forces of nature such as the rains."", ""license_url"": ""https://creativecommons.org/licenses/by-nc/2.0/"", ""raw_license_url"": null}","[{""name"": ""bardenas"", ""provider"": ""flickr""}, {""name"": ""barranco"", ""provider"": ""flickr""}, {""name"": ""desert"", ""provider"": ""flickr""}, {""name"": ""espagne"", ""provider"": ""flickr""}, {""name"": ""glaise"", ""provider"": ""flickr""}, {""name"": ""navarre"", ""provider"": ""flickr""}, {""name"": ""pyrennees"", ""provider"": ""flickr""}, {""name"": ""ravin"", ""provider"": ""flickr""}, {""name"": ""roches"", ""provider"": ""flickr""}, {""name"": ""water"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
 aeba0547-61da-42ee-b561-27c8fc817d5a,2022-07-16 05:51:03.000000+00,2022-07-16 05:51:03.000000+00,provider_api,flickr,flickr,51745788239,https://www.flickr.com/photos/88123769@N02/51745788239,https://live.staticflickr.com/65535/51745788239_b645ce02fe_b.jpg,https://live.staticflickr.com/65535/51745788239_b645ce02fe_m.jpg,1024,602,281512,pdm,1.0,Bernard Spragg,https://www.flickr.com/photos/88123769@N02,Alone on the prairie.,"{""views"": ""1779"", ""pub_date"": ""1639441826"", ""date_taken"": ""2017-09-26 11:39:16"", ""description"": ""The Canadian Prairies (usually referred to as simply the Prairies in Canada) is a region in Western Canada. It includes the Canadian portion of the Great Plains and the Prairie Provinces, namely Alberta, Saskatchewan, and Manitoba.These provinces are partially covered by grasslands, plains, and lowlands, mostly in the southern regions."", ""license_url"": ""https://creativecommons.org/publicdomain/mark/1.0/"", ""raw_license_url"": null}","[{""name"": ""alberta"", ""provider"": ""flickr""}, {""name"": ""alone"", ""provider"": ""flickr""}, {""name"": ""canada"", ""provider"": ""flickr""}, {""name"": ""evening"", ""provider"": ""flickr""}, {""name"": ""house"", ""provider"": ""flickr""}, {""name"": ""landscape"", ""provider"": ""flickr""}, {""name"": ""lumixfz1000"", ""provider"": ""flickr""}, {""name"": ""old"", ""provider"": ""flickr""}, {""name"": ""outside"", ""provider"": ""flickr""}, {""name"": ""prairie"", ""provider"": ""flickr""}, {""name"": ""rural"", ""provider"": ""flickr""}, {""name"": ""scenery"", ""provider"": ""flickr""}, {""name"": ""sky"", ""provider"": ""flickr""}, {""name"": ""travel"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
 3c98150c-51a8-4175-a47f-acef10e784f7,2022-06-10 09:14:13.000000+00,2022-06-10 09:14:13.000000+00,provider_api,flickr,flickr,51747927224,https://www.flickr.com/photos/151325871@N07/51747927224,https://live.staticflickr.com/65535/51747927224_3ca7ac2e93.jpg,https://live.staticflickr.com/65535/51747927224_3ca7ac2e93_m.jpg,318,500,53633,cc0,1.0,lyndawaybi3,https://www.flickr.com/photos/151325871@N07,Naughty Little Elf,"{""views"": ""1342"", ""pub_date"": ""1639526583"", ""date_taken"": ""2021-12-14 16:02:55"", ""license_url"": ""https://creativecommons.org/publicdomain/zero/1.0/""}","[{""name"": ""babe"", ""provider"": ""flickr""}, {""name"": ""bi"", ""provider"": ""flickr""}, {""name"": ""brunette"", ""provider"": ""flickr""}, {""name"": ""chick"", ""provider"": ""flickr""}, {""name"": ""christmas"", ""provider"": ""flickr""}, {""name"": ""dress"", ""provider"": ""flickr""}, {""name"": ""elf"", ""provider"": ""flickr""}, {""name"": ""great"", ""provider"": ""flickr""}, {""name"": ""hot"", ""provider"": ""flickr""}, {""name"": ""hotwife"", ""provider"": ""flickr""}, {""name"": ""leggings"", ""provider"": ""flickr""}, {""name"": ""legs"", ""provider"": ""flickr""}, {""name"": ""lynda"", ""provider"": ""flickr""}, {""name"": ""married"", ""provider"": ""flickr""}, {""name"": ""milf"", ""provider"": ""flickr""}, {""name"": ""mini"", ""provider"": ""flickr""}, {""name"": ""mom"", ""provider"": ""flickr""}, {""name"": ""nylons"", ""provider"": ""flickr""}, {""name"": ""panyhose"", ""provider"": ""flickr""}, {""name"": ""season"", ""provider"": ""flickr""}, {""name"": ""sexy"", ""provider"": ""flickr""}, {""name"": ""short"", ""provider"": ""flickr""}, {""name"": ""skirt"", ""provider"": ""flickr""}, {""name"": ""stockings"", ""provider"": ""flickr""}, {""name"": ""sweater"", ""provider"": ""flickr""}, {""name"": ""wife"", ""provider"": ""flickr""}, {""name"": ""young"", ""provider"": ""flickr""}]",f,2021-12-15 22:19:02.971943+00,f,jpg,photograph,
 cdbd3bf6-1745-45bb-b399-61ee149cd58a,2022-12-28 15:41:34.000000+00,2022-12-28 15:41:34.000000+00,provider_api,flickr,flickr,51745389858,https://www.flickr.com/photos/126744325@N07/51745389858,https://live.staticflickr.com/65535/51745389858_c10358e1a3_b.jpg,https://live.staticflickr.com/65535/51745389858_c10358e1a3_m.jpg,1024,683,157497,by,2.0,Kristoffer Trolle,https://www.flickr.com/photos/126744325@N07,Train area in Copenhagen South / Tog område i Syd København,"{""views"": ""1337"", ""pub_date"": ""1639441947"", ""date_taken"": ""2021-07-14 23:49:46"", ""description"": ""This old train area in Copenhagen South will soon be transformed into a residential area. I love to go there and take photos. I used a Tiffen Black Pro Mist 1/4 filter for this photo, it gives that diffused highlights look, read more about it on my blog here . The photo is Creative Commons license: Use it for free. Keywords: train, tog, DSB, område, syd, København, south, Copenhagen, Danmark, Denmark, Fujifilm X-H1, Fujifilm XF 35mm f2 R WR, Tiffen Black Pro-Mist 1/4 filter"", ""license_url"": ""https://creativecommons.org/licenses/by/2.0/"", ""raw_license_url"": null}","[{""name"": ""copenhagen"", ""provider"": ""flickr""}, {""name"": ""danmark"", ""provider"": ""flickr""}, {""name"": ""denmark"", ""provider"": ""flickr""}, {""name"": ""dsb"", ""provider"": ""flickr""}, {""name"": ""fujifilmxf35mmf2rwr"", ""provider"": ""flickr""}, {""name"": ""fujifilmxh1"", ""provider"": ""flickr""}, {""name"": ""københavn"", ""provider"": ""flickr""}, {""name"": ""område"", ""provider"": ""flickr""}, {""name"": ""south"", ""provider"": ""flickr""}, {""name"": ""syd"", ""provider"": ""flickr""}, {""name"": ""tiffenblackpromist14filter"", ""provider"": ""flickr""}, {""name"": ""tog"", ""provider"": ""flickr""}, {""name"": ""train"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,

You should run just recreate, just init after these changes to make sure the new sample data is used.
Then, you would be able to see fixed responses for the following urls:
http://localhost:50280/v1/images/0e3315c5-3328-4a99-80ab-567ac32f685f/watermark/
http://localhost:50280/v1/images/b840de61-fb9d-4ec5-9572-8d778875869f/watermark/

akhilsrivatsa · 2023-09-28T05:01:37Z

@obulat I'll work on fixing the UnidentifiedImageError.

obulat · 2023-10-06T13:21:33Z

@akhilsrivatsa, do you plan to work on this PR? It's Okay if you don't - but please let us know if so, or if there's anything we can help you with to get this PR finished.

krysal

Hey @akhilsrivatsa! This is looking quite close to a complete solution. Can you still work on this? I left some comments with suggestions.

api/api/utils/watermark.py

api/test/unit/views/test_image_views.py

Co-authored-by: Krystle Salazar <github@krysal.co>

This reverts commit 11518b6.

dhruvkb

The changes LGTM; thanks for the contribution @akhilsrivatsa!

I would prefer that exceptions like UpstreamWatermarkException be defined in a standalone file like exceptions.py but this works and resolves a long-standing issue.

Changes have been implemented.

akhilsrivatsa requested a review from a team as a code owner September 12, 2023 18:28

akhilsrivatsa requested review from krysal and stacimc September 12, 2023 18:28

openverse-bot added 🟩 priority: low Low priority and doesn't need to be rushed 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository 🧱 stack: api Related to the Django API labels Sep 12, 2023

obulat reviewed Sep 18, 2023

View reviewed changes

api/api/utils/watermark.py Outdated Show resolved Hide resolved

krysal marked this pull request as draft September 19, 2023 15:32

obulat requested changes Sep 24, 2023

View reviewed changes

obulat force-pushed the watermark-exception branch from d866e26 to 5d7b3bb Compare October 10, 2023 14:08

krysal previously requested changes Oct 19, 2023

View reviewed changes

api/api/utils/watermark.py Outdated Show resolved Hide resolved

api/test/unit/views/test_image_views.py Show resolved Hide resolved

obulat force-pushed the watermark-exception branch from 16c920e to f1ad8fa Compare October 25, 2023 15:27

akhilsrivatsa and others added 9 commits October 27, 2023 14:17

Added UpStreamWaterMark Excpetion

636f49d

Added Watermark exception

bd0b0a3

Update api/api/utils/watermark.py

a786fa5

Fix errros

4553edf

Watermark bug fix

f9637f4

Fix Line length

7591328

Handle UnidentifiedImageError

a980b93

Add tests

235d66f

Update api/test/unit/views/test_image_views.py

1dde891

Co-authored-by: Krystle Salazar <github@krysal.co>

obulat added 2 commits October 27, 2023 14:18

Revert "Update api/test/unit/views/test_image_views.py"

a0f20af

This reverts commit 11518b6.

Add suggestion from code review

21b38e8

obulat force-pushed the watermark-exception branch from f1ad8fa to 21b38e8 Compare October 27, 2023 11:20

obulat requested review from obulat and krysal October 27, 2023 11:27

obulat marked this pull request as ready for review October 27, 2023 11:27

obulat approved these changes Oct 27, 2023

View reviewed changes

obulat requested a review from dhruvkb November 1, 2023 15:39

dhruvkb approved these changes Nov 3, 2023

View reviewed changes

dhruvkb merged commit b2a3970 into WordPress:main Nov 3, 2023
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watermark exception #3019

Watermark exception #3019

akhilsrivatsa commented Sep 12, 2023

akhilsrivatsa commented Sep 13, 2023

obulat left a comment

obulat commented Sep 19, 2023

akhilsrivatsa commented Sep 19, 2023

krysal commented Sep 19, 2023

akhilsrivatsa commented Sep 21, 2023

obulat left a comment

akhilsrivatsa commented Sep 28, 2023

obulat commented Oct 6, 2023

krysal left a comment

dhruvkb left a comment

Watermark exception #3019

Watermark exception #3019

Conversation

akhilsrivatsa commented Sep 12, 2023

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

akhilsrivatsa commented Sep 13, 2023

obulat left a comment

Choose a reason for hiding this comment

obulat commented Sep 19, 2023

akhilsrivatsa commented Sep 19, 2023

krysal commented Sep 19, 2023

akhilsrivatsa commented Sep 21, 2023

obulat left a comment

Choose a reason for hiding this comment

akhilsrivatsa commented Sep 28, 2023

obulat commented Oct 6, 2023

krysal left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment