Usability enhancements and hardening for Django models #11725 #11726

jacobtylerwalls · 2025-01-10T22:11:49Z

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

I found some usability improvements to make to our Django models while working on #11596, and I wanted to discuss them separately.

Issues Solved

Closes #11725

Checklist

I targeted one of these branches:
- dev/8.0.x (under development): features, bugfixes not covered below
- dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
- dev/6.2.x (extended support): major security issues, data loss issues
I added a changelog in arches/releases
I submitted a PR to arches-docs (if appropriate)
Unit tests pass locally with my changes
I added tests that prove my fix is effective or that my feature works
My test fails on the target branch

Ticket Background

Sponsored by: Getty Conservation Institute
Found by: @jacobtylerwalls
Tested by: @jacobtylerwalls

Further comments

See commit messages for descriptions

jacobtylerwalls · 2025-01-10T22:12:15Z

arches/app/models/migrations/11725_add_blank_and_related_names.py

+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):


This migration does not produce any SQL

jacobtylerwalls · 2025-01-10T22:13:43Z

arches/app/models/migrations/11725_make_tiledata_not_nullable.py

+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):


This SQL looks like:

BEGIN; -- -- Alter field data on tilemodel -- ALTER TABLE "tiles" ALTER COLUMN "tiledata" SET DEFAULT '{}'::jsonb; UPDATE "tiles" SET "tiledata" = '{}'::jsonb WHERE "tiledata" IS NULL; SET CONSTRAINTS ALL IMMEDIATE; ALTER TABLE "tiles" ALTER COLUMN "tiledata" SET NOT NULL; ALTER TABLE "tiles" ALTER COLUMN "tiledata" DROP DEFAULT; COMMIT;

A db_default on this field didn't seem of much value, but I will circle back to #10958 to get moving on db defaults for primary key fields.

This avoids having two representations of no data--None and {}.

We could discuss whether to do the same thing for provisionaledits.

Where this actually came up in practice is that while dev'ing #11596 I occasionally created corrupt tiles with data = None, and this caused failures all over where we depend on calling .data.keys().

Clarified over slack with Cyrus, repeating here: this doesn't represent an intent to save any tiles with {} in the data column, this is just a something-better-than-nothing way to get null=False on this column, and then we can we add some other layer of protection against {} later, e.g. as a check constraint.

jacobtylerwalls · 2025-01-10T22:15:25Z

arches/app/models/models.py

@@ -99,11 +81,11 @@ def __init__(self, *args, **kwargs):
        if isinstance(self.cardid, str):
            self.cardid = uuid.UUID(self.cardid)

-    def save(self, *args, **kwargs):
+    def save(self, **kwargs):


Positional arguments to save() were deprecated in 5.1 and will raise errors in 6.

jacobtylerwalls · 2025-01-10T22:15:39Z

arches/app/models/models.py

        if self.pk == self.source_identifier_id:
            self.source_identifier_id = None
            add_to_update_fields(kwargs, "source_identifier_id")
-        super(CardModel, self).save()
+        super(CardModel, self).save(**kwargs)


Noticed we were swallowing **kwargs in several places.

jacobtylerwalls · 2025-01-10T22:16:03Z

arches/app/models/models.py

+        related_name="children",
+        related_query_name="child",


Alternatively if we prefer the verbosity we could do child_nodegroups and child_nodegroup.

jacobtylerwalls · 2025-01-10T22:16:43Z

arches/app/models/models.py

@@ -1209,8 +1193,10 @@ class Meta:


 class ResourceInstance(models.Model):
-    resourceinstanceid = models.UUIDField(primary_key=True)
-    graph = models.ForeignKey(GraphModel, db_column="graphid", on_delete=models.CASCADE)
+    resourceinstanceid = models.UUIDField(primary_key=True, blank=True)


blank=True lets us use built-in Django model validation without tripping over fields we expect a client to leave blank and have generated on the backend.

jacobtylerwalls · 2025-01-10T22:17:22Z

