[#3343] feat(core): Supports multiple securable objects #3497

qqqttt123 · 2024-05-22T03:51:56Z

What changes were proposed in this pull request?

Refactor the underlying layout to support multiple securable objects.

Why are the changes needed?

Fix: #3343

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT.

qqqttt123 · 2024-05-22T06:06:14Z

@jerryshao Could you help me review this pull request?

xloya · 2024-05-22T06:54:17Z

scripts/mysql/schema-0.5.0-mysql.sql

@@ -135,10 +135,7 @@ CREATE TABLE IF NOT EXISTS `role_meta` (
    `role_name` VARCHAR(128) NOT NULL COMMENT 'role name',
    `metalake_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'metalake id',
    `properties` MEDIUMTEXT DEFAULT NULL COMMENT 'schema properties',
-    `securable_object_full_name` VARCHAR(256) NOT NULL COMMENT 'securable object full name',


If you need to change the structure of the MySQL table, in the long run, I think it is best to provide some additional alter table statements, such as creating an upgrade-0.5.0-to-0.5.1-mysql.sql file to help users of lower versions can be upgraded smoothly. You can create a new version of the schema sql file, such as schema-0.5.1-mysql.sql, so that users can directly use the new schema when accessing version 0.5.1.

This feature isn't available. Maybe we don't need to provide it now. Once we released this feature, we should provide alter scripts.

Think twice. You're right. I will update the script. Although our feature isn't available, users can create legacy table using the script.

The layout is shipped with 0.5.0, even it is not available from feature level, we still need to provide a script to support users to upgrade their storage layout. You can refer to #3506 for the solution.

jerryshao

Overall, I think you need to add more UTs to cover your changes. Currently, the UT is quite limited, doesn't cover all of your changes.

core/src/test/java/com/datastrato/gravitino/proto/TestEntityProtoSerDe.java

jerryshao · 2024-05-22T12:02:20Z

core/src/main/java/com/datastrato/gravitino/proto/RoleEntitySerDe.java

+            .name()
+            .equals(role.getSecurableObjects(index).getPrivilegeConditions(privIndex))) {
+          privileges.add(
+              Privileges.allow(role.getSecurableObjects(index).getPrivilegeNames(privIndex)));


Can you please add more tests to verify the serialization and deserialization of this part?

core/src/main/java/com/datastrato/gravitino/storage/relational/utils/POConverters.java

jerqi · 2024-05-23T08:40:44Z

@jerryshao I modified the ut to cover more cases.

jerryshao · 2024-05-23T12:55:41Z

core/src/main/java/com/datastrato/gravitino/storage/relational/utils/POConverters.java

          .withAuditInfo(
              JsonUtils.anyFieldMapper().readValue(rolePO.getAuditInfo(), AuditInfo.class))
          .build();
    } catch (JsonProcessingException e) {
      throw new RuntimeException("Failed to deserialize json object:", e);
    }
  }
+
+  private static List<SecurableObject> fromSecurablePersistObjects(String persisteSecurableObjects)


"persistent" or "persist"

core/src/main/java/com/datastrato/gravitino/storage/relational/utils/POConverters.java

jerryshao · 2024-05-23T13:08:53Z

core/src/test/resources/h2/schema-h2.sql

-    `securable_object_type` VARCHAR(32) NOT NULL COMMENT 'securable object type',
-    `privileges` VARCHAR(64) NOT NULL COMMENT 'securable privileges',
-    `privilege_conditions` VARCHAR(64) NOT NULL COMMENT 'securable privilege conditions',
+    `securable_objects` VARCHAR(2048) NOT NULL COMMENT 'securable objects',


I'm afraid 2048 may not be enough for lots of securable_objects if the role is a big role.

Change to TEXT.

jerryshao · 2024-05-23T13:09:34Z

scripts/mysql/upgrade-0.5.0-to-0.5.1-mysql.sql

+--
+
+ALTER TABLE `role_meta`
+    ADD COLUMN `securable_objects` VARCHAR(2048) NOT NULL COMMENT 'securable objects' AFTER `properties`;


Also here, 2048 may not be enough.

lw-yang · 2024-05-24T03:51:55Z

scripts/mysql/upgrade-0.5.0-to-0.5.1-mysql.sql

+ALTER TABLE `role_meta`
+    ADD COLUMN `securable_objects` VARCHAR(2048) NOT NULL COMMENT 'securable objects' AFTER `properties`;


we need an update script to migrate the existing securable_object data to the new securable_objects column

Feature isn't available now. We don't need to assume that we have already data.

I added the scripts.

lw-yang · 2024-05-24T03:55:37Z

scripts/mysql/upgrade-0.5.0-to-0.5.1-mysql.sql

+ALTER TABLE `role_meta` DROP COLUMN `securable_object_full_name`;
+ALTER TABLE `role_meta` DROP COLUMN `securable_object_type`;
+ALTER TABLE `role_meta` DROP COLUMN `privileges`;
+ALTER TABLE `role_meta` DROP COLUMN `privilege_conditions`;


In upgrade script, we should never remove fields, we should maintain backward compatibility.

If we execute this script, the production environment will immediately report an error.

I change to use alter.

This reverts commit 1487d89.

qqqttt123 · 2024-06-06T06:27:22Z

All comments are addressed.

jerryshao · 2024-06-06T22:47:21Z

.../src/main/java/com/datastrato/gravitino/storage/relational/mapper/SecurableObjectMapper.java

+      "DELETE FROM "
+          + SECURABLE_OBJECT_TABLE_NAME
+          + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeLine} LIMIT #{limit}")
+  Integer deleteSecurableObjectsByLegacyTimeLine(


@qqqttt123 can you please create a separate PR to fix all the typo "TimeLine" to "Timeline", timeline is a word, here it separates into two words.

OK, I will. I have created an issue to track this. #3813

jerryshao · 2024-06-06T22:50:17Z

core/src/main/java/com/datastrato/gravitino/storage/relational/po/SecurableObjectPO.java

+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class SecurableObjectPO {


Again, do you need a securable object id, can you please think a bit on this?

Securable object id seems not useful after think twice.

jerryshao · 2024-06-06T22:52:22Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/CatalogMetaService.java

+  // Catalog may be deleted, so the CatalogPO may be null.
+  // We don't choose to delete the securable object when we delete the catalog,
+  // Because they will bring conflicts if we are updating securable object
+  public CatalogPO getCatalogPOById(Long catalogId) {


You should add the nullable annotation, also the explanation should be in the right place, not here, it's a bit weird added here. Just the first sentence is enough.

jerryshao · 2024-06-06T22:58:35Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+  private static final String ROOT = "ROOT";
+  private static final String ALL_METALAKES = "*";
+  private static final long ALL_METALAKES_ENTITY_ID = 0;


I believe you also define this in other place, you should define them in a consoliated place, besides, this entity id should be defined in the Entity definition there, not here.

Can you please carefully think of how to better organize the code, not just simply achieve it.

jerryshao · 2024-06-06T23:00:17Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+                  RoleMetaMapper.class,
+                  mapper -> {
+                    if (overwritten) {
+                      mapper.insertRoleMetaOnDuplicateKeyUpdate(rolePO);


Why do you still need to call insert on duplicate since they're already marked as deleted above?

We just deleted the securable objects instead of role meta.

jerryshao · 2024-06-06T23:04:25Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+
+    for (SecurableObjectPO securableObjectPO : securableObjectPOs) {
+      String fullName = getSecurableObjectFullName(securableObjectPO);
+      if (fullName != null) {


You should add log about the null situation.

jerryshao · 2024-06-06T23:13:01Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+        + securableObjectsCount[0];
+  }
+
+  long getSecurableObjectEntityId(long metalakeId, String fullName, MetadataObject.Type type) {


a) Why there's no access level modifier for this method?
b) This and below methods are generic to role, you should make them generic and put into a utility class, so that tag implementation can also leverage these methods.

jerryshao · 2024-06-06T23:13:56Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+    throw new IllegalArgumentException(String.format("Doesn't support the type %s", type));
+  }
+
+  String getSecurableObjectFullName(SecurableObjectPO securableObjectPO) {


If it returns null, please add an annotation, also add more comments here.

jerryshao · 2024-06-06T23:18:37Z

core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestRoleMetaService.java

+        SecurableObjects.ofTopic(
+            schemaObject, "topic", Lists.newArrayList(Privileges.ReadTopic.deny()));
+    SecurableObject allMetalakesObject =
+        SecurableObjects.ofAllMetalakes(Lists.newArrayList(Privileges.UseMetalake.allow()));


Make securable object related test class be a separate class and add test to verify all the entities with cases.

jerryshao · 2024-06-07T04:27:24Z

.../src/main/java/com/datastrato/gravitino/storage/relational/mapper/SecurableObjectMapper.java

+    "</script>"
+  })
+  void batchInsertSecurableObjects(
+      @Param("securableObjects") List<SecurableObjectPO> securableObjectPOS);


“securableObjectPOs”

jerryshao · 2024-06-07T04:39:49Z

core/src/main/java/com/datastrato/gravitino/storage/relational/utils/MetadataObjectUtils.java

+      if (tablePO == null) {
+        return null;
+      }
+      return getSchemaFullName(tablePO.getSchemaId()) + "." + tablePO.getTableName();


use DOT joiner, instead of hard code "." here.

yuqi1129 · 2024-06-07T04:20:06Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java

+                roleEntity.id(), object, getEntityType(object));
+        objectBuilder.withEntityId(
+            MetadataObjectUtils.getSecurableObjectEntityId(
+                metalakeId, object.fullName(), object.type()));


What's the fullname used for? can we store the all pareants id at the same time, I notice you will split the fullanme every time and fetch the corresponding ID from DB.

It's similar to NameIdenfitier. The behaviour is similar to other nameidentier's implement. It's inconvenient to store parentId. The length of parents isn't fixed.

jerryshao

I'm generally OK with the current implementation with several small comments, @yuqi1129 please also help to review. I will merge this when @yuqi1129 's review and update is done.

yuqi1129 · 2024-06-07T06:17:02Z

core/src/main/java/com/datastrato/gravitino/storage/relational/utils/MetadataObjectUtils.java

+    throw new IllegalArgumentException(String.format("Doesn't support the type %s", type));
+  }
+
+  // Securable object may be  null because the securable object may be deleted.


extra space betwen be and null

yuqi1129 · 2024-06-07T06:20:37Z

LGTM overall except a minor issue.

jerryshao · 2024-06-07T06:44:12Z

I will merge this tomorrow if there's no more comment.

yuqi1129

LGTM

…#3497) ### What changes were proposed in this pull request? Refactor the underlying layout to support multiple securable objects. ### Why are the changes needed? Fix: apache#3343 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing UT. --------- Co-authored-by: Heng Qin <qqtt@123.com>

Supports multiple securable objects

85c56aa

qqqttt123 self-assigned this May 22, 2024

qqqttt123 requested a review from jerryshao May 22, 2024 06:05

xloya reviewed May 22, 2024

View reviewed changes

Heng Qin added 4 commits May 22, 2024 16:14

Remove limit

2dab53a

Remove blank

f97d872

Update SQL script

4ebe2ce

Modify document and add upgrade script

f8d0649

jerryshao reviewed May 22, 2024

View reviewed changes

Add new uts

65108ca

fix style

d71132f

jerryshao reviewed May 23, 2024

View reviewed changes

Change type

68a2cd4

apache deleted a comment from jerqi May 24, 2024

lw-yang reviewed May 24, 2024

View reviewed changes

Add fields

1487d89

lw-yang reviewed May 24, 2024

View reviewed changes

Heng Qin added 2 commits May 24, 2024 17:27

Use another table to store securable objects

927ec29

Merge remote-tracking branch 'upstream/main' into ISSUE-3343

80a3335

qqqttt123 closed this May 24, 2024

qqqttt123 reopened this May 24, 2024

Heng Qin added 7 commits May 24, 2024 17:41

Commit information

dd237ba

Commit information

89c3e42

Change to MetadataObject.Type

7a55a0e

fix style

f079305

Change SecurableObject type

1891cec

Update SQL

a2f8d5f

Revert "Add fields"

8a95066

This reverts commit 1487d89.

Heng Qin added 2 commits June 6, 2024 15:09

Add comment

a021351

address comments

edc15ef

jerryshao reviewed Jun 6, 2024

View reviewed changes

Address comments

140ad19

jerryshao reviewed Jun 7, 2024

View reviewed changes

yuqi1129 reviewed Jun 7, 2024

View reviewed changes

jerryshao reviewed Jun 7, 2024

View reviewed changes

Heng Qin added 3 commits June 7, 2024 13:32

Address comments

168919d

fix style

ec41730

fix ut

033ec85

yuqi1129 reviewed Jun 7, 2024

View reviewed changes

remove extra blank

3719280

fix typo

134ba80

yuqi1129 approved these changes Jun 7, 2024

View reviewed changes

jerryshao merged commit ab0eaf6 into apache:main Jun 7, 2024
33 checks passed

jerqi deleted the ISSUE-3343 branch June 7, 2024 22:38

		ALTER TABLE `role_meta`
		ADD COLUMN `securable_objects` VARCHAR(2048) NOT NULL COMMENT 'securable objects' AFTER `properties`;

[#3343] feat(core): Supports multiple securable objects #3497

[#3343] feat(core): Supports multiple securable objects #3497

Conversation

qqqttt123 commented May 22, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

qqqttt123 commented May 22, 2024

xloya May 22, 2024 • edited Loading

Choose a reason for hiding this comment

jerqi May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented May 23, 2024

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

qqqttt123 commented Jun 6, 2024

jerryshao Jun 6, 2024 • 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

jerryshao Jun 6, 2024 • 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

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

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

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 commented Jun 7, 2024

jerryshao commented Jun 7, 2024

yuqi1129 left a comment

Choose a reason for hiding this comment

xloya May 22, 2024 •

edited

Loading

jerqi May 22, 2024 •

edited

Loading

jerryshao Jun 6, 2024 •

edited

Loading

jerryshao Jun 6, 2024 •

edited

Loading