[capital] Code generation: update services and models #1664
Conversation
Summary of ChangesHello @AdyenAutomationBot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Adyen Capital API by introducing a wide array of new models and services. These additions enable more granular management of financial grants, disbursements, and diverse international bank account identifications, reflecting an updated OpenAPI specification for the Capital service. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a large number of new model and service classes for the capital service, apparently generated from an OpenAPI specification. My review has identified a few systematic issues in the generated code that should be addressed in the generator templates to improve code quality and prevent potential bugs.
Specifically:
- The
equals()andhashCode()methods in many model classes incorrectly include internal state flags (isSet...), which violates their contracts and can cause issues with collections. This is a critical issue. - A required query parameter in
GrantsApi.getAllGrantsis not being enforced, which could lead to invalid API requests. - There's an inconsistency in how unknown enum values are handled during deserialization; some enums return
nullwhile others throw an exception. The latter is preferred for robustness.
I've provided specific examples and suggestions for these issues. Addressing them in the generator will ensure all current and future generated code is correct and consistent.
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| AULocalAccountIdentification auLocalAccountIdentification = (AULocalAccountIdentification) o; | ||
| return Objects.equals(this.accountNumber, auLocalAccountIdentification.accountNumber) | ||
| && Objects.equals(this.isSetAccountNumber, auLocalAccountIdentification.isSetAccountNumber) | ||
| && Objects.equals(this.bsbCode, auLocalAccountIdentification.bsbCode) | ||
| && Objects.equals(this.isSetBsbCode, auLocalAccountIdentification.isSetBsbCode) | ||
| && Objects.equals(this.type, auLocalAccountIdentification.type) | ||
| && Objects.equals(this.isSetType, auLocalAccountIdentification.isSetType) | ||
| && super.equals(o); | ||
| } |
There was a problem hiding this comment.
The equals() method incorrectly includes the isSet... flags in its comparison. These flags are likely intended for serialization logic and should not affect object equality. Two objects with the same field values should be considered equal regardless of how those values were set. Including these flags violates the equals contract and can lead to unexpected behavior, especially when using these objects in collections like HashSet. This issue appears to be systematic across many of the generated model classes and should ideally be fixed in the code generator template.
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
AULocalAccountIdentification auLocalAccountIdentification = (AULocalAccountIdentification) o;
return Objects.equals(this.accountNumber, auLocalAccountIdentification.accountNumber)
&& Objects.equals(this.bsbCode, auLocalAccountIdentification.bsbCode)
&& Objects.equals(this.type, auLocalAccountIdentification.type)
&& super.equals(o);
}| public int hashCode() { | ||
| return Objects.hash( | ||
| accountNumber, | ||
| isSetAccountNumber, | ||
| bsbCode, | ||
| isSetBsbCode, | ||
| type, | ||
| isSetType, | ||
| super.hashCode()); | ||
| } |
There was a problem hiding this comment.
The hashCode() method incorrectly includes the isSet... flags in its hash calculation. Similar to the equals() method, these flags should not affect the hash code of the object. The hash code should be based on the object's actual field values. Including these flags violates the hashCode contract and can lead to issues with hash-based collections. This systematic issue should be addressed in the code generator.
@Override
public int hashCode() {
return Objects.hash(
accountNumber,
bsbCode,
type,
super.hashCode());
}| // Add query params | ||
| Map<String, String> queryParams = new HashMap<>(); | ||
| if (counterpartyAccountHolderId != null) { | ||
| queryParams.put("counterpartyAccountHolderId", counterpartyAccountHolderId); | ||
| } |
There was a problem hiding this comment.
The Javadoc for this method indicates that the counterpartyAccountHolderId parameter is required. However, the implementation only checks if it's not null before adding it to the query parameters, but doesn't enforce its presence. This can lead to requests being sent without a required parameter. It would be more robust to throw an IllegalArgumentException if the required parameter is null, similar to how path parameters are handled in other methods of this API client.
| // Add query params | |
| Map<String, String> queryParams = new HashMap<>(); | |
| if (counterpartyAccountHolderId != null) { | |
| queryParams.put("counterpartyAccountHolderId", counterpartyAccountHolderId); | |
| } | |
| // Add query params | |
| if (counterpartyAccountHolderId == null) { | |
| throw new IllegalArgumentException("Please provide the counterpartyAccountHolderId query parameter"); | |
| } | |
| Map<String, String> queryParams = new HashMap<>(); | |
| queryParams.put("counterpartyAccountHolderId", counterpartyAccountHolderId); |
There was a problem hiding this comment.
@gcatanese @galesky-a I checked that since this is a query parameter, we don't validate as paths (like here). I'm assuming this type of validation happens in the backend since this is the existing behavior for query params and it's what we have in the template. Updating the template could be an option but: would it be necessary? if so, we need a general update across the codebase. Let me know WDYT.
There was a problem hiding this comment.
for completeness: here is also a required queryParam that is handled similar like this:
| for (ContractTypeEnum b : ContractTypeEnum.values()) { | ||
| if (b.value.equals(value)) { | ||
| return b; | ||
| } | ||
| } | ||
| // handling unexpected value | ||
| LOG.warning( | ||
| "ContractTypeEnum: unexpected enum value '" | ||
| + value | ||
| + "' - Supported values are " | ||
| + Arrays.toString(ContractTypeEnum.values())); | ||
| return null; |
There was a problem hiding this comment.
The fromValue method for ContractTypeEnum returns null for unexpected values, which can lead to NullPointerExceptions in downstream code. This is also inconsistent with other enums in the generated code (like AdditionalBankIdentificationTypes) which throw an IllegalArgumentException. It's better to fail fast by throwing an exception for unknown enum values to ensure consistency and robustness.
| for (ContractTypeEnum b : ContractTypeEnum.values()) { | |
| if (b.value.equals(value)) { | |
| return b; | |
| } | |
| } | |
| // handling unexpected value | |
| LOG.warning( | |
| "ContractTypeEnum: unexpected enum value '" | |
| + value | |
| + "' - Supported values are " | |
| + Arrays.toString(ContractTypeEnum.values())); | |
| return null; | |
| for (ContractTypeEnum b : ContractTypeEnum.values()) { | |
| if (b.value.equals(value)) { | |
| return b; | |
| } | |
| } | |
| throw new IllegalArgumentException("Unexpected value '" + value + "'"); |
| for (CodeEnum b : CodeEnum.values()) { | ||
| if (b.value.equals(value)) { | ||
| return b; | ||
| } | ||
| } | ||
| // handling unexpected value | ||
| LOG.warning( | ||
| "CodeEnum: unexpected enum value '" | ||
| + value | ||
| + "' - Supported values are " | ||
| + Arrays.toString(CodeEnum.values())); | ||
| return null; |
There was a problem hiding this comment.
The fromValue method for CodeEnum returns null for unexpected values. This is inconsistent with other enums in the generated code which throw an IllegalArgumentException. Returning null can lead to NullPointerExceptions in downstream code. It's better to fail fast by throwing an exception for unknown enum values to ensure consistency and robustness.
| for (CodeEnum b : CodeEnum.values()) { | |
| if (b.value.equals(value)) { | |
| return b; | |
| } | |
| } | |
| // handling unexpected value | |
| LOG.warning( | |
| "CodeEnum: unexpected enum value '" | |
| + value | |
| + "' - Supported values are " | |
| + Arrays.toString(CodeEnum.values())); | |
| return null; | |
| for (CodeEnum b : CodeEnum.values()) { | |
| if (b.value.equals(value)) { | |
| return b; | |
| } | |
| } | |
| throw new IllegalArgumentException("Unexpected value '" + value + "'"); |
This PR contains the automated changes for the
capitalservice.The commit history of this PR reflects the
adyen-openapicommits that have been applied.