New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add migration to fix database edges state #5640

Open

LucasG0 wants to merge 5 commits into stable from lgu-fix-to-time-migration

+391 −13

Contributor

LucasG0 commented Jan 31, 2025 •

edited

Loading

Fixes IFC-1204

Detail of the issue is within migration code

I updated the PR to have 3 separated queries:

One fixes incorrect -global- edgrs
One fixes missing to time
One fixes incorrectly deleted relationships edges when a node has been deleted

Note that, testing separatily each query might not be relevant as:

NodeListGetRelationshipsQuery filters on both requested branch AND -global- even for full aware nodes/rels, so setting an edge on global would not break it
Removing to time of a deleted edge might not have impact as query would still work as it would detect the deleted edge.

So in the end, a single test has been added reproducing (almost) corrupted state observed on user db.
Note this test does not trigger IFC-1204 bug, but a manual test confirmed this migration fixes it.

github-actions bot added the group/backend label

codspeed-hq bot commented Jan 31, 2025 •

edited

Loading

CodSpeed Performance Report

Merging #5640 will degrade performances by 20.65%

_{Comparing lgu-fix-to-time-migration (c2b0693) with stable (a663a5f)}

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`test_schemabranch_duplicate`	334.9 µs	422.1 µs	-20.65%

ajtmccarty reviewed

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

LucasG0 force-pushed the lgu-fix-to-time-migration branch 7 times, most recently from 083b5df to a6518f2 Compare

February 5, 2025 18:55

LucasG0 commented

View reviewed changes

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated

+                      """
+                      Fix corrupted state introduced by Migration012 when duplicating a CoreAccount (branch Aware)
+                      being part of a CoreStandardGroup (branch Agnostic). Database is corrupted at multiple points:
+                      - Old CoreAccount node <> group_member node `active` edge has no `to` time (possibly because of #5590).

Contributor Author

LucasG0 Feb 5, 2025

Actually, #5590 deserves a dedicated migration fixing missing to time for any couple of deleted edge on -global- with an active edge on another branch.

LucasG0 requested a review from a team

February 5, 2025 19:01

LucasG0 commented

View reviewed changes

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

ajtmccarty reviewed

View reviewed changes

Contributor

ajtmccarty left a comment

this is a good start
as I say in my comments, I think we should split this into at least 2 migrations
I took a shot at writing a more general version of what I think the first migration should be

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

LucasG0 force-pushed the lgu-fix-to-time-migration branch 7 times, most recently from ab99efb to 4ea0e08 Compare

February 11, 2025 15:30

LucasG0 marked this pull request as ready for review

February 11, 2025 15:34

LucasG0 requested review from a team and ajtmccarty

February 11, 2025 15:34

LucasG0 changed the title ~~Add migration to restore time~~ Add migration to fix wrong db edges state

LucasG0 changed the title ~~Add migration to fix wrong db edges state~~ Add migration to fix database edges state

LucasG0 force-pushed the lgu-fix-to-time-migration branch 3 times, most recently from 7828def to a842f14 Compare

February 11, 2025 17:38

ajtmccarty reviewed

View reviewed changes

Contributor

ajtmccarty left a comment

this is most of the way there
some comments on the queries

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

LucasG0 force-pushed the lgu-fix-to-time-migration branch 2 times, most recently from 746e80e to 699adfb Compare

February 13, 2025 09:07

LucasG0 requested review from ajtmccarty and a team

February 13, 2025 18:03

ajtmccarty reviewed

View reviewed changes

Contributor

ajtmccarty left a comment

looking great, just a few small comments to go

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated

+                              MATCH (rel)-[active_edge {status: "active"}]-(peer_2)
+                              RETURN active_edge.branch as active_edge_branch
+                              LIMIT 1
+                      }

Contributor

ajtmccarty Feb 14, 2025

if a relationship is created on a branch and then merged into main, there will actually be 2 active edges between the Node node and Relationship node
and it is possible that a node in this situation is deleted on just one of the branches for which it has an active edge
would it work to just use deleted_edge_branch here instead of trying to get active_edge_branch? deleted_edge_branch should be the correct branch to delete, right?

Contributor Author

LucasG0 Feb 14, 2025

I should have added more comments, taking into consideration the fact that relationship and node might not have the same branch support type, we may need to retrieve active branch, in details:

If res is agnostic, we should delete on global branch (and we do not use deleted_edge_branch)
If rel is aware and deleted node is aware, we should use deleted edge branch
If rel is aware and delete node is agnostic, we need to create deleted edges for any branch on which this relationship exists. I did not have in mind situations where there could be multiple active branches, so I think we should actually retrieve any distinct branch for which an active edge exist, instead of a single one as it's currently done.

Contributor

ajtmccarty Feb 14, 2025

this makes sense to me

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Show resolved Hide resolved

LucasG0 added 2 commits

February 17, 2025 10:17


          Add migration to fix database edges state

d83e774


          Fix queries

29424c6

LucasG0 force-pushed the lgu-fix-to-time-migration branch from 699adfb to 55db64d Compare

February 17, 2025 09:22


          Improve computation of branch on which create the deleted edge

ea5beef

LucasG0 force-pushed the lgu-fix-to-time-migration branch from 55db64d to ea5beef Compare

February 17, 2025 10:09

LucasG0 commented

View reviewed changes

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated

+                      // Note that if an AWARE node has been deleted on a branch and relationship is AGNOSTIC, we do not "delete" this relationship
+                      // right now as this aware node might exist on another branch. Or, should we check there is no existing active is_part_of
+                      // to consider this node completely deleted and so we can also deleted connected agnostic relationships?

Contributor Author

LucasG0 Feb 17, 2025

@ajtmccarty would be glad to have your opinion on this

Contributor

ajtmccarty Feb 17, 2025

if you have seen this issue during your testing, then I think we should handle it, meaning check if the aware node is completely deleted and, if so, delete the agnostic relationship
but if you haven't encountered it, then I think we can leave this as is
this actually sounds like something that we might not have correct handling for in our cypher queries to delete a node, I can look at adding a unit test for deleting an aware node with an agnostic relationship to check if we handle

Contributor Author

LucasG0 Feb 17, 2025

I did not met that while testing, I let it as is

LucasG0 commented

View reviewed changes

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py Outdated

+                                       }
+                                ELSE []  // deleted_node.branch_support = -local-
+                              END
+                          ELSE []  // rel.branch_support = -local-

Contributor Author

LucasG0 Feb 17, 2025

@ajtmccarty, not exactly sure when/why we end up with rel.branch_support = -local- and/or deleted_node.branch_support = -local-, we might need to discuss that.

Contributor

ajtmccarty Feb 17, 2025

it's just "local", without the hyphens
local branch support means that an object only exists on the branch where it is created and will not be merged back into main when a branch is merged
I think that you can treat branch_support = "local" the same as branch_support = "aware" nodes and relationships for the purpose of this migration.
so I think this case statement can simplified to check for <> "agnostic" instead of = "aware" to handle branch_support = "local" nodes too


          Same behavior for local and aware branch support type

7f26453

ajtmccarty reviewed

View reviewed changes

Contributor

ajtmccarty left a comment

I think the migration queries look good
just a few test-related comments

backend/tests/unit/core/migrations/graph/test_019.py Outdated Show resolved Hide resolved

backend/tests/unit/core/migrations/graph/test_019.py Outdated Show resolved Hide resolved

backend/tests/unit/core/migrations/graph/test_019.py Outdated Show resolved Hide resolved

backend/tests/unit/core/migrations/graph/test_019.py Outdated

+                              RETURN new_node;
+                              """
+                              await ts.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME})

Contributor

ajtmccarty Feb 18, 2025

might be good to validate all the edges are as expected with a cypher query

LucasG0 requested a review from ajtmccarty

February 18, 2025 09:41


          Improve test

c2b0693

LucasG0 force-pushed the lgu-fix-to-time-migration branch from 5270ad0 to c2b0693 Compare

February 18, 2025 09:57

ajtmccarty approved these changes

View reviewed changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels