This is an automated email from the ASF dual-hosted git repository.
jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new bfddf5cbca Fix early return in GroupMetaService.updateGroup ignoring
non-role changes (#8255)
bfddf5cbca is described below
commit bfddf5cbcac26915b85267cb5628495c12ca40c2
Author: JeonDaehong <[email protected]>
AuthorDate: Sat Aug 23 10:11:30 2025 +0900
Fix early return in GroupMetaService.updateGroup ignoring non-role changes
(#8255)
**What changes were proposed in this pull request?**
In `GroupMetaService.java`, the `updateGroup` method had an early return
when there were no role changes (`insertRoleIds.isEmpty() &&
deleteRoleIds.isEmpty()`), which caused other group changes like name
updates or audit info modifications to be ignored. This PR removes the
early return logic and ensures that group metadata is always updated,
while role-specific operations are only performed when necessary.
**Why are the changes needed?**
Without this fix, updating a group's name or other non-role properties
would be silently ignored if there were no role changes, leading to
inconsistent behavior. Users expecting group renames or audit info
updates would find their changes were not persisted to the database.
Fixes #8201
**Does this PR introduce any user-facing change?**
No. Valid requests now behave correctly where they previously failed
silently. Group updates that don't involve role changes will now work as
expected.
**How was this patch tested?**
* Added unit test `updateGroupWithoutRoleChange()` to verify that group
name changes work without role modifications
* Verified that the test fails with the old code and passes with the fix
* Confirmed that existing role-based update functionality remains
unchanged
* All existing tests continue to pass, ensuring no regression in role
management features
---
.../relational/service/GroupMetaService.java | 31 +++++++----------
.../relational/service/TestGroupMetaService.java | 40 ++++++++++++++++++++++
2 files changed, 53 insertions(+), 18 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java
b/core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java
index 4329b3a0a1..14fec721e1 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java
@@ -210,9 +210,6 @@ public class GroupMetaService {
Set<Long> insertRoleIds = Sets.difference(newRoleIds, oldRoleIds);
Set<Long> deleteRoleIds = Sets.difference(oldRoleIds, newRoleIds);
- if (insertRoleIds.isEmpty() && deleteRoleIds.isEmpty()) {
- return newEntity;
- }
try {
SessionUtils.doMultipleWithCommit(
() ->
@@ -223,25 +220,23 @@ public class GroupMetaService {
POConverters.updateGroupPOWithVersion(oldGroupPO,
newEntity),
oldGroupPO)),
() -> {
- if (insertRoleIds.isEmpty()) {
- return;
+ if (!insertRoleIds.isEmpty()) {
+ SessionUtils.doWithoutCommit(
+ GroupRoleRelMapper.class,
+ mapper ->
+ mapper.batchInsertGroupRoleRel(
+ POConverters.initializeGroupRoleRelsPOWithVersion(
+ newEntity, Lists.newArrayList(insertRoleIds))));
}
- SessionUtils.doWithoutCommit(
- GroupRoleRelMapper.class,
- mapper ->
- mapper.batchInsertGroupRoleRel(
- POConverters.initializeGroupRoleRelsPOWithVersion(
- newEntity, Lists.newArrayList(insertRoleIds))));
},
() -> {
- if (deleteRoleIds.isEmpty()) {
- return;
+ if (!deleteRoleIds.isEmpty()) {
+ SessionUtils.doWithoutCommit(
+ GroupRoleRelMapper.class,
+ mapper ->
+ mapper.softDeleteGroupRoleRelByGroupAndRoles(
+ newEntity.id(), Lists.newArrayList(deleteRoleIds)));
}
- SessionUtils.doWithoutCommit(
- GroupRoleRelMapper.class,
- mapper ->
- mapper.softDeleteGroupRoleRelByGroupAndRoles(
- newEntity.id(), Lists.newArrayList(deleteRoleIds)));
});
} catch (RuntimeException re) {
ExceptionUtils.checkSQLException(
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java
index 5e90f0eb89..a5ab960693 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java
@@ -1012,4 +1012,44 @@ class TestGroupMetaService extends TestJDBCBackend {
}
return count;
}
+
+ @Test
+ void updateGroupWithoutRoleChange() throws IOException {
+ AuditInfo auditInfo =
+
AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build();
+ BaseMetalake metalake =
+ createBaseMakeLake(RandomIdGenerator.INSTANCE.nextId(), metalakeName,
auditInfo);
+ backend.insert(metalake, false);
+
+ GroupMetaService groupMetaService = GroupMetaService.getInstance();
+
+ GroupEntity group1 =
+ createGroupEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ AuthorizationUtils.ofGroupNamespace(metalakeName),
+ "group1",
+ auditInfo);
+ groupMetaService.insertGroup(group1, false);
+
+ Function<GroupEntity, GroupEntity> renameUpdater =
+ group ->
+ GroupEntity.builder()
+ .withNamespace(group.namespace())
+ .withId(group.id())
+ .withName("group_renamed")
+ .withRoleNames(group.roleNames())
+ .withRoleIds(group.roleIds())
+ .withAuditInfo(group.auditInfo())
+ .build();
+ groupMetaService.updateGroup(group1.nameIdentifier(), renameUpdater);
+
+ Assertions.assertThrows(
+ NoSuchEntityException.class,
+ () -> groupMetaService.getGroupByIdentifier(group1.nameIdentifier()));
+
+ GroupEntity updated =
+ groupMetaService.getGroupByIdentifier(
+ AuthorizationUtils.ofGroup(metalakeName, "group_renamed"));
+ Assertions.assertEquals("group_renamed", updated.name());
+ }
}
