refactor(openapi): final cleanup placeholder (no-op) #5477
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
the
size:XL
WalkthroughAdds OpenAPI code-generation tooling in apollo-portal/pom.xml with new dependencies, properties, plugins, and JDK-specific profiles. Introduces OpenApiModelConverters with numerous DTO/BO conversion methods. Updates AuthConfiguration to pass a lazily-injected ConsumerService. Enhances SpringSecurityUserInfoHolder to resolve users from OpenAPI Consumer context before falling back to Spring Security. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Portal as Portal (Controller)
participant UIH as SpringSecurityUserInfoHolder
participant CS as ConsumerService
participant US as UserService
Client->>Portal: HTTP request
Portal->>UIH: getUser()
alt OpenAPI request (/openapi/...)
UIH->>UIH: Extract consumerId from request context
UIH->>CS: findConsumer(consumerId)
alt Consumer found
CS-->>UIH: Consumer data
UIH-->>Portal: UserInfo (from Consumer)
else Consumer not found/error
UIH->>US: findByUsername(principal)
US-->>UIH: UserInfo
UIH-->>Portal: UserInfo (from Spring Security)
end
else Non-OpenAPI request
UIH->>US: findByUsername(principal)
US-->>UIH: UserInfo
UIH-->>Portal: UserInfo
end
Portal-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
tacklequestions
force-pushed
the
pr/a10-cleanup
branch
from
a0f40b7 to
c49cca1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/pom.xml (1)
30-174: Remote OpenAPI spec makes the build non-deterministicBinding code generation to
https://raw.githubusercontent.com/.../apollo-openapi.yamlmeans every local/CI build now depends on external network access and on whatever happens to be onmainof that personal repo at build time. This is a supply-chain and reproducibility risk; the spec (and therefore generated sources) can change without review, and offline builds will fail outright. Please vendor the spec inside this repository (or at least pin to an immutable commit/tag that we control) before wiring the generator into the build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apollo-portal/pom.xml(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
SpringSecurityUserInfoHolder(33-121)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (8)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/ItemBO.java (1)
ItemBO(21-76)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/NamespaceBO.java (1)
NamespaceBO(23-103)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/ReleaseBO.java (1)
ReleaseBO(24-46)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceSyncModel.java (1)
NamespaceSyncModel(26-70)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceTextModel.java (1)
NamespaceTextModel(24-104)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/EnvClusterInfo.java (1)
EnvClusterInfo(24-48)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/NamespaceIdentifier.java (1)
NamespaceIdentifier(23-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
Comment on lines
+242
to
+256
| public static GrayReleaseRuleDTO toGrayReleaseRuleDTO(OpenGrayReleaseRuleDTO openGrayReleaseRuleDTO) { | ||
| Preconditions.checkArgument(openGrayReleaseRuleDTO != null); | ||
| String appId = openGrayReleaseRuleDTO.getAppId(); | ||
| String branchName = openGrayReleaseRuleDTO.getBranchName(); | ||
| String clusterName = openGrayReleaseRuleDTO.getClusterName(); | ||
| String namespaceName = openGrayReleaseRuleDTO.getNamespaceName(); | ||
| GrayReleaseRuleDTO grayReleaseRuleDTO = new GrayReleaseRuleDTO(appId, clusterName, namespaceName, branchName); | ||
| Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()); | ||
| openGrayReleaseRuleItemDTOSet.forEach(openGrayReleaseRuleItemDTO -> { | ||
| String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId(); | ||
| Set<String> clientIpList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); | ||
| Set<String> clientLabelList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()); | ||
| GrayReleaseRuleItemDTO ruleItem = new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList); | ||
| grayReleaseRuleDTO.addRuleItem(ruleItem); | ||
| }); |
There was a problem hiding this comment.
Guard against null rule items in gray release conversion.
new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()) blows up when clients omit ruleItems, and the follow-up new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()) / getClientLabelList() NPE if those collections are null. That turns a valid OpenAPI payload into a 500. Please null-check before copying and default to empty sets. Example fix:
- Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = new HashSet<>(openGrayReleaseRuleDTO.getRuleItems());
- openGrayReleaseRuleItemDTOSet.forEach(openGrayReleaseRuleItemDTO -> {
+ Collection<OpenGrayReleaseRuleItemDTO> ruleItems = openGrayReleaseRuleDTO.getRuleItems();
+ if (CollectionUtils.isEmpty(ruleItems)) {
+ return grayReleaseRuleDTO;
+ }
+ for (OpenGrayReleaseRuleItemDTO openGrayReleaseRuleItemDTO : new LinkedHashSet<>(ruleItems)) {
String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId();
- Set<String> clientIpList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList());
- Set<String> clientLabelList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList());
+ Set<String> clientIpList = openGrayReleaseRuleItemDTO.getClientIpList() == null
+ ? Collections.emptySet()
+ : new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList());
+ Set<String> clientLabelList = openGrayReleaseRuleItemDTO.getClientLabelList() == null
+ ? Collections.emptySet()
+ : new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList());
GrayReleaseRuleItemDTO ruleItem = new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList);
grayReleaseRuleDTO.addRuleItem(ruleItem);
- });
+ }📝 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.
Suggested change
| public static GrayReleaseRuleDTO toGrayReleaseRuleDTO(OpenGrayReleaseRuleDTO openGrayReleaseRuleDTO) { | |
| Preconditions.checkArgument(openGrayReleaseRuleDTO != null); | |
| String appId = openGrayReleaseRuleDTO.getAppId(); | |
| String branchName = openGrayReleaseRuleDTO.getBranchName(); | |
| String clusterName = openGrayReleaseRuleDTO.getClusterName(); | |
| String namespaceName = openGrayReleaseRuleDTO.getNamespaceName(); | |
| GrayReleaseRuleDTO grayReleaseRuleDTO = new GrayReleaseRuleDTO(appId, clusterName, namespaceName, branchName); | |
| Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()); | |
| openGrayReleaseRuleItemDTOSet.forEach(openGrayReleaseRuleItemDTO -> { | |
| String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId(); | |
| Set<String> clientIpList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); | |
| Set<String> clientLabelList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()); | |
| GrayReleaseRuleItemDTO ruleItem = new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList); | |
| grayReleaseRuleDTO.addRuleItem(ruleItem); | |
| }); | |
| public static GrayReleaseRuleDTO toGrayReleaseRuleDTO(OpenGrayReleaseRuleDTO openGrayReleaseRuleDTO) { | |
| Preconditions.checkArgument(openGrayReleaseRuleDTO != null); | |
| String appId = openGrayReleaseRuleDTO.getAppId(); | |
| String branchName = openGrayReleaseRuleDTO.getBranchName(); | |
| String clusterName = openGrayReleaseRuleDTO.getClusterName(); | |
| String namespaceName = openGrayReleaseRuleDTO.getNamespaceName(); | |
| GrayReleaseRuleDTO grayReleaseRuleDTO = new GrayReleaseRuleDTO(appId, clusterName, namespaceName, branchName); | |
| Collection<OpenGrayReleaseRuleItemDTO> ruleItems = openGrayReleaseRuleDTO.getRuleItems(); | |
| if (CollectionUtils.isEmpty(ruleItems)) { | |
| return grayReleaseRuleDTO; | |
| } | |
| for (OpenGrayReleaseRuleItemDTO openGrayReleaseRuleItemDTO : new LinkedHashSet<>(ruleItems)) { | |
| String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId(); | |
| Set<String> clientIpList = openGrayReleaseRuleItemDTO.getClientIpList() == null | |
| ? Collections.emptySet() | |
| : new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); | |
| Set<String> clientLabelList = openGrayReleaseRuleItemDTO.getClientLabelList() == null | |
| ? Collections.emptySet() | |
| : new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()); | |
| GrayReleaseRuleItemDTO ruleItem = | |
| new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList); | |
| grayReleaseRuleDTO.addRuleItem(ruleItem); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
around lines 242 to 256, the conversion assumes
openGrayReleaseRuleDTO.getRuleItems(), getClientIpList(), and
getClientLabelList() are non-null and will NPE; change to guard against nulls by
treating ruleItems as empty collection when null, iterate only if
non-null/empty, and when constructing clientIpList and clientLabelList default
to empty sets if their getters return null before creating
GrayReleaseRuleItemDTO; ensure you still add items when present but never pass
null collections into new HashSet<>().
Comment on lines
+83
to
+104
| // 获取Consumer ID | ||
| Object consumerIdObj = request.getAttribute("Authorization"); | ||
| if (consumerIdObj == null) { | ||
| return null; | ||
| } | ||
|
|
||
| long consumerId = Long.parseLong(consumerIdObj.toString()); | ||
| Consumer consumer = consumerService.getConsumerByConsumerId(consumerId); | ||
| if (consumer == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // 构建基于Consumer的用户信息 | ||
| UserInfo userInfo = new UserInfo(); | ||
| userInfo.setUserId(consumer.getOwnerName()); | ||
| userInfo.setName(consumer.getName()); | ||
| userInfo.setEmail(consumer.getOwnerEmail()); | ||
|
|
||
| return userInfo; | ||
| } catch (Exception e) { | ||
| // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 | ||
| return null; |
There was a problem hiding this comment.
OpenAPI consumer resolution never succeeds
request.getAttribute("Authorization") is never populated by the OpenAPI auth filters; the consumer credential only arrives as the HTTP Authorization header (e.g. Apollo <consumerId>:<signature>). Parsing that string with Long.parseLong(...) will therefore always throw and you silently fall back to the legacy path, wiping out the intended OpenAPI user resolution. Please read the header and extract the consumerId before calling consumerService.
- // 获取Consumer ID
- Object consumerIdObj = request.getAttribute("Authorization");
- if (consumerIdObj == null) {
- return null;
- }
-
- long consumerId = Long.parseLong(consumerIdObj.toString());
+ String authorization = request.getHeader("Authorization");
+ if (authorization == null || !authorization.startsWith("Apollo ")) {
+ return null;
+ }
+
+ String credentials = authorization.substring("Apollo ".length());
+ int separator = credentials.indexOf(':');
+ if (separator < 0) {
+ return null;
+ }
+
+ long consumerId = Long.parseLong(credentials.substring(0, separator));📝 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.
Suggested change
| // 获取Consumer ID | |
| Object consumerIdObj = request.getAttribute("Authorization"); | |
| if (consumerIdObj == null) { | |
| return null; | |
| } | |
| long consumerId = Long.parseLong(consumerIdObj.toString()); | |
| Consumer consumer = consumerService.getConsumerByConsumerId(consumerId); | |
| if (consumer == null) { | |
| return null; | |
| } | |
| // 构建基于Consumer的用户信息 | |
| UserInfo userInfo = new UserInfo(); | |
| userInfo.setUserId(consumer.getOwnerName()); | |
| userInfo.setName(consumer.getName()); | |
| userInfo.setEmail(consumer.getOwnerEmail()); | |
| return userInfo; | |
| } catch (Exception e) { | |
| // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 | |
| return null; | |
| // 获取Consumer ID | |
| - Object consumerIdObj = request.getAttribute("Authorization"); | |
| - if (consumerIdObj == null) { | |
| - return null; | |
| - } | |
| - | |
| String authorization = request.getHeader("Authorization"); | |
| if (authorization == null || !authorization.startsWith("Apollo ")) { | |
| return null; | |
| } | |
| String credentials = authorization.substring("Apollo ".length()); | |
| int separator = credentials.indexOf(':'); | |
| if (separator < 0) { | |
| return null; | |
| } | |
| long consumerId = Long.parseLong(credentials.substring(0, separator)); | |
| Consumer consumer = consumerService.getConsumerByConsumerId(consumerId); | |
| if (consumer == null) { | |
| return null; | |
| } | |
| // 构建基于Consumer的用户信息 | |
| UserInfo userInfo = new UserInfo(); | |
| userInfo.setUserId(consumer.getOwnerName()); | |
| userInfo.setName(consumer.getName()); | |
| userInfo.setEmail(consumer.getOwnerEmail()); | |
| return userInfo; | |
| } catch (Exception e) { | |
| // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 | |
| return null; |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
around lines 83 to 104, the code reads the consumer credential from
request.getAttribute("Authorization") and parses it as a long which never
matches the actual OpenAPI header format; instead, read the HTTP Authorization
header via request.getHeader("Authorization"), verify it follows the expected
"Apollo <consumerId>:<signature>" pattern, extract the substring between the
space and the colon as the consumerId token, parse that token with
Long.parseLong, then call consumerService.getConsumerByConsumerId(consumerId);
if the header is missing or malformed return null (without throwing), and ensure
you only catch unexpected exceptions but do not mask header parsing failures
silently.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's the purpose of this PR
Reserve a final cleanup placeholder to consolidate the OpenAPI refactor after PR01–PR09 land. This PR intentionally makes no code changes; it documents
and tracks the final fold-in and removals.
Which issue(s) this PR fixes:
Fixes #5462
Brief changelog
Summary by CodeRabbit
New Features
Chores