arches/app/models/models.py

+    class Meta:
+        managed = True
+        db_table = "resource_instances"
+        permissions = (("no_access_to_resourceinstance", "No Access"),)
+
+    def __init__(self, *args, **kwargs):
+        super(ResourceInstance, self).__init__(*args, **kwargs)
+        if not self.resourceinstanceid:
+            self.resourceinstanceid = uuid.uuid4()


I moved this from the bottom of the class because I sort of expect to see it up before the other methods, but if others feel different, happy to shuffle it back.

jacobtylerwalls · 2025-01-10T22:18:30Z

arches/app/models/models.py

+    def __repr__(self):
+        return f"<{self.graph.name}: {self.name} ({self.pk})>"


<Scheme: AAT Entries (b73e741b-46da-496c-8960-55cc1007bec4)>

jacobtylerwalls · 2025-01-10T22:19:14Z

arches/app/models/models.py

+        related_name="children",
+        related_query_name="child",


Same decision point as above (child_tile, child_tiles?) Or childtile for symmetry with parenttile?

Ah, the other one on NodeGroup already merged in #11614. But we can change it if we want to wordsmith a bit more.

jacobtylerwalls · 2025-01-10T22:20:20Z

tests/models/resource_test.py

-        self.assertIn(str(other_resource.pk), str(mock._mock_call_args))
+        # delete_resources() was called with the correct resource id.
+        self.assertEqual(other_resource.pk, mock._mock_call_args[1]["resources"].pk)


Completely unrelated, but I had this lying around and didn't want to forget about it. When I wrote this test the assertion didn't really capture the issue.

Also stop swallowing **kwargs in some cases. Refs django/django#17667

chiatt

Looks good! Thanks for all of your comments @jacobtylerwalls - made the review much easier.

jacobtylerwalls requested a review from chiatt January 10, 2025 22:11

jacobtylerwalls commented Jan 10, 2025

View reviewed changes

jacobtylerwalls force-pushed the jtw/cleanup-models branch from 0d47957 to 8d7a260 Compare January 10, 2025 23:09

jacobtylerwalls added 3 commits January 10, 2025 18:24

Remove positional args from Model.save() overrides

9afa57d

Also stop swallowing **kwargs in some cases. Refs django/django#17667

Mark some fields blank=True where the backend supplies them

704a223

Add repr() and str() for ResourceInstance and TileModel

2b6630d

jacobtylerwalls force-pushed the jtw/cleanup-models branch from 8d7a260 to 39ec076 Compare January 10, 2025 23:24

jacobtylerwalls added 4 commits January 10, 2025 18:31

Add related_name and related_query_name to TileModel

7c76a8a

Miscellaneous cleanups

d289ee9

Make no-op migration re #11725

4cf845e

Make TileModel.data not nullable

774e857

jacobtylerwalls force-pushed the jtw/cleanup-models branch from 39ec076 to 774e857 Compare January 10, 2025 23:31

jacobtylerwalls assigned chiatt Jan 22, 2025

chiatt approved these changes Jan 23, 2025

View reviewed changes

jacobtylerwalls merged commit 3714ef9 into dev/8.0.x Jan 27, 2025
7 checks passed

jacobtylerwalls deleted the jtw/cleanup-models branch January 27, 2025 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Usability enhancements and hardening for Django models #11725 #11726

Usability enhancements and hardening for Django models #11725 #11726

jacobtylerwalls commented Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 27, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

jacobtylerwalls Jan 10, 2025

chiatt left a comment

		from django.db import migrations, models


		class Migration(migrations.Migration):

		def __repr__(self):
		return f"<{self.graph.name}: {self.name} ({self.pk})>"

Usability enhancements and hardening for Django models #11725 #11726

Usability enhancements and hardening for Django models #11725 #11726

Conversation

jacobtylerwalls commented Jan 10, 2025

Types of changes

Description of Change

Issues Solved

Checklist

Ticket Background

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiatt left a comment

Choose a reason for hiding this comment