jerryshao commented on code in PR #13880:
URL: https://github.com/apache/gravitino/pull/13880#discussion_r4386382935
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java:
##########
@@ -106,8 +114,8 @@ public GroupEntity getGroupByIdentifier(NameIdentifier
identifier) {
NameIdentifier metalakeIdent =
NameIdentifier.of(NameIdentifierUtil.getMetalake(identifier));
long metalakeId = EntityIdService.getEntityId(metalakeIdent,
Entity.EntityType.METALAKE);
- GroupPO groupPO = getGroupPOByMetalakeIdAndName(metalakeId,
identifier.name());
- List<RolePO> rolePOs =
RoleMetaService.getInstance().listRolesByGroupId(groupPO.getGroupId());
+ GroupPO groupPO = getGroupPOByMetalakeIdAndName(metalakeId,
identifier.name(), true);
+ List<RolePO> rolePOs = groupPO.requireRolePOs();
Review Comment:
[Nit] With this line and `updateGroup` (GroupMetaService.java:292) reading
memberships from the group statement, `RoleMetaService.listRolesByGroupId`
(RoleMetaService.java:158) has no production caller left. Its `@Monitored`
metric `listRolesByGroupId` therefore goes to zero, the same observability
change the PR body already discloses for `listSecurableObjectsByRoleId` — but
this one is not mentioned anywhere. Either add it to the same sentence in the
description and the audit doc, or drop the now-unused public method.
The user side is not affected: `listRolesByUserId` still has a caller in
`listRolesByUserIdent` (RoleMetaService.java:114).
Verified by: grepped `listRolesByGroupId` across the repository excluding
tests. The only remaining hits are its own declaration, the mapper, the
provider, and `JcasbinAuthorizer.java:1350`, which calls
`RoleMetaMapper.listRolesByGroupId` directly and so never passes through the
monitored service method.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/RoleMetaBaseSQLProvider.java:
##########
@@ -232,7 +232,46 @@ public String batchGetRoleUpdatedAt(@Param("roleIds")
List<Long> roleIds) {
}
return "<script>SELECT role_id as roleId, role_name as roleName,
updated_at as updatedAt FROM "
+ ROLE_TABLE_NAME
- + " WHERE role_id IN <foreach item='id' collection='roleIds' open='('
separator=',' close=')'>#{id}</foreach>"
- + " AND deleted_at = 0</script>";
+ + " WHERE role_id IN <foreach item='id' collection='roleIds' open='('
separator=','"
+ + " close=')'>#{id}</foreach> AND deleted_at = 0</script>";
+ }
+ /**
+ * Returns a single-statement role and privilege snapshot by name.
+ *
+ * @param metalakeId the metalake ID
+ * @param roleName the role name
+ * @return the aggregate select SQL
+ */
+ public String selectRoleWithPrivilegesByMetalakeIdAndName(
+ @Param("metalakeId") Long metalakeId, @Param("roleName") String
roleName) {
+ return withPrivileges(selectRoleMetaByMetalakeIdAndName(metalakeId,
roleName));
+ }
+
+ /**
+ * Returns matching roles together with all their privileges.
+ *
+ * @param metadataObjectId the metadata object ID
+ * @param metadataObjectType the metadata object type
+ * @return the aggregate select SQL
+ */
+ public String listRolesWithPrivilegesByMetadataObjectIdAndType(
+ @Param("metadataObjectId") Long metadataObjectId,
+ @Param("metadataObjectType") String metadataObjectType) {
+ return withPrivileges(listRolesByMetadataObjectIdAndType(metadataObjectId,
metadataObjectType));
+ }
+
+ private String withPrivileges(String roleSql) {
+ // Role overwrites replace their privilege rows. Resolve both from one
statement snapshot,
+ // including empty roles, rather than combining a root read with a later
privilege read.
+ return "SELECT r.*, p.id AS priv_relationId, p.role_id AS priv_roleId,
p.metadata_object_id AS"
Review Comment:
[Nit] The static-root guard landed in `withColumns` only. `withPrivileges`
embeds `selectRoleMetaByMetalakeIdAndName` and
`listRolesByMetadataObjectIdAndType` in a derived table exactly the same way,
and `UserMetaBaseSQLProvider.selectUserWithRolesByMetalakeIdAndName`
(UserMetaBaseSQLProvider.java:402) and
`GroupMetaBaseSQLProvider.selectGroupWithRolesByMetalakeIdAndName`
(GroupMetaBaseSQLProvider.java:326) do the same with their roots. None of the
three checks the root, so the contract the table wrapper now enforces is
documented in one place out of four. A backend override returning a `<script>`
root for any of these would put the tag inside `FROM (...)`, where MyBatis
stops treating it as dynamic SQL and ships the XML to the database as literal
text.
Neither wrapper strips a trailing semicolon either, which is fine today but
is the other half of the same undocumented assumption. Suggestion: lift the
check into one small helper, for example `requireStaticRoot(String sql)`, and
call it from all four wrappers.
Not blocking: I grepped the h2, mysql and postgresql provider packages and
none of the six embedded roots is overridden, so nothing is dynamic today.
Verified by: read `withPrivileges` and both principal wrappers in full on
this checkout, confirmed the guard exists only at
TableMetaBaseSQLProvider.java:469-473, and grepped the three backend provider
packages for overrides of the six root methods (no hits).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
