Provide a new configfiles API to return the raw content of configuration files directly #5336
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes introduce a new endpoint within the configuration controller to handle raw configuration file requests. This includes adding a new enum value ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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 (3)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java (1)
194-221: LGTM! Comprehensive test coverage for raw configuration endpoint.The test method thoroughly validates:
- Response status code
- Content type header for JSON format
- Response body content
However, consider adding more test cases to cover other formats:
+@Test +public void testQueryConfigAsRawYaml() throws Exception { + String someKey = "someKey"; + String someValue = "someValue"; + String someWatchKey = "someWatchKey"; + Set<String> watchKeys = Sets.newHashSet(someWatchKey); + + ApolloConfig someApolloConfig = mock(ApolloConfig.class); + when(configController + .queryConfig(someAppId, someClusterName, someNamespace, someDataCenter, "-1", someClientIp, someClientLabel,null, + someRequest, someResponse)).thenReturn(someApolloConfig); + when(someApolloConfig.getNamespaceName()).thenReturn(someNamespace + ".yaml"); + String yamlContent = "someKey: someValue"; + when(someApolloConfig.getConfigurations()).thenReturn(ImmutableMap.of("content", yamlContent)); + when(watchKeysUtil + .assembleAllWatchKeys(someAppId, someClusterName, someNamespace, someDataCenter)) + .thenReturn(watchKeys); + + ResponseEntity<String> response = + configFileController + .queryConfigAsRaw(someAppId, someClusterName, someNamespace + ".yaml", someDataCenter, + someClientIp, someClientLabel, someRequest, someResponse); + + assertEquals(HttpStatus.OK, response.getStatusCode()); + assertEquals("application/yaml;charset=UTF-8", response.getHeaders().getContentType().toString()); + assertEquals(yamlContent, response.getBody()); +}apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java (1)
170-206: LGTM! Comprehensive implementation of raw configuration endpoint.The implementation correctly:
- Handles different namespace formats
- Sets appropriate response headers
- Returns raw content
However, consider adding error handling for unsupported formats:
ConfigFileFormat format = determineNamespaceFormat(namespace); +if (format == ConfigFileFormat.UNKNOWN) { + return new ResponseEntity<>("Unsupported namespace format", plainTextResponseHeaders, HttpStatus.BAD_REQUEST); +} HttpHeaders responseHeaders;docs/zh/client/other-language-client-user-guide.md (1)
34-50: LGTM! Clear and comprehensive documentation.The documentation:
- Clearly explains the response format for different namespace types
- Provides helpful examples
- Includes information about the raw configuration endpoint
However, consider adding a note about error handling:
Add information about potential error responses:
> 通过`{config_server_url}/configfiles/raw/{appId}/{clusterName}/{namespaceName}?ip={clientIp}`可以获取到原始的配置内容，不会进行转义。 +> 注意：如果配置不存在或格式不支持，接口会返回相应的错误状态码和错误信息。🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java(8 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java(3 hunks)docs/zh/client/other-language-client-user-guide.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/zh/client/other-language-client-user-guide.md
50-50: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (4)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java (1)
97-97: Improve mock behavior to handle namespace suffixes.The change from exact matching to
startsWithmatcher is necessary to handle namespace suffixes (e.g.,.json,.yaml) in the raw configuration endpoint. This aligns with the implementation inConfigFileController.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java (3)
71-74: LGTM! Well-organized response headers for different formats.The headers are correctly initialized with appropriate content types and character encoding.
Also applies to: 112-119
303-311: Add validation for content field.For non-properties formats, the method assumes the 'content' field exists in configurations.
Add null check to prevent potential
NullPointerException:if (format == ConfigFileFormat.Properties) { Properties properties = new Properties(); properties.putAll(apolloConfig.getConfigurations()); return PropertiesUtil.toString(properties); } -return apolloConfig.getConfigurations().get("content"); +String content = apolloConfig.getConfigurations().get("content"); +if (content == null) { + throw new IllegalStateException("Content field not found in non-properties namespace"); +} +return content;
324-333: LGTM! Robust format detection.The method correctly handles case-insensitive format detection and defaults to Properties format.
Shawyeok
force-pushed
the
feature/api-return-raw-content-of-namespace
branch
from
d0dd22e to
6ffd4ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java (1)
324-333: Consider adding a comment explaining the default format choice.The implementation is solid, but it would be helpful to add a comment explaining why Properties is chosen as the default format.
ConfigFileFormat determineNamespaceFormat(String namespaceName) { String lowerCase = namespaceName.toLowerCase(); for (ConfigFileFormat format : ConfigFileFormat.values()) { if (lowerCase.endsWith("." + format.getValue())) { return format; } } + // Default to Properties format for backward compatibility return ConfigFileFormat.Properties; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java(8 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java(3 hunks)docs/zh/client/other-language-client-user-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/zh/client/other-language-client-user-guide.md
50-50: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (5)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java (4)
71-74: LGTM! Well-structured response headers initialization.The response headers are properly initialized with appropriate content types for different formats. The rename from
propertiesResponseHeaderstoplainTextResponseHeadersbetter reflects its usage.Also applies to: 112-119
170-206: LGTM! Well-implemented raw configuration endpoint.The implementation follows consistent patterns with proper error handling and content-type selection based on the namespace format.
303-311: LGTM! Clean implementation of raw content retrieval.The method properly handles both Properties format and other formats, with clear separation of concerns.
358-358: LGTM! Consistent enum extension.The addition of RAW format follows the existing pattern and naming convention.
docs/zh/client/other-language-client-user-guide.md (1)
34-50: LGTM! Clear and comprehensive documentation update.The documentation effectively explains the response format differences between properties and non-properties namespaces, and clearly introduces the new raw configuration endpoint.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
| @@ -31,14 +31,23 @@ | |||
| ### 1.2.2 Http接口返回格式 | |||
| 该Http接口返回的是JSON格式、UTF-8编码，包含了对应namespace中所有的配置项。 | |||
|
|
|||
| 返回内容Sample如下： | |||
There was a problem hiding this comment.
Please update the English version of the document as well.
There was a problem hiding this comment.
Thanks for notice, updated.
…iguration files directly
Shawyeok
force-pushed
the
feature/api-return-raw-content-of-namespace
branch
from
6ffd4ad to
3b7a1c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/en/client/other-language-client-user-guide.md (1)
53-53: Enhance documentation for the new raw configuration endpoint.While the new endpoint is documented, consider these improvements:
- Move it out of the blockquote to make it more prominent
- Add a sample response to demonstrate the raw format
- Add a brief explanation of when to use this endpoint vs. the JSON endpoint
Consider restructuring like this:
- > You can get the raw configuration content without escaping via `{config_server_url}/configfiles/raw/{appId}/{clusterName}/{namespaceName}?ip={clientIp}`. + ### Raw Configuration Content + + To get the raw configuration content without JSON escaping, use: + + ``` + {config_server_url}/configfiles/raw/{appId}/{clusterName}/{namespaceName}?ip={clientIp} + ``` + + Sample response for a properties file: + ``` + portal.elastic.document.type=biz + portal.elastic.cluster.name=hermes-es-fws + ``` + + This endpoint is useful when you need to: + - Process the configuration in its original format + - Avoid JSON escaping overhead + - Directly use the configuration file content
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.md(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java(8 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java(3 hunks)docs/en/client/other-language-client-user-guide.md(2 hunks)docs/zh/client/other-language-client-user-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/en/client/other-language-client-user-guide.md
54-54: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
docs/zh/client/other-language-client-user-guide.md
50-50: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (6)
CHANGES.md (1)
8-8: LGTM! The changelog entry is well-formatted and accurate.The entry follows the standard format, properly links to the PR, and accurately describes the new configfiles API feature. The description aligns well with the PR objectives.
docs/zh/client/other-language-client-user-guide.md (3)
34-47: LGTM! Clear explanation of response formats.The documentation clearly explains how the response format differs between properties and non-properties namespace types, with helpful JSON examples for both cases.
49-49: LGTM! Helpful addition about raw configuration access.The note about accessing raw configuration content through a different URL endpoint is a valuable addition, especially for users who need unescaped content.
34-49: Update the English version of the documentation.Please ensure these changes are also reflected in the English version of the documentation.
docs/en/client/other-language-client-user-guide.md (2)
37-44: LGTM! Clear and well-documented return format.The documentation clearly explains the return format for properties namespace with a well-structured JSON example.
46-51: LGTM! Clear distinction between namespace types.The documentation effectively illustrates how non-properties namespace responses differ, with content properly escaped in JSON format.
What's the purpose of this PR
In some application scenarios, if Apollo can provide an API to directly return the raw content of configuration files, users can reference a ConfigService URL directly without the need for an additional proxy URL. See details at #5327
Which issue(s) this PR fixes:
Fixes #5327
Brief changelog
Provide a new configfiles API to return the raw content of configuration files directly
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
New Features
Documentation
Tests