fix: correct permission target format to appId+env+namespace/cluster#5407
Conversation
Fixed the permission target ID format issue where cluster and namespace permissions were created with incorrect parameter order: - buildClusterTargetId: now generates appId+env+clusterName (was appId+clusterName+env) - buildNamespaceTargetId: now generates appId+env+namespaceName (was appId+namespaceName+env) - Fixed initClusterNamespaceRoles parameter order in AppService This resolves the extra bug mentioned in issue apolloconfig#5392 where default permissions had incorrect format.
WalkthroughThis change updates the format of permission target IDs by adjusting the order of concatenated parameters in utility methods and their usages. It also adds new unit tests to verify the updated formatting logic and documents the bugfix in the release notes. Changes
Sequence Diagram(s)sequenceDiagram
participant AppService
participant RoleUtils
participant RoleInitializationService
AppService->>RoleUtils: buildNamespaceTargetId(appId, namespace, env)
RoleUtils-->>AppService: returns "appId+env+namespace"
AppService->>RoleInitializationService: initClusterNamespaceRoles(appId, env, cluster, userId)
Assessment against linked issues
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 (
|
- Add tests for buildNamespaceTargetId with and without env parameter - Add tests for buildClusterTargetId method - Verify correct parameter order: appId+env+namespaceName/clusterName - Ensure the bug fix for permission format is properly tested
|
修改后： |
Add entry for PR apolloconfig#5407 to Apollo 2.5.0 changelog documenting the fix for permission target format issue (appId+env+namespace/cluster order).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
173-175: Duplicate logic – consider extracting to helperIdentical call inside the loop duplicates the logic above.
Minor, but extracting to a private method (or re-usingcreateAppInRemote) clarifies intent and eases future changes.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/util/RoleUtilsTest.java (1)
41-56: Consolidate repetitive assertions via parameterised testThe three test methods have near-identical structure.
JUnit 5’s@ParameterizedTest(or JUnit 4’sParameterized) would collapse this duplication and make future cases trivial to add.Example sketch (JUnit 5):
@ParameterizedTest @CsvSource({ "app1,application,,app1+application", "app1,application,LOCAL,app1+LOCAL+application", "test-app,config,DEV,test-app+DEV+config" }) void buildNamespaceTargetId(String appId, String ns, String env, String expected) { assertEquals(expected, RoleUtils.buildNamespaceTargetId(appId, ns, StringUtils.isBlank(env)?null:env)); }Not blocking, but improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/util/RoleUtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/util/RoleUtilsTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
🔇 Additional comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
145-147:Details
✅ Verification successful
Good catch on parameter order – ensure all call-sites are aligned
The reorder now matches the
(appId, env, clusterName, operator)signature.
Please run a quick scan to confirm no other invocations still use the old(appId, clusterName, env, …)order.
🏁 Script executed:
#!/bin/bash # Find potential mis-ordered calls (env looks like a constant CLUSTER_NAME_DEFAULT etc.) rg --line-number --context 2 $'initClusterNamespaceRoles\\([^,]+,[^,]+ConfigConsts\\.CLUSTER_NAME_DEFAULT' src/main/javaLength of output: 173
🏁 Script executed:
#!/bin/bash # Search for all invocations of initClusterNamespaceRoles across Java files rg --color=never --line-number --context 2 'initClusterNamespaceRoles' -g '*.java' .Length of output: 8856
All initClusterNamespaceRoles invocations use the new parameter order
I scanned every call site of
initClusterNamespaceRolesand confirmed they all now follow the(appId, env, clusterName, operator)signature. No instances of the old(appId, clusterName, env, …)order remain.CHANGES.md (1)
17-18: Entry looks goodRelease note clearly documents the behavioural change.
| public static String buildNamespaceTargetId(String appId, String namespaceName, String env) { | ||
| return STRING_JOINER.join(appId, namespaceName, env); | ||
| return STRING_JOINER.join(appId, env, namespaceName); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against blank env to avoid double separators
Joiner.skipNulls() omits null but not empty strings.
If callers pass "" (quite common when a variable is trimmed but not null-checked), the result will be
appId++namespaceName, producing an invalid target id and breaking ACL look-ups.
- return STRING_JOINER.join(appId, env, namespaceName);
+ return STRING_JOINER.join(appId, StringUtils.isBlank(env) ? null : env, namespaceName);(Use the existing org.apache.commons.lang3.StringUtils or your util of choice.)
🏁 Script executed:
#!/bin/bash
# Locate RoleUtils.java to confirm its path
echo "Searching for RoleUtils.java..."
fd --hidden --max-depth 4 RoleUtils.java
# Find definition of STRING_JOINER in RoleUtils.java
echo -e "\nDefinition of STRING_JOINER:"
rg -n "STRING_JOINER" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
# Show context around buildNamespaceTargetId method
echo -e "\nContext around buildNamespaceTargetId:"
rg -n "public static String buildNamespaceTargetId" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java -A3
# Check for skipNulls usage in RoleUtils.java
echo -e "\nskipNulls occurrences:"
rg -n "skipNulls" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java || echo "No skipNulls found in RoleUtils.java"Length of output: 2364
Guard blank env in buildNamespaceTargetId to avoid double separators
Joiner.skipNulls() omits nulls but not empty strings—passing "" produces appId++namespaceName. Wrap env in a blank‐check so empty values are treated as null:
- File:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java - Lines: 109–111
@@ apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java:109
- public static String buildNamespaceTargetId(String appId, String namespaceName, String env) {
- return STRING_JOINER.join(appId, env, namespaceName);
- }
+ public static String buildNamespaceTargetId(String appId, String namespaceName, String env) {
+ return STRING_JOINER.join(
+ appId,
+ StringUtils.isBlank(env) ? null : env,
+ namespaceName
+ );
+ }• Make sure you have import org.apache.commons.lang3.StringUtils; at the top.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String buildNamespaceTargetId(String appId, String namespaceName, String env) { | |
| return STRING_JOINER.join(appId, namespaceName, env); | |
| return STRING_JOINER.join(appId, env, namespaceName); | |
| } | |
| public static String buildNamespaceTargetId(String appId, String namespaceName, String env) { | |
| return STRING_JOINER.join( | |
| appId, | |
| StringUtils.isBlank(env) ? null : env, | |
| namespaceName | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
around lines 109 to 111, the buildNamespaceTargetId method uses
STRING_JOINER.skipNulls() which does not skip empty strings, causing double
separators if env is blank. Fix this by checking if env is blank using
StringUtils.isBlank(env) and passing null instead of an empty string to
STRING_JOINER.join. Also ensure the import for
org.apache.commons.lang3.StringUtils is present at the top of the file.
nobodyiam
left a comment
There was a problem hiding this comment.
Please also fix the conflicts.
…rd compatibility The previous change to buildNamespaceTargetId parameter order from appId+namespaceName+env to appId+env+namespaceName would break existing permission data stored in the database, causing permission checks to fail. Reverted to original appId+namespaceName+env format to ensure backward compatibility with existing deployments.
|
The erroneous modification has been reverted. tldr: This PR fixes an incorrect parameter passed during a function call, which caused a database format error. Please Take A Look, thank you. |
What's the purpose of this PR
Fixed the permission target ID format issue where cluster and namespace permissions were created with incorrect parameter order:
This resolves the extra bug mentioned in issue #5392 where default permissions had incorrect format.
Which issue(s) this PR fixes:
Fixes #5392 extra bug
Brief changelog
Fixed
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