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 (
|
![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
🧹 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
fillItemDetailparameter 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
fillItemDetailparameter 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
fillItemDetailparameter to thegetNamespacesmethod aligns with the PR objectives and provides the desired functionality to optionally retrieve item details for multiple namespaces. However, similar to thegetNamespacemethod, there are a couple of improvements that could be made:
- Add documentation for the new
fillItemDetailparameter 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
NamespaceOpenApiServiceinterface 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
fillItemDetailparameter in bothgetNamespaceandgetNamespacesmethods.- 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 whenfillItemDetailis 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 whenfillItemDetailis set tofalsefor thegetNamespacesmethod.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
getNamespacemethod look good and align with the PR objectives. The newfillItemDetailparameter 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
getNamespacesmethod look good and align with the PR objectives. The newfillItemDetailparameter 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
NamespaceOpenApiServiceinterface has been successfully updated with the newfillItemDetailparameter. However, theApolloOpenApiClientclass still contains deprecated method calls togetNamespaceandgetNamespacesthat need to be updated to utilize the new parameters.Please address the following:
- Replace deprecated
getNamespaceandgetNamespacescalls in theApolloOpenApiClientclass with the updated method signatures that include thefillItemDetailparameter.- 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
getNamespaceandgetNamespacesmethods are well-implemented and align with the PR objectives. The newfillItemDetailparameter 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
NamespaceOpenApiServiceinterface with the new method signatures.- Modify the
ApolloOpenApiClientclass 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@Deprecatedannotation.The deprecation of the existing
getNamespacesmethod is well-implemented. The comment correctly directs users to the new method, and the implementation ensures backward compatibility.Consider adding the
@Deprecatedannotation to complement the Javadoc@deprecatedtag:+ @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
getNamespacesmethod with thefillItemDetailparameter 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
fillItemDetailparameter:/** * 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@Deprecatedannotation.The deprecation of the existing
getNamespacemethod is well-implemented. The comment correctly directs users to the new method, and the implementation ensures backward compatibility.Consider adding the
@Deprecatedannotation to complement the Javadoc@deprecatedtag:+ @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
getNamespacemethod with thefillItemDetailparameter 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
fillItemDetailparameter:/** * 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
getNamespacewith the additionalfillItemDetailparameter. 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
getNamespaceswith the additionalfillItemDetailparameter. This change maintains the integrity of the error handling test for thegetNamespacesmethod.
Line range hint
1-160: Summary of changes and suggestionsThe test file has been successfully updated to accommodate the new
fillItemDetailparameter in thegetNamespaceandgetNamespacesmethods. The changes maintain existing test coverage and error handling scenarios. However, to ensure comprehensive testing, consider adding test cases for whenfillItemDetailis set tofalsefor bothgetNamespaceandgetNamespacesmethods.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
ApolloOpenApiClientsuccessfully implement the newfillItemDetailparameter for bothgetNamespacesandgetNamespacemethods. 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
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
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.
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
getNamespacemethod with thefillItemDetailparameter aligns well with the PR objectives. The@sincetag is correctly used to indicate the version where this method was introduced.Consider adding a brief description of the
fillItemDetailparameter 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
getNamespacesmethod with thefillItemDetailparameter aligns well with the PR objectives. The@sincetag is correctly used to indicate the version where this method was introduced.Consider adding a brief description of the
fillItemDetailparameter 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
NamespaceOpenApiServicesuccessfully address the PR objectives:
- New
fillItemDetailparameter 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.0annotations.The parameter name
fillItemDetailis slightly different from the proposedfillItemsin 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
fillItemDetailflag and optimizes data retrieval accordingly.- Update the client-side code (e.g.,
ApolloOpenApiClient) to utilize these new methods and expose thefillItemDetailoption to end-users.- Consider updating the documentation to explain the performance implications and use cases for the
fillItemDetailparameter.
📜 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
fillItemDetailset to true, which preserves the existing behavior.
42-47: LGTM: Proper deprecation and backward compatibility for getNamespaces.The deprecated
getNamespacesmethod is correctly annotated and implemented. It maintains backward compatibility by delegating to the new method withfillItemDetailset to true, preserving the existing behavior.
1. remove @deprecated 2. add unit test
There was a problem hiding this comment.
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
getNamespacesandgetNamespacemethods 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 offillItemDetailvariableThe
fillItemDetailvariable is correctly declared to control the inclusion of item details in the API calls.
51-51: Initialization offillItemDetailInitializing
fillItemDetailtotrueensures that existing tests continue to pass with the default behavior.
70-72: Assertion includesfillItemDetailparameterThe assertion correctly verifies that the
fillItemDetailparameter is included in the request URI with the expected value.
113-113: Assertion intestGetNamespacesincludesfillItemDetailThe assertion correctly checks that the
fillItemDetailparameter is included in the request URI for thegetNamespacesmethod.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 OverloadedgetNamespacesMethod withfillItemDetailParameterThe 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 ExistinggetNamespaceMethod 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 NewgetNamespaceMethod withfillItemDetailParameterThe 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 offillItemDetailBehavior Across ServicesEnsure that the
fillItemDetailparameter is consistently implemented and properly handled in the underlyingnamespaceServicemethods. 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
fillItemDetailinNamespaceOpenApiService:Also applies to: 140-141
✅ Verification successful
Consistency of
fillItemDetailBehavior Verified SuccessfullyThe
fillItemDetailparameter is consistently implemented and properly handled within theNamespaceOpenApiServicemethods. 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
Show resolved
Hide resolved
...test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java
Show resolved
Hide resolved
2 participants
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 testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes
Tests