Aias00 commented on code in PR #7254:
URL: https://github.com/apache/shenyu/pull/7254#discussion_r4111256576
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DataPermissionServiceImpl.java:
##########
@@ -316,26 +313,32 @@ public void onRuleCreated(final RuleCreatedEvent event) {
dataPermissionDTO.setUserId(JwtUtils.getUserInfo().getUserId());
dataPermissionDTO.setDataId(event.getRule().getId());
dataPermissionDTO.setDataType(AdminConstants.RULE_DATA_TYPE);
-
dataPermissionMapper.insertSelective(DataPermissionDO.buildPermissionDO(dataPermissionDTO));
+
grantMissingPermissions(Collections.singletonList(dataPermissionDTO.getUserId()),
dataPermissionDTO.getDataId(), dataPermissionDTO.getDataType());
} else {
String namespaceId = event.getRule().getNamespaceId();
if (StringUtils.isNoneBlank(namespaceId)) {
// support namespace
List<NamespaceUserRelDO> namespaceUserRelDOList =
namespaceUserRelMapper.selectListByNamespaceId(namespaceId);
if (CollectionUtils.isNotEmpty(namespaceUserRelDOList)) {
- namespaceUserRelDOList.forEach(namespaceUserRelDO -> {
- DataPermissionDTO dataPermissionDTO = new
DataPermissionDTO();
-
dataPermissionDTO.setUserId(namespaceUserRelDO.getUserId());
- dataPermissionDTO.setDataId(event.getRule().getId());
-
dataPermissionDTO.setDataType(AdminConstants.RULE_DATA_TYPE);
-
dataPermissionMapper.insertSelective(DataPermissionDO.buildPermissionDO(dataPermissionDTO));
- });
+
grantMissingPermissions(namespaceUserRelDOList.stream().map(NamespaceUserRelDO::getUserId).collect(Collectors.toList()),
+ event.getRule().getId(),
AdminConstants.RULE_DATA_TYPE);
}
}
}
}
+ private void grantMissingPermissions(final List<String> userIds, final
String dataId, final Integer dataType) {
+ Set<String> existingUsers = new
HashSet<>(dataPermissionMapper.selectUserIds(dataId, dataType));
+ List<DataPermissionDO> permissions = userIds.stream().distinct()
+ .filter(userId -> !existingUsers.contains(userId))
+ .map(userId ->
DataPermissionDO.buildCreatePermissionDO(dataId, userId, dataType))
+ .collect(Collectors.toList());
+ if (CollectionUtils.isNotEmpty(permissions)) {
+ dataPermissionMapper.insertBatch(permissions);
+ }
Review Comment:
[suggestion] The dedup is a read-then-write with no unique constraint behind
it.
`data_permission` has only `PRIMARY KEY (id)` — see
`db/init/mysql/schema.sql:174-182` and
`shenyu-admin/src/main/resources/sql-script/h2/schema.sql:312-320`. There is no
`(user_id, data_id, data_type)` unique index, so two concurrent deliveries of
the same created-event can both observe an empty result here and both proceed
to `insertBatch`, producing duplicate grants.
The new `@Transactional` limits the blast radius (the loser now fails
instead of silently duplicating), but nothing actually prevents the duplicate
until a constraint exists to trip over. Consider adding `UNIQUE KEY
uk_user_data (user_id, data_id, data_type)` plus a `db/upgrade` script, and
letting `insertBatch` be the only dedup mechanism.
--
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]
