fix: Add /delete bidirectional relations in case of owner #206

dikshathakur3119 · 2020-09-28T22:10:40Z

Signed-off-by: dikshathakur3119 [email protected]

Summary of Changes

Added Bidirectional relation to add/delete in case of user and owner relationship
When we add or delete an OWNER relation from UI, it only deletes one way relation whereas when we do it from extractor, it add bidirectional relations, which is causing some inconsistency in neo4j graph, and creating duplicate OWNER_OF relations. To make this consistent, bidirectional owner relations will be created by UI API as well.

Tests

No new tests added

Documentation

What documentation did you add or modify and why? Add any relevant links then remove this line

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
PR passes make test

Signed-off-by: dikshathakur3119 <[email protected]>

feng-tao · 2020-09-28T23:22:15Z

CI fails

Signed-off-by: dikshathakur3119 <[email protected]>

metadata_service/proxy/neo4j_proxy.py

feng-tao · 2020-09-29T20:32:39Z

metadata_service/proxy/neo4j_proxy.py

-        MATCH {rel_clause}
-        DELETE rel
-        """.format(rel_clause=rel_clause))
+        if relation_type == UserResourceRel.own:


could you comment on why we only delete two direction for owning resources?

i c, now wouldn't be an issue for bookmark/follow relationships? to do it generic, let's add bi-direction for all three relationships, and do delete for r1, r2 for all three cases? WDYT?

Yes it would be good to have consistent relations for follow and bookmark as well. I will update it

Signed-off-by: dikshathakur3119 <[email protected]>

feng-tao · 2020-09-30T03:51:53Z

lgtm, thanks @dikshathakur3119 , have you had the chance to deploy this branch to staging for a sanity test?

feng-tao

lgtm if done a sanity test on Lyft staging

dikshathakur3119 · 2020-09-30T16:48:31Z

lgtm if done a sanity test on Lyft staging

Deploying this branch on staging currently. Will merge after sanity tests.

Signed-off-by: dikshathakur3119 <[email protected]>

feng-tao · 2020-10-01T19:46:45Z

any other issues?

dikshathakur3119 · 2020-10-01T22:02:41Z

any other issues?

No I did not see any other issue today. Should I merge it or wait till Monday?

PaschalisDim · 2020-10-23T11:09:42Z

metadata_service/proxy/neo4j_proxy.py

@@ -958,11 +959,14 @@ def _get_user_resource_relationship_clause(relation_type: UserResourceRel, id: s
                user_matcher += ' {key: $user_key}'

        if relation_type == UserResourceRel.follow:
-            relation = f'(usr{user_matcher})-[rel:FOLLOW]->(resource{resource_matcher})'
+            relation = f'(resource{resource_matcher})-[r1:FOLLOWED_BY]->(usr{user_matcher})-[r2:FOLLOW]->' \


I think those lines should include optional as in the case of deletion. If not then, the existing owners/bookmarks, will not be able to retrieve their existing ones, if they have just one relation and thus it is a breaking change.

yeah, we should make it optional and delete if so. cc @dikshathakur3119 wdyt?

Thanks @PaschalisDim for mentioning, this function is used to create query for add, delete and read, I will fix this function to return appropriate match for each of these cases.

Thanks for taking this over.
@dikshathakur3119
Creation and deletion I guess should be fine. However, when returning what exists, we need to take care to return the ones that were created prior to this MR, so that is the only thing to get altered to include all cases.
Thanks

No actually, that was reason I made this change at first place. If we do delete with existing query, that is A -Relation1->B-Realtion2->A , it only deletes if there is exact match for bidirectional relation, else it will not make any delete to single relations. And when you use single directional queries to read a relation it will show your deleted tables as well.
Similarly, in case of add, if we add with query A -Relation1->B-Realtion2->A, it will create duplicate relation if there was existing only one way relation. And this will ultimately cause a loop and you will never be able to delete any owner/bookmark which was added from Frontend UI before this MR .

@PaschalisDim let me know if it makes sense

Hi @dikshathakur3119
Apologies, I was not clear enough here.
So, before this MR let's assume that there is a user who owns and follows a table (table A).
So he will have one relation (usr{user_matcher})<-[rel:OWNER]-(resource{resource_matcher}) and one relation
(usr{user_matcher})-[rel:FOLLOW]->(resource{resource_matcher}).
Then you make this MR.
After we merge this MR and it goes to production,
when the user will try the tables that he owns, the amundsenmeta library will search for (resource{resource_matcher})-[r1:OWNER]->(usr{user_matcher})-[r2:OWNER_OF]->'
f'(resource{resource_matcher})
these type of matches in order to see whether the user owns any table.
Since before this MR the user just had one-directional, the tableA will not show up in the list of the owned tables.
If he adds himself as an owner to another table, tableB, then he will have applied right the bi-directional and he will just see tableB in the list of owned tables.
To the point, for the existing one-way relations this change is a breaking change.
The add functionality should be fine. The get functionality is in my thoughts not right, since it will get just owners that have assigned themselves as owners just only AFTER this MR was deployed while I am not sure about the delete one which needs to be tested after the GET functionality is fixed.
@dikshathakur3119

PaschalisDim · 2020-11-02T10:18:55Z

@dikshathakur3119
Another thing that came to my mind today is that we need to make sure that we also update the neo4j_staleness_removal_task from databuilder when this one gets finalized, to ensure deletion of all the possible relationships of the table, since after this MR there will be more relationships added.
Thanks!

pPanda-beta · 2021-01-15T21:57:23Z

tests/unit/proxy/test_neo4j_proxy.py

            self.assertEqual(expected, actual)

            actual = neo4j_proxy._get_user_resource_relationship_clause(UserResourceRel.own,
                                                                        id='foo',
                                                                        user_key='bar',
                                                                        resource_type=ResourceType.Table)
-            expected = '(usr:User {key: $user_key})<-[rel:OWNER]-(resource:Table {key: $resource_key})'
+            expected = '(resource:Table {key: $resource_key})-[r1:OWNER]->(usr:User {key: $user_key})-[r2:OWNER_OF]->' \


@verdan
We need something similar (and precise as well) for atlas as well, what do you think?

Add /delete bidirectonal relations in case of owner

bb47aff

Signed-off-by: dikshathakur3119 <[email protected]>

dikshathakur3119 requested review from allisonsuarez, bolkedebruin, feng-tao, jinhyukchang, mgorsk1 and verdan as code owners September 28, 2020 22:10

Update unit test case

56b43c4

Signed-off-by: dikshathakur3119 <[email protected]>

dikshathakur3119 added 2 commits September 28, 2020 16:25

Fix test case

c0ae7ae

Signed-off-by: dikshathakur3119 <[email protected]>

fix lint

4f505a6

Signed-off-by: dikshathakur3119 <[email protected]>

feng-tao reviewed Sep 29, 2020

View reviewed changes

Add bidirectional relationships to both follow and read

f709816

Signed-off-by: dikshathakur3119 <[email protected]>

dikshathakur3119 force-pushed the dikksha-owner-rel branch from 8246d12 to f709816 Compare September 30, 2020 00:43

fix lint

3ee0924

Signed-off-by: dikshathakur3119 <[email protected]>

feng-tao approved these changes Sep 30, 2020

View reviewed changes

Add Optional to delete owner query

b58669f

Signed-off-by: dikshathakur3119 <[email protected]>

dikshathakur3119 force-pushed the dikksha-owner-rel branch from 0a43002 to b58669f Compare September 30, 2020 19:25

dikshathakur3119 added 3 commits September 30, 2020 12:50

Fix lint

34d31a2

Signed-off-by: dikshathakur3119 <[email protected]>

fix lint

4876ace

Signed-off-by: dikshathakur3119 <[email protected]>

fix lint

1c49649

Signed-off-by: dikshathakur3119 <[email protected]>

dikshathakur3119 force-pushed the dikksha-owner-rel branch from 80478c0 to 1c49649 Compare September 30, 2020 21:46

dikshathakur3119 merged commit 40cd0dd into master Oct 5, 2020

PaschalisDim reviewed Oct 23, 2020

View reviewed changes

feng-tao deleted the dikksha-owner-rel branch January 13, 2021 05:23

pPanda-beta reviewed Jan 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add /delete bidirectional relations in case of owner #206

fix: Add /delete bidirectional relations in case of owner #206

dikshathakur3119 commented Sep 28, 2020

feng-tao commented Sep 28, 2020

feng-tao Sep 29, 2020

feng-tao Sep 29, 2020

dikshathakur3119 Sep 30, 2020

feng-tao commented Sep 30, 2020

feng-tao left a comment

dikshathakur3119 commented Sep 30, 2020

feng-tao commented Oct 1, 2020

dikshathakur3119 commented Oct 1, 2020

PaschalisDim Oct 23, 2020

feng-tao Oct 23, 2020 •

edited

Loading

dikshathakur3119 Oct 23, 2020

PaschalisDim Oct 23, 2020

dikshathakur3119 Oct 23, 2020

dikshathakur3119 Oct 23, 2020

PaschalisDim Oct 24, 2020

PaschalisDim commented Nov 2, 2020

pPanda-beta Jan 15, 2021

fix: Add /delete bidirectional relations in case of owner #206

fix: Add /delete bidirectional relations in case of owner #206

Conversation

dikshathakur3119 commented Sep 28, 2020

Summary of Changes

Tests

Documentation

CheckList

feng-tao commented Sep 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-tao commented Sep 30, 2020

feng-tao left a comment

Choose a reason for hiding this comment

dikshathakur3119 commented Sep 30, 2020

feng-tao commented Oct 1, 2020

dikshathakur3119 commented Oct 1, 2020

Choose a reason for hiding this comment

feng-tao Oct 23, 2020 • edited Loading

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

PaschalisDim commented Nov 2, 2020

Choose a reason for hiding this comment

feng-tao Oct 23, 2020 •

edited

Loading