bugfix: auto delete roles relate to cluster#5395
bugfix: auto delete roles relate to cluster#5395nobodyiam merged 6 commits intoapolloconfig:masterfrom
Conversation
dosubot
bot
added
the
size:XS
WalkthroughA release note was added for a bugfix in Apollo 2.5.0 concerning automatic deletion of cluster-related roles. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClusterService
participant RolePermissionService
participant ClusterAPI
Client->>ClusterService: deleteCluster(env, appId, clusterName)
ClusterService->>ClusterAPI: delete(env, appId, clusterName)
ClusterService->>RolePermissionService: deleteRolePermissionsByCluster(appId, env, clusterName, operator)
RolePermissionService->>RolePermissionService: delete permissions and roles related to cluster
RolePermissionService-->>ClusterService: deletion complete
ClusterService-->>Client: cluster deleted with related roles/permissions removed
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug related to the automatic deletion of roles associated with clusters by updating the SQL conditions in the RoleRepository.
- Update the JPQL query in RoleRepository to include patterns for cluster-related roles.
- Update CHANGES.md to log the bugfix.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java | Updated SQL query conditions to include new patterns for cluster roles. |
| CHANGES.md | Added a bugfix entry for auto deletion of cluster-related roles. |
Comments suppressed due to low confidence (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java:39
- [nitpick] Verify the naming consistency: the new cluster-specific role pattern uses 'ModifyNamespacesInCluster' while existing patterns use the singular 'ModifyNamespace'. If intentional, consider documenting the naming convention.
+ + "OR r.roleName like CONCAT('ModifyNamespacesInCluster+', ?1, '+%')"
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java:40
- [nitpick] Verify the naming consistency: the new cluster-specific role pattern uses 'ReleaseNamespacesInCluster' while other related patterns use the singular form. If this is intentional, consider clarifying the convention in a comment.
+ + "OR r.roleName like CONCAT('ReleaseNamespacesInCluster+', ?1, '+%')"
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java
Show resolved
Hide resolved
dosubot
bot
removed
the
size:XS
dosubot
bot
added
the
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java (1)
26-26: Remove unused import.The
javax.transaction.Transactionalimport is not used in this file.-import javax.transaction.Transactional;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java
🔇 Additional comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java (1)
91-94: LGTM! Method declaration follows established patterns.The new method declaration is consistent with existing deletion methods and properly documents the cluster-scoped deletion functionality.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java (1)
54-56:Details
✅ Verification successful
Verify the targetId format consistency across the codebase.
The query logic appears correct, but please ensure the targetId format (
appId + '+' + env + '+' + clusterName) is consistent with how cluster-scoped permissions are created elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify how cluster-scoped permission targetIds are constructed in the codebase # Expected: Find patterns showing targetId construction for cluster permissions # Search for targetId construction patterns involving cluster names rg -A 3 -B 3 "targetId.*\+.*cluster|cluster.*\+.*targetId" --type java # Search for permission creation with cluster context ast-grep --pattern 'new Permission($$$)' | grep -A 10 -B 10 cluster # Search for ModifyNamespacesInCluster and ReleaseNamespacesInCluster usage rg "ModifyNamespacesInCluster|ReleaseNamespacesInCluster" --type java -A 2 -B 2Length of output: 19582
🏁 Script executed:
#!/bin/bash # Inspect createClusterRole method in DefaultRoleInitializationService rg -A10 -B2 "createClusterRole" --type java apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.javaLength of output: 1938
🏁 Script executed:
#!/bin/bash # Show buildClusterTargetId method and STRING_JOINER definition in RoleUtils rg -n "buildClusterTargetId" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java rg -A5 -B5 "buildClusterTargetId" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java rg -A5 -B5 "STRING_JOINER" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.javaLength of output: 4516
🏁 Script executed:
#!/bin/bash # Locate ConfigConsts to verify CLUSTER_NAMESPACE_SEPARATOR value rg -n "class ConfigConsts" -n . rg -A2 -B2 "CLUSTER_NAMESPACE_SEPARATOR" --no-headingLength of output: 21172
targetId format is consistent with buildClusterTargetId
The SQL in PermissionRepository
…WHERE p.targetId = CONCAT(?1, '+', ?2, '+', ?3)…matches exactly what RoleUtils.buildClusterTargetId(appId, env, clusterName) produces (it joins on
ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR, which is'+'). No further changes needed.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java (2)
37-37: LGTM! Proper dependency injection.The RolePermissionService dependency is correctly injected through constructor injection.
Also applies to: 40-41, 45-45
68-70: LGTM! Proper cleanup integration.The role permission cleanup is correctly called after the cluster deletion, ensuring data consistency.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java (1)
352-377:Details
✅ Verification successful
LGTM! Implementation follows established patterns correctly.
The implementation is consistent with existing deletion methods and properly handles the deletion order to maintain referential integrity. The transactional boundary and error handling are appropriate.
Please verify that the
roleRepository.findRoleIdsByClustermethod exists, as it's called but not shown in the provided files:
🏁 Script executed:
#!/bin/bash # Description: Verify the findRoleIdsByCluster method exists in RoleRepository # Expected: Find the method declaration and implementation # Search for the method in RoleRepository ast-grep --pattern 'findRoleIdsByCluster($_,$_,$_)' # Search for any references to this method rg "findRoleIdsByCluster" --type java -A 2 -B 2Length of output: 1492
Confirmed:
findRoleIdsByClusterExists – No Further Action NeededI’ve verified that
RoleRepositorydeclares the required method:
- In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java:List<Long> findRoleIdsByCluster(String appId, String env, String clusterName);Everything aligns with the deletion logic in
DefaultRolePermissionService. No changes are required.
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java
Outdated
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Transactional | ||
| @Override | ||
| public void deleteRolePermissionsByCluster(String appId, String env, String clusterName, String operator) { |
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java(3 hunks)apollo-portal/src/test/resources/sql/permission/RolePermissionServiceTest.deleteRolePermissionsByAppIdWithClusterRoles.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apollo-portal/src/test/resources/sql/permission/RolePermissionServiceTest.deleteRolePermissionsByAppIdWithClusterRoles.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RoleRepository.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java
- CHANGES.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
🔇 Additional comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java (1)
325-340: LGTM! Well-structured test for cluster role deletion.The test correctly verifies that cluster-related roles are properly deleted when calling
deleteRolePermissionsByAppId. The test logic is sound:
- Sets up test data via SQL script
- Calls the deletion method with a specific app ID
- Uses
RoleUtilsto construct expected role names (consistent with the utility class design)- Verifies that the cluster roles no longer exist after deletion
This test provides good coverage for the cluster role deletion functionality mentioned in the PR objectives.
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent testing framework imports.
The test class uses JUnit 4 throughout (imports on lines 19-21, @Test annotations), but line 22 imports assertNull from JUnit 5. This creates inconsistency and potential conflicts.
Use the JUnit 4 equivalent instead:
-import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.Assert.assertNull;🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java
at line 22, replace the import of assertNull from JUnit 5 with the JUnit 4
equivalent to maintain consistency with the rest of the test class that uses
JUnit 4. Change the import statement to use org.junit.Assert.assertNull instead
of org.junit.jupiter.api.Assertions.assertNull.
What's the purpose of this PR
fix bug
Which issue(s) this PR fixes:
Fixes ##5392
Brief changelog
fix sql conditions for quick fix
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Documentation
Bug Fixes