Hardens editable_future_graph against nodegroup changes #11570 #11609

chrabyrd · 2024-11-08T01:12:15Z

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

This handles source_graph nodegroup creation for when a node is set to is_collector after already being created. It includes general performance/QoL improvements, and has spawned a followup ticket: #11610

Issues Solved

Closes #11570

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

chrabyrd · 2024-11-14T18:35:22Z

re-running all tests to ensure flakiness no longer occurs. 1 successful run isn't enough 😂

johnatawnclementawn

Dog-fooding this for converting string nodes into non-localized string nodes and found that if your graph has a slug, you get an error like this:

File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2577, in update_from_editable_future_graph
    return self.restore_state_from_serialized_graph(
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        serialized_editable_future_graph
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2689, in restore_state_from_serialized_graph
    updated_graph.save()
    ~~~~~~~~~~~~~~~~~~^^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 599, in save
    self.validate()
    ~~~~~~~~~~~~~^^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2328, in validate
    raise GraphValidationError(
    ...<4 lines>...
    )
arches.app.models.graph.GraphValidationError: "Another resource model already uses the slug 'string_example_model'"

…nto 11570-cbyrd-harden-editable_future_graphs-against-nodegroup-changes

chrabyrd · 2024-11-15T03:04:01Z

Dog-fooding this for converting string nodes into non-localized string nodes and found that if your graph has a slug, you get an error like this:

File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2577, in update_from_editable_future_graph
    return self.restore_state_from_serialized_graph(
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        serialized_editable_future_graph
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2689, in restore_state_from_serialized_graph
    updated_graph.save()
    ~~~~~~~~~~~~~~~~~~^^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 599, in save
    self.validate()
    ~~~~~~~~~~~~~^^
  File "/Users/johnathanclementi/Documents/Arches/Projects/rdm/arches/arches/app/models/graph.py", line 2328, in validate
    raise GraphValidationError(
    ...<4 lines>...
    )
arches.app.models.graph.GraphValidationError: "Another resource model already uses the slug 'string_example_model'"

we should chat, I cannot reproduce this error in test or UX. I added a test anyway 👍

tests/models/graph_tests.py

jacobtylerwalls

Love the QoL improvements, and thanks for digging into this many-tentacled issue! A few questions for you.

arches/app/media/js/viewmodels/graph-settings.js

jacobtylerwalls · 2024-11-19T19:39:05Z

arches/app/models/migrations/11570_harden_editble_future_graphs.py

@@ -0,0 +1,38 @@
+# Generated by Django 5.1.2 on 2024-11-13 18:17


filename: edtible -> editable

arches/app/models/models.py

jacobtylerwalls · 2024-11-19T19:58:15Z

arches/app/models/graph.py

+                        serialized_user_permission["content_object"] = (
+                            models.NodeGroup.objects.get(
+                                pk=serialized_user_permission["object_pk"]
+                            )
+                        )


WDYT about prefetching these to avoid one-by-one queries?

jacobtylerwalls · 2024-11-19T20:18:33Z

tests/models/graph_tests.py

+        source_graph = Graph.new(name="TEST RESOURCE", is_resource=True, author="TEST")
+        source_graph.save()


As a newer developer to this part of the code, tests like these help me understand the intended developer interface. Am I supposed to save() after calling new() because new() lacks some functionality? Would be nice to clarify with a comment here if so, or else open a follow-up ticket if the implementations need some attention.

tests/models/graph_tests.py

jacobtylerwalls · 2024-11-19T20:22:23Z

tests/models/graph_tests.py

+
+        nodegroup = models.NodeGroup.objects.create()
+        string_node = models.Node.objects.create(
+            graph=source_graph,


Let's ensure there's a collector/root node:

Suggested change

graph=source_graph,

pk=nodegroup.pk,

graph=source_graph,

tests/models/graph_tests.py

jacobtylerwalls · 2024-11-19T20:40:58Z

arches/app/models/graph.py

+            # ensures any resources that were related to the source graph are not deleted
+            self.pk = uuid.uuid4()
+            self.delete()


The chance of legit data loss from a UUID collision is infinitesimal, but I wonder if we should decompose Graph.delete() into deleteRelatedObjects() so we can call it without calling super().delete() and fiddling with the pk. By not hacking the pk, it will be a little clearer and hardened against other ORM internals (e.g., do we need to also fiddle with ._state.adding and set it False, now that it's a nonexistent object? Would rather not massage that...)

but we also want to delete the graph. I'm unsure decomposing this will do anything for

graph = models.ForeignKey(GraphModel, db_column="graphid", on_delete=models.CASCADE)

on ResourceInstance.

Well from a code read, we're not deleting the graph, and I just checked the queries to be sure. We set the PK to a random value, so that super().delete() will shoot a bunch of blanks, firing off deletes on a bunch of tables that will never match on anything. Since we're not deleting the graph, I was thinking it would be clearer to decompose that out and avoid shooting the blanks:

BEFORE: a6cd3760-199b-4071-bd15-307fb04e53d2 AFTER: de19d77d-4b59-41ae-bdee-aa7669cda961

Notice the deletes are for the random uuid (also why I was mentioning the very very rare risk of collision):

-> super(Graph, self).delete() (Pdb) print(connection.queries) [{'sql': 'SELECT "cards"."cardid" FROM "cards" WHERE "cards"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.003'}, {'sql': 'SELECT "edges"."edgeid" FROM "edges" WHERE "edges"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.002'}, {'sql': 'SELECT "graphs"."graphid" FROM "graphs" WHERE "graphs"."source_identifier" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}, {'sql': 'SELECT "graphs_x_published_graphs"."publicationid" FROM "graphs_x_published_graphs" WHERE "graphs_x_published_graphs"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.002'}, {'sql': 'SELECT "nodes"."nodeid" FROM "nodes" WHERE "nodes"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}, {'sql': 'SELECT "resource_instances"."resourceinstanceid" FROM "resource_instances" WHERE "resource_instances"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}, {'sql': 'DELETE FROM "functions_x_graphs" WHERE "functions_x_graphs"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}, {'sql': 'DELETE FROM "resource_x_resource" WHERE ("resource_x_resource"."resourceinstancefrom_graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid) OR "resource_x_resource"."resourceinstanceto_graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid))', 'time': '0.001'}, {'sql': 'DELETE FROM "graphs_x_mapping_file" WHERE "graphs_x_mapping_file"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}, {'sql': 'DELETE FROM "graphs" WHERE "graphs"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}]

I guess I'm talking about:

{'sql': 'DELETE FROM "graphs" WHERE "graphs"."graphid" IN (\'de19d77d-4b59-41ae-bdee-aa7669cda961\'::uuid)', 'time': '0.001'}

so that the current graph object gets deleted. I know the Graph.delete method manually handles cards, nodes, widgets, nodegroups, and edges in a way that wouldn't be affected by changing the graphid. graph_x_published_graph is interesting, I haven't checked that those are also deleted, but I will. Same with checking for strays with functions_x_graphs

I'm not against the decomposition here, but at some point the graph itself needs to be deleted, so not I'm entirely sure that the decomposition will handle what we need it to.

And you're right about firing blanks, that's intentional for resource_x_resource and resource_instances.

But de19d77d-4b59-41ae-bdee-aa7669cda961 isn't the current object, it's the random one, right?

BEFORE is self.pk, then
AFTER is after setting self.pk = uuid.uuid4()

BEFORE: a6cd3760-199b-4071-bd15-307fb04e53d2 AFTER: de19d77d-4b59-41ae-bdee-aa7669cda961

ah. yeah I get it 😞 . Def something to handle for

tests/models/graph_tests.py

chrabyrd added 2 commits November 7, 2024 17:10

Hardens editable_future_graph against nodegroup changes #11570

1638ecb

nit #11570

6abb19a

chrabyrd requested a review from jacobtylerwalls November 8, 2024 01:24

chrabyrd assigned jacobtylerwalls Nov 8, 2024

chrabyrd marked this pull request as draft November 8, 2024 03:56

chrabyrd added 16 commits November 8, 2024 15:54

correctly hardens editable_future_graphs #11570

d7372ed

re-architect editable_future_graphs #11570

d7e2bfc

nit #11570

b6c6a8d

nit #11570

e1b497f

nit #11570

2a34dc3

removes editable_future_graph_utils #11570

124560c

nit #11570

4964911

updates editable_future_graph tests #11570

db6051e

update graph #11570

c59b4b5

adds migration #11570

ccc1ff6

nit #11570

ee75d44

merge #11570

156e637

updates relateable resources logic #11570

fd77699

nit #11570

d57289b

nit #11570

d802939

nit #11570

92a69de

chrabyrd marked this pull request as ready for review November 14, 2024 06:48

jacobtylerwalls mentioned this pull request Nov 14, 2024

Add NodeGroup.grouping_node #11613 #11614

Open

11 tasks

chrabyrd added 3 commits November 14, 2024 07:44

nit #11570

6f54153

nit #11570

af89334

nit #11570

723485d

chrabyrd requested a review from robgaston November 14, 2024 17:26

johnatawnclementawn self-requested a review November 15, 2024 00:13

johnatawnclementawn requested changes Nov 15, 2024

View reviewed changes

Merge branch 'dev/8.0.x' of https://github.com/archesproject/arches i…

8f589d1

…nto 11570-cbyrd-harden-editable_future_graphs-against-nodegroup-changes

nit #11570

e2a7fc7

johnatawnclementawn reviewed Nov 19, 2024

View reviewed changes

tests/models/graph_tests.py Show resolved Hide resolved

jacobtylerwalls requested changes Nov 19, 2024

View reviewed changes

johnatawnclementawn reviewed Nov 20, 2024

View reviewed changes

tests/models/graph_tests.py Outdated Show resolved Hide resolved

chrabyrd added 2 commits November 20, 2024 13:54

nit #11570

23234ad

nit #11570

453c2e6

jacobtylerwalls mentioned this pull request Nov 21, 2024

deserialize() produces dict instead of model instances #11643

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardens editable_future_graph against nodegroup changes #11570 #11609

Hardens editable_future_graph against nodegroup changes #11570 #11609

chrabyrd commented Nov 8, 2024 •

edited

Loading

chrabyrd commented Nov 14, 2024

johnatawnclementawn left a comment

chrabyrd commented Nov 15, 2024 •

edited

Loading

jacobtylerwalls left a comment

jacobtylerwalls Nov 19, 2024

jacobtylerwalls Nov 19, 2024

jacobtylerwalls Nov 19, 2024

jacobtylerwalls Nov 19, 2024

jacobtylerwalls Nov 19, 2024

chrabyrd Nov 20, 2024

jacobtylerwalls Nov 21, 2024

chrabyrd Nov 21, 2024 •

edited

Loading

jacobtylerwalls Nov 21, 2024

chrabyrd Nov 21, 2024

		@@ -0,0 +1,38 @@
		# Generated by Django 5.1.2 on 2024-11-13 18:17

		source_graph = Graph.new(name="TEST RESOURCE", is_resource=True, author="TEST")
		source_graph.save()

Hardens editable_future_graph against nodegroup changes #11570 #11609

Are you sure you want to change the base?

Hardens editable_future_graph against nodegroup changes #11570 #11609

Conversation

chrabyrd commented Nov 8, 2024 • edited Loading

Types of changes

Description of Change

Issues Solved

Checklist

chrabyrd commented Nov 14, 2024

johnatawnclementawn left a comment

Choose a reason for hiding this comment

chrabyrd commented Nov 15, 2024 • edited Loading

jacobtylerwalls left a 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

chrabyrd Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrabyrd commented Nov 8, 2024 •

edited

Loading

chrabyrd commented Nov 15, 2024 •

edited

Loading

chrabyrd Nov 21, 2024 •

edited

Loading