Add new SensitiveMedia and MediaReport models #3510

obulat · 2023-12-11T10:44:54Z

Fixes

Description

This PR adds the models as described in the Steps 1 and 2 of the Implementation Plan/ API

@WordPress/openverse-api , I need help figuring out why these changes cause one sensitive table db query per result, both for search and for the single result requests:

Sample search query

``` E 1. SELECT "image"."id", "image"."created_on", "image"."updated_on", "image"."identifier", "image"."foreign_identifier", "image"."title", "image"."foreign_landing_url", "image"."creator", "image"."creator_url", "image"."thumbnail", "image"."provider", "image"."url", "image"."filesize", "image"."filetype", "image"."watermarked", "image"."license", "image"."license_version", "image"."source", "image"."last_synced_with_source", "image"."removed_from_source", "image"."view_count", "image"."tags", "image"."category", "image"."meta_data", "image"."width", "image"."height", "api_matureimage"."created_on", "api_matureimage"."identifier" FROM "image" LEFT OUTER JOIN "api_matureimage" ON ("image"."identifier" = "api_matureimage"."identifier") WHERE (NOT ("image"."provider" IN (SELECT U0."provider_identifier" FROM "content_provider" U0 WHERE U0."filter_content") AND "image"."provider" IS NOT NULL) AND "image"."identifier" IN ('6b7d6870ab734fe7b187d5509f408b48'::uuid, 'f890597b52c8432fbd86a8f4362182af'::uuid, '5aa22382cc77481eafb88693b304a44e'::uuid, '676777e3bb9c432eb8c80ce27f6a639b'::uuid, '4657177f42764b379e9cfc2e88b6b996'::uuid, '32a1ccf579d84eafb974045d93ea769e'::uuid, '69a7b7fda0c9497e8ec7f3a2177298fd'::uuid, 'caefbd828ae145e2b5f5d901778198f4'::uuid, '06f5b2e2f2c546e789e5d7c2fe2330f6'::uuid, '467d9209e90a4f469447d4511ef00bdf'::uuid, '475ead8a9fa4481c999c101b8407bd30'::uuid, 'cb64afe904864da8a5c46714bf72b02d'::uuid, '9c6571bf310740b6baab3a7e170ae301'::uuid, '6af6e4ca356c4c828047a3d0fbede4bb'::uuid, '8ca3fb5ed56747c9abc7e9c4b7374c92'::uuid, 'c2020b32f0c04b20a714a74a3750d8e2'::uuid, 'acabd6bf5a774289a587918dac0eb66b'::uuid, 'ea4edd319f974cdaa02a43d85bce6674'::uuid, '0f5bf191878b45588674f22a210a80b0'::uuid, '798eea993dd74aa5a9a92acfd281a573'::uuid)) ORDER BY "image"."created_on" DESC E 2. SELECT "api_sensitiveimage"."created_on", "api_sensitiveimage"."identifier" FROM "api_sensitiveimage" WHERE "api_sensitiveimage"."identifier" = '6b7d6870ab734fe7b187d5509f408b48'::uuid LIMIT 21 ```

Testing Instructions

Checkout main branch and mark several items asmature in the admin (http://localhost:50280/admin, user deploy, password deploy). I used a search for "Couple Food Photo" and marked a couple of results as sensitive.
Checkout this branch and run just api/dj migrate. This should create the new db tables.
Go to admin and check that you can still see the old reports.
Create some new reports.

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.

Signed-off-by: Olga Bulat <obulat@gmail.com>

github-actions · 2023-12-11T11:15:43Z

This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced.

sarayourfriend

@obulat Can you recreate the migrations to see if it can be in a single migration rather than two? It is hard to keep track of what is actually changing between the two migrations.

I also think it might be easier to reivew this PR if it was split into two separate PRs: first to just add the new models/update references; second to actually implement the new writing to the new models.

Seeing how complex it is to juggle the table rename, I wonder if it's worth renaming the table, instead of just using a view to give an alias with the new name. @AetherUnbound @obulat what do y'all think? It would massively simplify this PR and the later data migration work because we'd just need to add the new sensitive-based choices and transform them in place in the same table, rather than copy data between the new tables and juggle the reading/writing changes between the old and new model.

I think the goal here was just to get rid of nsfw from the table name, but an alias would make it possible to use a consistent name when querying the API databases directly without all of this massive amount of complexity.

sarayourfriend · 2023-12-11T20:46:39Z

api/test/factory/models/image.py



-class MatureImageFactory(DjangoModelFactory):
+class SensitiveImageFactory(DjangoModelFactory):


The old factories should probably stay around until we've removed the model entirely. Tests can be updated to use the new factory, but the new factory should create the old model in the create classmethod, so that tests reflect the actual behaviour of these models.

Alternatively, it'd be better to update the tests to create both as needed to reflect the actual business logic (even if it's temporary).

sarayourfriend · 2023-12-11T20:47:20Z

api/api/models/media.py

+    REPORT_CHOICES = [
+        (SENSITIVE, SENSITIVE),
+        (DMCA, DMCA),
+        (OTHER, OTHER),
+    ]


These can't be changed like this, it can cause downtime. After the canary applies the migration to change the model, the old code would try to write "mature" rather than "sensitive".

The temporary Nsfw models that represent the existing tables need to override this (and STATUS_CHOICES) to use mature rather than sensitive.

The new models should use sensitive though. The management command later on will transform the existing table data into the new version.

sarayourfriend · 2023-12-11T20:52:05Z

api/api/models/media.py

        """

        self.clean()

        super().save(*args, **kwargs)

-        if self.status == MATURE_FILTERED:
+        if self.status == SENSITIVE_FILTERED:


Suggested change

if self.status == SENSITIVE_FILTERED:

if self.status in [SENSITIVE_FILTERED, MATURE_FILTERED]:

This can safely handle both without the implementing class needing to override it.

sarayourfriend · 2023-12-11T20:53:53Z

api/api/models/media.py

+    def _update_es(self, is_mature: bool, raise_errors: bool):
+        """
+        Update the Elasticsearch document associated with the given model.
+
+        :param is_mature: whether to mark the media item as mature
+        :param raise_errors: whether to raise an error if the no media item is found
+        """
+        self._perform_index_update(
+            "update",
+            raise_errors,
+            doc={"mature": is_mature},
+        )


I suppose we can update these variable names as well for the new model, if we want:

Suggested change

def _update_es(self, is_mature: bool, raise_errors: bool):

"""

Update the Elasticsearch document associated with the given model.

:param is_mature: whether to mark the media item as mature

:param raise_errors: whether to raise an error if the no media item is found

"""

self._perform_index_update(

"update",

raise_errors,

doc={"mature": is_mature},

)

def _update_es(self, is_sensitive: bool, raise_errors: bool):

"""

Update the Elasticsearch document associated with the given model.

Note: The Elasticsearch data model still uses `mature` rather than `sensitive`.

:param is_sensitive: whether to mark the media item as sensitive

:param raise_errors: whether to raise an error if the no media item is found

"""

self._perform_index_update(

"update",

raise_errors,

doc={"mature": is_sensitive},

)

sarayourfriend · 2023-12-11T20:58:48Z

api/api/models/media.py

+        primary_key=True,
+        db_constraint=False,
+        db_column="identifier",
+        related_name="sensitive_abstract_media",


The new related name is the reason for the additional query. It should get added to the get_queryset method of the media views: https://github.com//WordPress/openverse/tree/main/api/api/views/image_views.py#L65-L66. Otherwise, whenever sensitive_media is accessed on the model instance Django automatically does a query in the background to populate sensitive_media.

Thank you, @sarayourfriend!

sarayourfriend · 2023-12-11T21:08:18Z

api/api/models/image.py

+        return super().url("images")
+
+
+class NsfwReport(AbstractMediaReport):


I know the IP says to use NsfwReport for this but I think it would be clearer to use NsfwImageReport instead for the temporary name, like we do for the other media-specific models.

obulat · 2023-12-12T07:26:04Z

Closing this PR because we decided that changing the table names is too much work for too little benefit.

krysal · 2023-12-12T14:13:59Z

Closing this PR because we decided that changing the table names is too much work for too little benefit.

Was this decided between you @obulat and @sarayourfriend? If I recall correctly, the team decided to make the table name change with the mature -> sensitive text changes.

Edit: Nevermind. I read the document with the stated reasons, and even though I really don't like the disparity in names, I understand the complexity 👍

Add new SensitiveMedia and MediaReport models

fb07787

Signed-off-by: Olga Bulat <obulat@gmail.com>

openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 💻 aspect: code Concerns the software code in the repository 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Dec 11, 2023

github-actions bot added 🧱 stack: api Related to the Django API migrations Modifications to Django migrations labels Dec 11, 2023

obulat force-pushed the add/sensitive_media-and-media_report-models branch from 77dc679 to fb07787 Compare December 11, 2023 10:48

WordPress deleted a comment from github-actions bot Dec 11, 2023

Add old mature reports to admin

a84b2f6

Signed-off-by: Olga Bulat <obulat@gmail.com>

WordPress deleted a comment from github-actions bot Dec 11, 2023

obulat marked this pull request as ready for review December 11, 2023 11:36

obulat requested a review from a team as a code owner December 11, 2023 11:36

obulat requested review from sarayourfriend and stacimc December 11, 2023 11:36

obulat removed the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Dec 11, 2023

sarayourfriend requested changes Dec 11, 2023

View reviewed changes

obulat closed this Dec 12, 2023

obulat deleted the add/sensitive_media-and-media_report-models branch March 19, 2024 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new SensitiveMedia and MediaReport models #3510

Add new SensitiveMedia and MediaReport models #3510

obulat commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 11, 2023

sarayourfriend left a comment

sarayourfriend Dec 11, 2023

sarayourfriend Dec 11, 2023

sarayourfriend Dec 11, 2023

sarayourfriend Dec 11, 2023

sarayourfriend Dec 11, 2023

obulat Dec 12, 2023

sarayourfriend Dec 11, 2023

obulat commented Dec 12, 2023

krysal commented Dec 12, 2023 •

edited

Loading



		class MatureImageFactory(DjangoModelFactory):
		class SensitiveImageFactory(DjangoModelFactory):

	if self.status == SENSITIVE_FILTERED:
	if self.status in [SENSITIVE_FILTERED, MATURE_FILTERED]:

		return super().url("images")


		class NsfwReport(AbstractMediaReport):

Add new SensitiveMedia and MediaReport models #3510

Add new SensitiveMedia and MediaReport models #3510

Conversation

obulat commented Dec 11, 2023 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

github-actions bot commented Dec 11, 2023

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

obulat Dec 12, 2023

Choose a reason for hiding this comment

sarayourfriend Dec 11, 2023

Choose a reason for hiding this comment

obulat commented Dec 12, 2023

krysal commented Dec 12, 2023 • edited Loading

obulat commented Dec 11, 2023 •

edited

Loading

krysal commented Dec 12, 2023 •

edited

Loading