-
-
Notifications
You must be signed in to change notification settings - Fork 77
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: Provide a new open APl to return the organization list(#5349) #102
Conversation
WalkthroughThis pull request introduces a new organization service within the Apollo OpenAPI framework. A new interface is added to declare a method for retrieving organization data, and a corresponding implementation is provided to perform an HTTP GET call and parse the response into DTOs. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as ApolloOpenApiClient
participant O as OrganizationOpenApiService
participant H as HTTP Service
C->>A: getOrganizations()
A->>O: getOrganizations()
O->>H: GET /organizations
H-->>O: JSON Response
O-->>A: List<OpenOrganizationDto>
A-->>C: List<OpenOrganizationDto>
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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 (
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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
🧹 Nitpick comments (2)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
89-94
: Consider renaming method for consistency.The method is named
getOrganization()
(singular) but returns a list of organizations and callsgetOrganizations()
(plural) internally. Consider renaming for consistency:- public List<OpenOrganizationDto> getOrganization() { + public List<OpenOrganizationDto> getOrganizations() { return organizationOpenService.getOrganizations(); }apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (1)
39-50
: API implementation looks good, but consider more specific error handling.The implementation correctly builds the API path and handles the HTTP request. The error handling catches all exceptions but could be more specific.
Consider providing more specific error messages based on exception types:
try (CloseableHttpResponse response = get(pathBuilder)) { return gson.fromJson(EntityUtils.toString(response.getEntity()), ORGANIZATIONS_DTO_LIST_TYPE); - } catch (Throwable ex) { - throw new RuntimeException("get organizations information failed", ex); + } catch (IOException ex) { + throw new RuntimeException("Failed to read organizations response from API", ex); + } catch (JsonSyntaxException ex) { + throw new RuntimeException("Failed to parse organizations JSON response", ex); + } catch (Throwable ex) { + throw new RuntimeException("Unexpected error when getting organizations information", ex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java
(1 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java
(5 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
(1 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/dto/OpenOrganizationDto.java
(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java (2)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/dto/OpenOrganizationDto.java (1)
OpenOrganizationDto
(20-47)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (1)
OrganizationOpenApiService
(30-51)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (1)
OrganizationOpenApiService
(30-51)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/dto/OpenOrganizationDto.java (1)
OpenOrganizationDto
(20-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (8)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/dto/OpenOrganizationDto.java (1)
20-46
: Looks good - well-structured DTO implementation.The
OpenOrganizationDto
class follows standard Java bean conventions with proper getters, setters, and toString implementation. The fields are appropriately named and the class is focused on a single responsibility.apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java (1)
23-26
: Interface design looks good.The interface is clean, concise, and follows the single responsibility principle. It properly defines the contract for fetching organizations with an appropriately typed return value.
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (4)
25-25
: Import statement looks good.The import for the new service is correctly added.
49-49
: Field declaration is appropriate.The service field is correctly declared as private final, consistent with other service declarations in this class.
59-60
: Spacing/indentation improvement.The indentation is now more consistent with the rest of the code.
68-68
: Service initialization looks good.The organization service is properly instantiated with the same parameters as other services.
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (2)
30-35
: Service implementation looks good.The class properly extends AbstractOpenApiService and implements the OrganizationOpenApiService interface. The constructor correctly calls the super constructor with all required parameters.
36-38
: Type token for deserialization is well-implemented.The ORGANIZATIONS_DTO_LIST_TYPE constant is correctly defined using TypeToken for JSON deserialization of the list of DTOs.
I have read the CLA Document and I hereby sign the CLA |
@nobodyiam PTAL |
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.
Please also update the CHANGES.md
...openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java
Show resolved
Hide resolved
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
.../main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
Outdated
Show resolved
Hide resolved
…i/client/ApolloOpenApiClient.java Co-authored-by: Jason Song <[email protected]>
…i/api/OrganizationOpenApiService.java Co-authored-by: Jason Song <[email protected]>
…i/client/ApolloOpenApiClient.java Co-authored-by: Jason Song <[email protected]>
…i/client/service/OrganizationOpenApiService.java Co-authored-by: Jason Song <[email protected]>
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
🧹 Nitpick comments (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
49-49
: Consider renaming for consistency with other service fieldsThe field name
organizationOpenService
doesn't follow the same naming pattern as other service fields likeappService
,itemService
, etc. Consider renaming it toorganizationService
for better consistency.- private final OrganizationOpenApiService organizationOpenService; + private final OrganizationOpenApiService organizationService;Remember to update the reference in the constructor and the
getOrganizations()
method as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.md
(1 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java
(1 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java
(4 hunks)apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGES.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java
- apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
🧰 Additional context used
🧬 Code Definitions (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java (1)
OrganizationOpenApiService
(30-52)
🔇 Additional comments (1)
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiClient.java (1)
89-94
: LGTM! New method follows established patternsThe implementation correctly delegates to the service layer and follows the same pattern as other methods in this class.
Thank you for your review and suggestions. I have updated the |
...openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/OrganizationOpenApiService.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
Outdated
Show resolved
Hide resolved
apollo-openapi/src/main/java/com/ctrip/framework/apollo/openapi/dto/OpenOrganizationDto.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/openapi/client/service/OrganizationOpenApiService.java
Outdated
Show resolved
Hide resolved
…i/client/service/OrganizationOpenApiService.java
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
feat: Provide a new open APl to return the organization list(#5349)
Which issue(s) this PR fixes:
#5349
Brief changelog
After updating the project, we can use this method like this way:
Summary by CodeRabbit