Support swapping default roles #2868

mithmatt · 2022-02-20T00:25:06Z

Pseudocode:

A. patchRole(role1, default=True):
B.   set role1.default = True
     for all users:
       add role1 to user.roles
C.   for role in roles if role != role1:
       set role.default = False
       for all users:
         delete role from user.roles

This logic ensures that changeDescription for the role(s) and user(s) are updated correctly.

Alternative considered:
Directly updating relationships with bulk sqlbatch update in entity_relationship between user and role to achieve the above.
However, this does not capture change description correctly.

Describe your changes :

See #2460

Type of change :

Frontend Preview (Screenshots) :

For frontend related change, please link screenshots of your changes preview! Optional for backend related changes.

Checklist:

I have read the CONTRIBUTING document.
I have performed a self-review of my own.
I have tagged my reviewers below.
I have commented on my code, particularly in hard-to-understand areas.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
All new and existing tests passed.

Reviewers

github-actions · 2022-02-20T00:25:56Z

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2022-02-20T00:26:42Z

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2022-02-20T04:45:45Z

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

mithmatt · 2022-02-20T07:43:14Z

...og-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java

@@ -56,11 +59,47 @@ public RoleResourceTest() {
  }

  @Test
-  void get_queryDefaultRole(TestInfo test) throws IOException {
-    Role defaultRole = createRolesAndSetDefault(test, 7);
+  void defaultRole(TestInfo test) throws IOException {


Main test for this feature

mithmatt · 2022-02-20T07:43:33Z

catalog-rest-service/src/main/resources/json/data/role/DataConsumer.json

@@ -2,5 +2,6 @@
  "name": "DataConsumer",
  "displayName": "Data Consumer",
  "description": "Users with Data Consumer role use different data assets for their day to day work.",
-  "deleted": false
+  "deleted": false,
+  "default": true


Setting DataConsumer as default role.

mithmatt · 2022-02-20T07:45:11Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

+      }
+      EntityRepository<User> userRepository = Entity.getEntityRepository(Entity.USER);
+      users
+          .parallelStream()


Parallelization to minimize performance impact.

Switching a default role for 100 users takes ~2 seconds on Mattbook Pro

Considered bulk update of relationships in entity_relationship table.
However, this does not capture the change description for the role update for each of the users.

Optimizing for correctness instead of performance for this feature.

mithmatt · 2022-02-20T07:48:06Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

-                  try {
-                    return withHref(uriInfo, setFields(JsonUtils.readValue(json, Role.class), fields));
-                  } catch (IOException e) {
-                    LOG.warn("Could not parse Role from json {}", json);


Wasn't happy about hiding this exception.
Replacing the lambda here with simple for loop.

mithmatt · 2022-02-20T07:48:30Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java

@@ -908,12 +908,12 @@ public EntityUpdater(T original, T updated, Operation operation) {
      this.operation = operation;
    }

-    public final void update() throws IOException {
+    public final void update() throws IOException, ParseException {


May need to make it throws Exception in the future.
Going with this for now.

mithmatt · 2022-02-20T07:49:10Z

bootstrap/sql/mysql/v004__create_db_connection_info.sql


 UPDATE role_entity
 SET json = JSON_SET(json, '$.default', FALSE);

+UPDATE role_entity
+SET json = JSON_SET(json, '$.default', TRUE)
+WHERE name = 'DataConsumer';


Set DataConsumer as default for migration purpose.

Pseudocode: ``` A. patchRole(role1, default=True): B. set role1.default = True for all users: add role1 to user.roles C. for role in roles if role != role1: set role.default = False for all users: delete role from user.roles ``` This ensures that changeDescription for the role(s) and user(s) are updated accordingly. Potential optimization: Adding role1 and removing role from user.roles could be considered/implemented as one change. However, increases code complexity.

harshach · 2022-02-20T18:40:09Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

+    List<Role> roles = new ArrayList<>();
+    for (String roleJson : daoCollection.roleDAO().getDefaultRoles()) {
+      roles.add(withHref(uriInfo, setFields(JsonUtils.readValue(roleJson, Role.class), fields)));
+    }
    if (roles.size() > 1) {


can't we avoid this ever happening when the admin tries to set the default role?

Technically, this PR with DB Transaction while swapping default role prevents getting into a situation where we have multiple default roles.

My testing pattern has been the following:

Start up OM with DataConsumer as default role

Set another role R as default role and observe that DataConsumer is no longer default and user relationships have been updated.

Set DataConsumer as default role and obseve that R is no longer default and user relationships have been updated.

I did a few iterations of these locally with PostMan and ended up converting the logic into a test (RoleResourceTest.defaultRole)

I left the log line as is for detection purpose, in case there are any unforeseeable edge cases being missed out (as the code evolves).

harshach · 2022-02-20T18:40:32Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

+        setDefaultToFalse(updatedRole);
+      }
+      recordChange("default", origRole.getDefault(), updatedRole.getDefault());
+      LOG.info(


lets mark this as debug?

mithmatt · 2022-02-20T18:53:36Z

...og-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java

+    // - the previous default role is no longer set as default
+    // - all users have roles set correctly (only current default role should exist)
+    for (int i = 0; i < 2; i++) { // Run two iterations for this test.
+      Role defaultRole = createRolesAndSetDefault(test, 7, i * 10);


This creates a set of 7 arbitrary roles and chooses one at random to be default (code below)

mithmatt · 2022-02-20T18:57:45Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

+      List<Role> defaultRoles = getDefaultRoles(null, ROLE_PATCH_FIELDS);
+      EntityRepository<Role> roleRepository = Entity.getEntityRepository(Entity.ROLE);
+      // Set default=FALSE for all existing default roles.
+      for (Role defaultRole : defaultRoles) {


There will be only one default role here (at any given point of time) based on how this feature has been implemented.

mithmatt · 2022-02-20T19:00:24Z

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

+          continue;
+        }
+        Role origDefaultRole = roleRepository.get(null, defaultRole.getId().toString(), ROLE_PATCH_FIELDS);
+        Role updatedDefaultRole = roleRepository.get(null, defaultRole.getId().toString(), ROLE_PATCH_FIELDS);


This is where jsonschema2pojo deep cloning would have helped.

Role origDefaultRole = roleRepository.get(null, defaultRole.getId().toString(), ROLE_PATCH_FIELDS); Role updatedDefaultRole = origDefaultRole.clone();

joelittlejohn/jsonschema2pojo#230

Resorting to two db calls for now. Need to add an internal utility for cloning entities, or contribute to jsonschema2pojo

sonarcloud · 2022-02-20T19:11:06Z

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2022-02-20T19:18:35Z

[catalog] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

88.1% Coverage
0.0% Duplication

mithmatt marked this pull request as draft February 20, 2022 00:25

mithmatt force-pushed the default_role_swap branch from 8e5bf6d to 9697a5e Compare February 20, 2022 00:26

mithmatt force-pushed the default_role_swap branch 2 times, most recently from cf90559 to 286fde3 Compare February 20, 2022 04:44

mithmatt force-pushed the default_role_swap branch 4 times, most recently from 7f14cf0 to 57d293c Compare February 20, 2022 05:37

mithmatt mentioned this pull request Feb 20, 2022

Set DataConsumer as default role #2864

Closed

12 tasks

mithmatt force-pushed the default_role_swap branch from 0e802e9 to dc99de8 Compare February 20, 2022 07:40

mithmatt commented Feb 20, 2022

View reviewed changes

mithmatt marked this pull request as ready for review February 20, 2022 07:49

mithmatt requested a review from harshach February 20, 2022 07:52

This was referenced Feb 20, 2022

Admin should be able to chose what is the default role a user should belong to #2460

Closed

Support ability to select a role as default from the UI #2874

Closed

mithmatt and others added 3 commits February 20, 2022 10:39

Set DataConsumer as default role

0d079a4

Fix tests

111ce6c

harshach reviewed Feb 20, 2022

View reviewed changes

mithmatt force-pushed the default_role_swap branch from 1f1517f to 4eef07c Compare February 20, 2022 18:41

harshach approved these changes Feb 20, 2022

View reviewed changes

mithmatt commented Feb 20, 2022

View reviewed changes

Fix code smell

4024d10

mithmatt force-pushed the default_role_swap branch from 4eef07c to 4024d10 Compare February 20, 2022 18:54

mithmatt commented Feb 20, 2022

View reviewed changes

mithmatt merged commit 9e4d8d7 into open-metadata:main Feb 20, 2022

mithmatt deleted the default_role_swap branch February 20, 2022 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support swapping default roles #2868

Support swapping default roles #2868

mithmatt commented Feb 20, 2022 •

edited

github-actions bot commented Feb 20, 2022

github-actions bot commented Feb 20, 2022

github-actions bot commented Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022

harshach Feb 20, 2022

mithmatt Feb 20, 2022

harshach Feb 20, 2022

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022 •

edited

mithmatt Feb 20, 2022

mithmatt Feb 20, 2022 •

edited

sonarcloud bot commented Feb 20, 2022

sonarcloud bot commented Feb 20, 2022

Support swapping default roles #2868

Support swapping default roles #2868

Conversation

mithmatt commented Feb 20, 2022 • edited

Describe your changes :

Type of change :

Frontend Preview (Screenshots) :

Checklist:

Reviewers

github-actions bot commented Feb 20, 2022

github-actions bot commented Feb 20, 2022

github-actions bot commented Feb 20, 2022

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

mithmatt Feb 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithmatt Feb 20, 2022 • edited

Choose a reason for hiding this comment

sonarcloud bot commented Feb 20, 2022

sonarcloud bot commented Feb 20, 2022

mithmatt commented Feb 20, 2022 •

edited

mithmatt Feb 20, 2022 •

edited

mithmatt Feb 20, 2022 •

edited