-
-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: openapi query namespace support not fill item #83
feat: openapi query namespace support not fill item #83
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (3)
29-29
: Approve change with suggestions for improvementThe addition of the
fillItemDetail
parameter aligns with the PR objectives and provides the desired functionality to optionally retrieve item details. However, there are a couple of improvements that could be made:
- Add documentation for the new
fillItemDetail
parameter to clarify its purpose and usage.- Consider adding a deprecated version of the old method signature to maintain backwards compatibility for existing clients.
Here's a suggested implementation with these improvements:
/** * Get namespace information. * * @param appId The application ID * @param env The environment * @param clusterName The cluster name * @param namespaceName The namespace name * @param fillItemDetail Whether to fill item details in the response * @return The namespace information */ OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName, boolean fillItemDetail); /** * @deprecated Use {@link #getNamespace(String, String, String, String, boolean)} instead. */ @Deprecated default OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName) { return getNamespace(appId, env, clusterName, namespaceName, true); }This approach provides clear documentation and maintains backwards compatibility.
31-31
: Approve change with suggestions for improvementThe addition of the
fillItemDetail
parameter to thegetNamespaces
method aligns with the PR objectives and provides the desired functionality to optionally retrieve item details for multiple namespaces. However, similar to thegetNamespace
method, there are a couple of improvements that could be made:
- Add documentation for the new
fillItemDetail
parameter to clarify its purpose and usage.- Consider adding a deprecated version of the old method signature to maintain backwards compatibility for existing clients.
Here's a suggested implementation with these improvements:
/** * Get multiple namespaces information. * * @param appId The application ID * @param env The environment * @param clusterName The cluster name * @param fillItemDetail Whether to fill item details in the response * @return List of namespace information */ List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName, boolean fillItemDetail); /** * @deprecated Use {@link #getNamespaces(String, String, String, boolean)} instead. */ @Deprecated default List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName) { return getNamespaces(appId, env, clusterName, true); }This approach provides clear documentation and maintains backwards compatibility.
Line range hint
1-38
: Overall feedback on changesThe modifications to the
NamespaceOpenApiService
interface successfully implement the desired functionality to optionally retrieve item details when fetching namespace information. This aligns well with the PR objectives and addresses the issue described in #5243.However, to ensure a smooth transition and maintain API usability, consider the following recommendations:
- Add clear documentation for the new
fillItemDetail
parameter in bothgetNamespace
andgetNamespaces
methods.- Implement deprecated versions of the old method signatures to maintain backwards compatibility.
- Update the class-level documentation to mention this new functionality.
These changes will significantly improve the API's usability and ensure a smoother transition for existing clients.
As this change affects a public API, consider updating the API documentation and providing migration guides for users upgrading to this new version. Additionally, it might be beneficial to add examples of how to use these new method signatures in the Apollo documentation.
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (2)
61-61
: Consider adding a test case forfillItemDetail=false
.The test has been updated to use the new method signature with
fillItemDetail=true
, which is good. However, to ensure comprehensive test coverage, it would be beneficial to add another test case that checks the behavior whenfillItemDetail
is set tofalse
.Would you like me to provide an example of how to implement this additional test case?
86-86
: Consider adding a test case forfillItemDetail=false
.The test has been updated to use the new method signature with
fillItemDetail=true
, which is good. However, to ensure comprehensive test coverage, it would be beneficial to add another test case that checks the behavior whenfillItemDetail
is set tofalse
for thegetNamespaces
method.Would you like me to provide an example of how to implement this additional test case?
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiService.java (3)
Line range hint
44-62
: LGTM! Consider adding a deprecated method for backwards compatibility.The changes to
getNamespace
method look good and align with the PR objectives. The newfillItemDetail
parameter is correctly added and used in theOpenApiPathBuilder
.To ensure better backwards compatibility, consider adding a deprecated version of the method:
@Deprecated @Override public OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName) { return getNamespace(appId, env, clusterName, namespaceName, true); }This will allow existing code to continue working while encouraging migration to the new method signature.
Line range hint
73-88
: LGTM! Consider adding a deprecated method for backwards compatibility.The changes to
getNamespaces
method look good and align with the PR objectives. The newfillItemDetail
parameter is correctly added and used in theOpenApiPathBuilder
.To ensure better backwards compatibility, consider adding a deprecated version of the method:
@Deprecated @Override public List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName) { return getNamespaces(appId, env, clusterName, true); }This will allow existing code to continue working while encouraging migration to the new method signature.
Incomplete Updates in Related Classes
The
NamespaceOpenApiService
interface has been successfully updated with the newfillItemDetail
parameter. However, theApolloOpenApiClient
class still contains deprecated method calls togetNamespace
andgetNamespaces
that need to be updated to utilize the new parameters.Please address the following:
- Replace deprecated
getNamespace
andgetNamespaces
calls in theApolloOpenApiClient
class with the updated method signatures that include thefillItemDetail
parameter.- Update all existing calls across the codebase to use the new methods to ensure consistency and leverage the improved functionality.
- Consider removing or properly documenting deprecated methods to prevent future confusion and maintain codebase cleanliness.
🔗 Analysis chain
Line range hint
1-145
: Overall changes look good. Don't forget to update related classes.The modifications to
getNamespace
andgetNamespaces
methods are well-implemented and align with the PR objectives. The newfillItemDetail
parameter allows clients to control whether to retrieve detailed item information, which should improve efficiency as intended.To ensure consistency across the codebase, please make sure to:
- Update the
NamespaceOpenApiService
interface with the new method signatures.- Modify the
ApolloOpenApiClient
class to utilize the new parameter.- Update any existing calls to these methods in other parts of the codebase.
You can use the following script to verify these changes:
This script will help ensure that all necessary updates have been made throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify updates to related classes and usage of new methods # Check for updates in the interface echo "Checking NamespaceOpenApiService interface:" rg --type java "interface NamespaceOpenApiService" -A 10 # Check for updates in ApolloOpenApiClient echo "Checking ApolloOpenApiClient class:" rg --type java "class ApolloOpenApiClient" -A 20 # Check for existing calls to getNamespace and getNamespaces echo "Checking existing calls to getNamespace and getNamespaces:" rg --type java "getNamespace\(|getNamespaces\("Length of output: 22305
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (4)
103-107
: LGTM! Consider adding@Deprecated
annotation.The deprecation of the existing
getNamespaces
method is well-implemented. The comment correctly directs users to the new method, and the implementation ensures backward compatibility.Consider adding the
@Deprecated
annotation to complement the Javadoc@deprecated
tag:+ @Deprecated @deprecated use {@link ApolloOpenApiClient#getNamespaces(String, String, String, boolean)} instead */ public List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName) { return namespaceService.getNamespaces(appId, env, clusterName, true); }
109-114
: LGTM! Consider enhancing the Javadoc.The new
getNamespaces
method with thefillItemDetail
parameter aligns well with the PR objectives. It provides the desired flexibility in retrieving namespace information.Consider enhancing the Javadoc to explain the purpose of the
fillItemDetail
parameter:/** * Get the namespaces + * @param fillItemDetail if true, fill item details for each namespace; if false, only return namespace metadata * @since 2.4.0 */ public List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName, boolean fillItemDetail) { return namespaceService.getNamespaces(appId, env, clusterName, fillItemDetail); }
136-140
: LGTM! Consider adding@Deprecated
annotation.The deprecation of the existing
getNamespace
method is well-implemented. The comment correctly directs users to the new method, and the implementation ensures backward compatibility.Consider adding the
@Deprecated
annotation to complement the Javadoc@deprecated
tag:+ @Deprecated @deprecated use {@link ApolloOpenApiClient#getNamespace(String, String, String, String, boolean)} instead */ public OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName) { return namespaceService.getNamespace(appId, env, clusterName, namespaceName, true); }
142-147
: LGTM! Consider enhancing the Javadoc.The new
getNamespace
method with thefillItemDetail
parameter aligns well with the PR objectives. It provides the desired flexibility in retrieving namespace information for a specific namespace.Consider enhancing the Javadoc to explain the purpose of the
fillItemDetail
parameter:/** * Get the namespace + * @param fillItemDetail if true, fill item details for the namespace; if false, only return namespace metadata * @since 2.4.0 */ public OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName, boolean fillItemDetail) { return namespaceService.getNamespace(appId, env, clusterName, namespaceName, fillItemDetail); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (1 hunks)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (2 hunks)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiService.java (4 hunks)
- apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (3)
76-76
: LGTM: Error handling test updated correctly.The test method has been properly updated to use the new signature of
getNamespace
with the additionalfillItemDetail
parameter. This change maintains the integrity of the error handling test.
101-101
: LGTM: Error handling test for getNamespaces updated correctly.The test method has been properly updated to use the new signature of
getNamespaces
with the additionalfillItemDetail
parameter. This change maintains the integrity of the error handling test for thegetNamespaces
method.
Line range hint
1-160
: Summary of changes and suggestionsThe test file has been successfully updated to accommodate the new
fillItemDetail
parameter in thegetNamespace
andgetNamespaces
methods. The changes maintain existing test coverage and error handling scenarios. However, to ensure comprehensive testing, consider adding test cases for whenfillItemDetail
is set tofalse
for bothgetNamespace
andgetNamespaces
methods.Overall, the changes align well with the PR objectives to support the new parameter for controlling whether to fill item details in namespace queries.
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
Line range hint
103-147
: Overall changes align well with PR objectives.The modifications to
ApolloOpenApiClient
successfully implement the newfillItemDetail
parameter for bothgetNamespaces
andgetNamespace
methods. This aligns perfectly with the PR objective of allowing clients to specify whether to include item information when fetching Namespace data.Key points:
- Backward compatibility is maintained through method overloading and appropriate deprecation.
- New methods are clearly documented with the version they were introduced (2.4.0).
- The implementation delegates to the
namespaceService
, which is expected to handle the new parameter.These changes should effectively address the issue of resource-intensive retrieval of unnecessary item data, as described in the linked issue #5243.
...lo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a feature, please also update the CHANGES.md.
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java
Outdated
Show resolved
Hide resolved
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java
Outdated
Show resolved
Hide resolved
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (3)
36-40
: LGTM: New method aligns with PR objectives.The new
getNamespace
method with thefillItemDetail
parameter aligns well with the PR objectives. The@since
tag is correctly used to indicate the version where this method was introduced.Consider adding a brief description of the
fillItemDetail
parameter in the Javadoc. For example:/** * Retrieves a single namespace * @param fillItemDetail if true, includes detailed item information in the response * @since 2.4.0 */
49-53
: LGTM: New getNamespaces method aligns with PR objectives.The new
getNamespaces
method with thefillItemDetail
parameter aligns well with the PR objectives. The@since
tag is correctly used to indicate the version where this method was introduced.Consider adding a brief description of the
fillItemDetail
parameter in the Javadoc. For example:/** * Retrieves a list of namespaces * @param fillItemDetail if true, includes detailed item information for each namespace in the response * @since 2.4.0 */
29-53
: Summary: Changes align well with PR objectives and maintain backward compatibility.The introduced changes to
NamespaceOpenApiService
successfully address the PR objectives:
- New
fillItemDetail
parameter allows clients to control whether to include detailed item information, potentially improving efficiency.- Backward compatibility is maintained through deprecated methods that delegate to the new implementations.
- New methods are properly versioned with
@since 2.4.0
annotations.The parameter name
fillItemDetail
is slightly different from the proposedfillItems
in the PR objectives, but it's more descriptive and appropriate for the context.To fully realize the benefits of these changes:
- Ensure that the underlying implementation of these methods respects the
fillItemDetail
flag and optimizes data retrieval accordingly.- Update the client-side code (e.g.,
ApolloOpenApiClient
) to utilize these new methods and expose thefillItemDetail
option to end-users.- Consider updating the documentation to explain the performance implications and use cases for the
fillItemDetail
parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGES.md (1 hunks)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGES.md
🧰 Additional context used
🔇 Additional comments (2)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (2)
29-34
: LGTM: Proper deprecation and backward compatibility.The deprecated method is correctly annotated and implemented. It maintains backward compatibility by delegating to the new method with
fillItemDetail
set to true, which preserves the existing behavior.
42-47
: LGTM: Proper deprecation and backward compatibility for getNamespaces.The deprecated
getNamespaces
method is correctly annotated and implemented. It maintains backward compatibility by delegating to the new method withfillItemDetail
set to true, preserving the existing behavior.
1. remove @deprecated 2. add unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
108-110
: Enhance JavaDoc Comments for New MethodsThe new methods lack comprehensive JavaDoc descriptions. Providing detailed JavaDoc for
getNamespaces
andgetNamespace
methods will improve code maintainability and assist developers in understanding the new functionality.Consider adding descriptions of the methods, parameters, and expected behavior. For example:
/** * Retrieves a list of namespaces for the specified application, environment, and cluster. * * @param appId the application ID * @param env the environment * @param clusterName the cluster name * @param fillItemDetail whether to include item details in the response * @return a list of OpenNamespaceDTO objects representing the namespaces * @since 2.4.0 */ public List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName, boolean fillItemDetail) { ... }Also applies to: 138-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java (1 hunks)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (2 hunks)
- apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/NamespaceOpenApiService.java
🧰 Additional context used
🔇 Additional comments (9)
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (4)
40-40
: Declaration offillItemDetail
variableThe
fillItemDetail
variable is correctly declared to control the inclusion of item details in the API calls.
51-51
: Initialization offillItemDetail
Initializing
fillItemDetail
totrue
ensures that existing tests continue to pass with the default behavior.
70-72
: Assertion includesfillItemDetail
parameterThe assertion correctly verifies that the
fillItemDetail
parameter is included in the request URI with the expected value.
113-113
: Assertion intestGetNamespaces
includesfillItemDetail
The assertion correctly checks that the
fillItemDetail
parameter is included in the request URI for thegetNamespaces
method.apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (5)
102-104
: Ensuring Backward Compatibility with Existing MethodThe existing method
getNamespaces(String appId, String env, String clusterName)
remains unchanged. This ensures that existing clients depending on this method will not be affected by the new changes.
110-111
: Addition of OverloadedgetNamespaces
Method withfillItemDetail
ParameterThe new overloaded method
getNamespaces(String appId, String env, String clusterName, boolean fillItemDetail)
provides enhanced functionality by allowing clients to specify whether to include item details. This aligns with the PR objective to improve efficiency when detailed item data is unnecessary.
132-134
: Maintaining ExistinggetNamespace
Method for CompatibilityThe method
getNamespace(String appId, String env, String clusterName, String namespaceName)
remains in the codebase, ensuring that existing integrations continue to function without modification.
140-141
: Introducing NewgetNamespace
Method withfillItemDetail
ParameterThe addition of
getNamespace(String appId, String env, String clusterName, String namespaceName, boolean fillItemDetail)
allows clients to control the inclusion of item details when retrieving a namespace. This enhancement supports the aim to reduce resource consumption when item details are not required.
110-111
: Verify Consistency offillItemDetail
Behavior Across ServicesEnsure that the
fillItemDetail
parameter is consistently implemented and properly handled in the underlyingnamespaceService
methods. This will guarantee that the behavior of including or excluding item details works as intended.You can run the following script to check for the usage of
fillItemDetail
inNamespaceOpenApiService
:Also applies to: 140-141
✅ Verification successful
Consistency of
fillItemDetail
Behavior Verified SuccessfullyThe
fillItemDetail
parameter is consistently implemented and properly handled within theNamespaceOpenApiService
methods. Both method definitions and their usages ensure that the behavior of including or excluding item details functions as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `fillItemDetail` is correctly used in NamespaceOpenApiService methods. # Search for the method definitions in NamespaceOpenApiService rg --type java 'public List<OpenNamespaceDTO> getNamespaces\(.+boolean fillItemDetail\)' src/main/java/com/ctrip/framework/apollo/openapi/client/service/ # Search for the method calls within ApolloOpenApiClient rg --type java 'namespaceService\.getNamespaces\(.*, fillItemDetail\)' src/main/java/com/ctrip/framework/apollo/openapi/client/ # Search for the usage of `fillItemDetail` parameter in the implementation rg --type java 'if.*fillItemDetail' src/main/java/com/ctrip/framework/apollo/openapi/client/service/Length of output: 695
Script:
#!/bin/bash # Description: Locate NamespaceOpenApiService.java in the codebase and verify usage of `fillItemDetail` # Locate the NamespaceOpenApiService.java file fd 'NamespaceOpenApiService\.java' src/main/java/ --type f # Once the file is located, search for `fillItemDetail` usage within it fd 'NamespaceOpenApiService\.java' src/main/java/ --type f | while read filepath; do echo "Checking file: $filepath" rg 'fillItemDetail' "$filepath" doneLength of output: 341
Script:
#!/bin/bash # Description: Locate NamespaceOpenApiService.java in the correct codebase directory and verify usage of `fillItemDetail` # Define the base directory based on the review comment BASE_DIR="apollo-openapi/src/main/java/" # Check if the base directory exists if [ ! -d "$BASE_DIR" ]; then echo "Error: Directory '$BASE_DIR' does not exist." exit 1 fi # Locate the NamespaceOpenApiService.java file within the correct base directory fd 'NamespaceOpenApiService\.java' "$BASE_DIR" --type f # Once the file is located, search for `fillItemDetail` usage within it fd 'NamespaceOpenApiService\.java' "$BASE_DIR" --type f | while read filepath; do echo "Checking file: $filepath" rg 'fillItemDetail' "$filepath" doneLength of output: 1831
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Outdated
Show resolved
Hide resolved
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Show resolved
Hide resolved
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Outdated
Show resolved
Hide resolved
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What's the purpose of this PR
XXXXX
Which issue(s) this PR fixes:
Fixes apolloconfig/apollo/issues/5243
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.Summary by CodeRabbit
New Features
Bug Fixes
Tests