Skip to content
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

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

Merged
merged 10 commits into from
Oct 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bb47aff
Add /delete bidirectonal relations in case of owner
dikshathakur3119 Sep 28, 2020
56b43c4
Update unit test case
dikshathakur3119 Sep 28, 2020
c0ae7ae
Fix test case
dikshathakur3119 Sep 28, 2020
4f505a6
fix lint
dikshathakur3119 Sep 28, 2020
f709816
Add bidirectional relationships to both follow and read
dikshathakur3119 Sep 30, 2020
3ee0924
fix lint
dikshathakur3119 Sep 30, 2020
b58669f
Add Optional to delete owner query
dikshathakur3119 Sep 30, 2020
34d31a2
Fix lint
dikshathakur3119 Sep 30, 2020
4876ace
fix lint
dikshathakur3119 Sep 30, 2020
1c49649
fix lint
dikshathakur3119 Sep 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions metadata_service/proxy/neo4j_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ def add_owner(self, *,

upsert_owner_relation_query = textwrap.dedent("""
MATCH (n1:User {key: $user_email}), (n2:Table {key: $tbl_key})
MERGE (n2)-[r2:OWNER]->(n1)
MERGE (n1)-[r1:OWNER_OF]->(n2)-[r2:OWNER]->(n1)
dikshathakur3119 marked this conversation as resolved.
Show resolved Hide resolved
RETURN n1.key, n2.key
""")

Expand Down Expand Up @@ -546,15 +546,16 @@ def delete_owner(self, *,
owner: str) -> None:
"""
Delete the owner / owned_by relationship.

:param table_uri:
:param owner:
:return:
"""
delete_query = textwrap.dedent("""
MATCH (n1:User{key: $user_email})<-[r1:OWNER]-(n2:Table {key: $tbl_key}) DELETE r1
MATCH (n1:User{key: $user_email}), (n2:Table {key: $tbl_key})
OPTIONAL MATCH (n1)-[r1:OWNER_OF]->(n2)
OPTIONAL MATCH (n2)-[r2:OWNER]->(n1)
DELETE r1,r2
""")

try:
tx = self._driver.session().begin_transaction()
tx.run(delete_query, {'user_email': owner,
Expand Down Expand Up @@ -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]->' \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@feng-tao feng-tao Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaschalisDim let me know if it makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

f'(resource{resource_matcher})'
elif relation_type == UserResourceRel.own:
relation = f'(usr{user_matcher})<-[rel:OWNER]-(resource{resource_matcher})'
relation = f'(resource{resource_matcher})-[r1:OWNER]->(usr{user_matcher})-[r2:OWNER_OF]->' \
f'(resource{resource_matcher})'
elif relation_type == UserResourceRel.read:
relation = f'(usr{user_matcher})-[rel:READ]->(resource{resource_matcher})'
relation = f'(resource{resource_matcher})-[r1:READ_BY]->(usr{user_matcher})-[r2:READ]->' \
f'(resource{resource_matcher})'
else:
raise NotImplementedError(f'The relation type {relation_type} is not defined!')
return relation
Expand Down Expand Up @@ -1160,9 +1164,9 @@ def delete_resource_relation_by_user(self, *,
)

delete_query = textwrap.dedent("""
MATCH {rel_clause}
DELETE rel
""".format(rel_clause=rel_clause))
MATCH {rel_clause}
DELETE r1, r2
""".format(rel_clause=rel_clause))

try:
tx = self._driver.session().begin_transaction()
Expand Down
12 changes: 8 additions & 4 deletions tests/unit/proxy/test_neo4j_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,28 +963,32 @@ def test_user_resource_relation_clause(self) -> None:
id='foo',
user_key='bar',
resource_type=ResourceType.Table)
expected = '(usr:User {key: $user_key})-[rel:FOLLOW]->(resource:Table {key: $resource_key})'
expected = '(resource:Table {key: $resource_key})-[r1:FOLLOWED_BY]->(usr:User {key: $user_key})-' \
'[r2:FOLLOW]->(resource:Table {key: $resource_key})'
self.assertEqual(expected, actual)

actual = neo4j_proxy._get_user_resource_relationship_clause(UserResourceRel.read,
id='foo',
user_key='bar',
resource_type=ResourceType.Table)
expected = '(usr:User {key: $user_key})-[rel:READ]->(resource:Table {key: $resource_key})'
expected = '(resource:Table {key: $resource_key})-[r1:READ_BY]->(usr:User {key: $user_key})-[r2:READ]->' \
'(resource:Table {key: $resource_key})'
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]->' \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'(resource:Table {key: $resource_key})'
self.assertEqual(expected, actual)

actual = neo4j_proxy._get_user_resource_relationship_clause(UserResourceRel.follow,
id='foo',
user_key='bar',
resource_type=ResourceType.Dashboard)
expected = '(usr:User {key: $user_key})-[rel:FOLLOW]->(resource:Dashboard {key: $resource_key})'
expected = '(resource:Dashboard {key: $resource_key})-[r1:FOLLOWED_BY]->(usr:User {key: $user_key})-' \
'[r2:FOLLOW]->(resource:Dashboard {key: $resource_key})'
self.assertEqual(expected, actual)


Expand Down