feat: more cleanups and validation for ObjectTag

openedx_tagging/core/tagging/api.py

@@ -174,7 +174,7 @@ def tag_object(
     tags: list[str],
     object_id: str,
     ObjectTagClass: type[ObjectTag] = ObjectTag,
-) -> list[ObjectTag]:
+) -> None:
     """
     Replaces the existing ObjectTag entries for the given taxonomy + object_id
     with the given list of tags.
@@ -244,18 +244,18 @@ def _check_new_tag_count(new_tag_count: int) -> None:
                     updated_tags.append(object_tag)
             else:
                 # We are newly applying this tag:
-                object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag, _value=tag.value)
+                object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag)
                 updated_tags.append(object_tag)
 
     # Save all updated tags at once to avoid partial updates
     with transaction.atomic():
-        for object_tag in updated_tags:
-            object_tag.save()
-        # ...and delete any omitted existing tags
+        # delete any omitted existing tags. We do this first to reduce chances of UNIQUE constraint edge cases
         for old_tag in current_tags:
             old_tag.delete()
-
-    return updated_tags
+        # add the new tags:
+        for object_tag in updated_tags:
+            object_tag.full_clean()  # Run validation
+            object_tag.save()
 
 
 # TODO: return tags from closed taxonomies as well as the count of how many times each is used.

openedx_tagging/core/tagging/migrations/0010_cleanups.py

@@ -0,0 +1,31 @@
+# Generated by Django 3.2.19 on 2023-09-29 16:59
+
+from django.db import migrations, models
+import django.db.models.expressions
+import openedx_learning.lib.fields
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oel_tagging', '0009_alter_objecttag_object_id'),
+    ]
+
+    operations = [
+        migrations.DeleteModel(
+            name='ModelObjectTag',
+        ),
+        migrations.DeleteModel(
+            name='UserModelObjectTag',
+        ),
+        migrations.AlterUniqueTogether(
+            name='objecttag',
+            unique_together={('object_id', 'taxonomy', 'tag_id'), ('object_id', 'taxonomy', '_value')},
+        ),
+        # ObjectTag.Tag can be blank
+        migrations.AlterField(
+            model_name='objecttag',
+            name='tag',
+            field=models.ForeignKey(blank=True, default=None, help_text="Tag associated with this object tag. Provides the tag's 'value' if set.", null=True, on_delete=django.db.models.deletion.SET_NULL, to='oel_tagging.tag'),
+        ),
+    ]

openedx_tagging/core/tagging/models/base.py

@@ -6,6 +6,7 @@
 import logging
 from typing import List
 
+from django.core.exceptions import ValidationError
 from django.db import models
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
@@ -59,7 +60,7 @@ class Tag(models.Model):
     )
     external_id = case_insensitive_char_field(
         max_length=255,
-        null=True,
+        null=True,  # To allow multiple values with our UNIQUE constraint, we need to use NULL values here instead of ""
         blank=True,
         help_text=_(
             "Used to link an Open edX Tag with a tag in an externally-defined taxonomy."
@@ -359,7 +360,7 @@ def validate_value(self, value: str) -> bool:
         """
         self.check_casted()
         if self.allow_free_text:
-            return True
+            return value != "" and isinstance(value, str)
         return self.tag_set.filter(value__iexact=value).exists()
 
     def tag_for_value(self, value: str) -> Tag:
@@ -435,23 +436,22 @@ class ObjectTag(models.Model):
     )
     tag = models.ForeignKey(
         Tag,
-        null=True,
+        null=True,  # NULL in the case of free text taxonomies or when the Tag gets deleted.
+        blank=True,
         default=None,
         on_delete=models.SET_NULL,
         help_text=_(
             "Tag associated with this object tag. Provides the tag's 'value' if set."
         ),
     )
     _name = case_insensitive_char_field(
-        null=False,
         max_length=255,
         help_text=_(
             "User-facing label used for this tag, stored in case taxonomy is (or becomes) null."
             " If the taxonomy field is set, then taxonomy.name takes precedence over this field."
         ),
     )
     _value = case_insensitive_char_field(
-        null=False,
         max_length=500,
         help_text=_(
             "User-facing value used for this tag, stored in case tag is null, e.g if taxonomy is free text, or if it"
@@ -465,7 +465,19 @@ class Meta:
             models.Index(fields=["taxonomy", "object_id"]),
             models.Index(fields=["taxonomy", "_value"]),
         ]
-        unique_together = ("taxonomy", "_value", "object_id")
+        unique_together = [
+            ("object_id", "taxonomy", "tag_id"),
+            ("object_id", "taxonomy", "_value"),
+        ]
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        if not self.pk:  # This is a new instance:
+            # Set _name and _value automatically on creation, if they weren't set:
+            if self.taxonomy and not self._name:
+                self._name = self.taxonomy.name
+            if not self._value and self.tag:
+                self._value = self.tag.value
 
     def __repr__(self):
         """
@@ -512,19 +524,26 @@ def value(self, value: str):
         Stores to the _value field.
         """
         self._value = value
-
-    def is_valid(self) -> bool:
+
+    @property
+    def is_deleted(self) -> bool:
         """
-        Returns True if this ObjectTag represents a valid taxonomy tag.
-
-        A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy.
+        Has this Tag been deleted from the Taxonomy? If so, we preserve this
+        ObjecTag in the DB but it shouldn't be shown to the user.
         """
+        return self.taxonomy is None or (self.tag is None and not self.taxonomy.allow_free_text)
+
+    def clean(self):
         if self.tag:
-            return True  # If the Tag still exists, this is a valid value in the taxonomy.
-        elif self.taxonomy:
-            # If this is not a free text value and it's no longer linked to a Tag, it's invalid.
-            # Running self.resync() _might_ fix this though.
-            return self.taxonomy.allow_free_text
+            if self.tag.taxonomy_id != self.taxonomy_id:
+                raise ValidationError("ObjectTag's Taxonomy does not match Tag taxonomy")
+            elif self.tag.value != self._value:
+                raise ValidationError("ObjectTag's _value is out of sync with Tag.value")
+        else:
+            # Note: self.taxonomy and/or self.tag may be NULL which is OK, because it means the Tag/Taxonomy
+            # was deleted, but we still preserve this _value here in case the Taxonomy or Tag get re-created in future.
+            if self._value == "":
+                raise ValidationError("Invalid _value - empty string")
 
     def get_lineage(self) -> Lineage:
         """

openedx_tagging/core/tagging/rest_api/v1/serializers.py

@@ -53,7 +53,8 @@ class Meta:
             "name",
             "value",
             "taxonomy_id",
-            "is_valid",
+            # If the Tag or Taxonomy has been deleted, this ObjectTag shouldn't be shown to users.
+            "is_deleted",
         ]