[checkout] Code generation: update services and models#901
[checkout] Code generation: update services and models#901AdyenAutomationBot wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello, 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 delivers an automated update to the Adyen Checkout API client library, incorporating the latest service and model definitions from the 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. 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 pull request introduces new PHP models for Alma, Australian Direct Debit, and Klarna network payment details, and updates several existing models to include new properties, refine documentation, and adjust enum values. Key changes include adding an 'amount' property to 'CheckoutForwardRequest', 'feeType', 'bankBranchCode', and 'klarnaNetworkData' to 'CheckoutPaymentMethod', and a 'store' property to 'DonationCampaignsRequest' and 'DonationPaymentRequest'. The 'InvalidField' model saw property reordering, and 'ShopperTaxInfo' had property renames. 'PaymentDetails' now includes 'TYPE_WERO' and removes 'TYPE_ALMA'. 'ResponseAdditionalDataCard' and 'ResponseAdditionalDataCommon' gained 'cardAltID' and 'networkProcessingMode' respectively. 'Result' now includes a 'NOT_REQUIRED' status, and 'TokenMandate' was extended with several new fields related to recurring payments. Feedback from the review highlights two main areas for improvement: the 'listInvalidProperties' method in newly added models is incomplete, as it fails to validate non-nullable properties against null values, potentially leading to invalid model states; and the enum setters in these models currently use 'error_log' for validation failures, which is suboptimal for debugging, with a recommendation to throw an '\InvalidArgumentException' instead.
| public function listInvalidProperties() | ||
| { | ||
| $invalidProperties = []; | ||
|
|
||
| $allowedValues = $this->getFeeTypeAllowableValues(); | ||
| if (!is_null($this->container['feeType']) && !in_array($this->container['feeType'], $allowedValues, true)) { | ||
| $invalidProperties[] = sprintf( | ||
| "invalid value '%s' for 'feeType', must be one of '%s'", | ||
| $this->container['feeType'], | ||
| implode("', '", $allowedValues) | ||
| ); | ||
| } | ||
|
|
||
| $allowedValues = $this->getTypeAllowableValues(); | ||
| if (!is_null($this->container['type']) && !in_array($this->container['type'], $allowedValues, true)) { | ||
| $invalidProperties[] = sprintf( | ||
| "invalid value '%s' for 'type', must be one of '%s'", | ||
| $this->container['type'], | ||
| implode("', '", $allowedValues) | ||
| ); | ||
| } | ||
|
|
||
| return $invalidProperties; | ||
| } |
There was a problem hiding this comment.
The listInvalidProperties method is incomplete. It validates the enum values for feeType and type, but it doesn't check if these properties are null. According to the $openAPINullables array, all properties in this model are non-nullable, so they should be validated against null. This could lead to invalid model states being considered valid.
This issue appears in other new models as well, such as DirectDebitAuDetails.php.
public function listInvalidProperties()
{
$invalidProperties = [];
if ($this->container['checkoutAttemptId'] === null) {
$invalidProperties[] = "'checkoutAttemptId' can't be null";
}
if ($this->container['feeType'] === null) {
$invalidProperties[] = "'feeType' can't be null";
}
$allowedValues = $this->getFeeTypeAllowableValues();
if (!is_null($this->container['feeType']) && !in_array($this->container['feeType'], $allowedValues, true)) {
$invalidProperties[] = sprintf(
"invalid value '%s' for 'feeType', must be one of '%s'",
$this->container['feeType'],
implode("', '", $allowedValues)
);
}
if ($this->container['sdkData'] === null) {
$invalidProperties[] = "'sdkData' can't be null";
}
if ($this->container['type'] === null) {
$invalidProperties[] = "'type' can't be null";
}
$allowedValues = $this->getTypeAllowableValues();
if (!is_null($this->container['type']) && !in_array($this->container['type'], $allowedValues, true)) {
$invalidProperties[] = sprintf(
"invalid value '%s' for 'type', must be one of '%s'",
$this->container['type'],
implode("', '", $allowedValues)
);
}
return $invalidProperties;
}| public function setFeeType($feeType) | ||
| { | ||
| $allowedValues = $this->getFeeTypeAllowableValues(); | ||
| if (!in_array($feeType, $allowedValues, true)) { | ||
| error_log( | ||
| sprintf( | ||
| "feeType: unexpected enum value '%s' - Supported values are [%s]", | ||
| $feeType, | ||
| implode(', ', $allowedValues) | ||
| ) | ||
| ); | ||
| } | ||
| $this->container['feeType'] = $feeType; | ||
|
|
||
| return $this; | ||
| } |
There was a problem hiding this comment.
Using error_log for validation failures in a setter is not ideal because it fails silently (from the perspective of the calling code) and still allows an invalid value to be set. This can make debugging difficult. It's better to throw an \InvalidArgumentException to provide immediate feedback and prevent the object from entering an invalid state. This comment also applies to setType() in this file and similar enum setters in other models.
public function setFeeType($feeType)
{
$allowedValues = $this->getFeeTypeAllowableValues();
if (!is_null($feeType) && !in_array($feeType, $allowedValues, true)) {
throw new \InvalidArgumentException(sprintf(
"Invalid value for 'feeType', must be one of '%s'",
implode("', '", $allowedValues)
));
}
$this->container['feeType'] = $feeType;
return $this;
}
AdyenAutomationBot
force-pushed
the
sdk-automation/checkout
branch
2 times, most recently
from
55ff810 to
610ec9a
Compare
AdyenAutomationBot
force-pushed
the
sdk-automation/checkout
branch
from
610ec9a to
3a4d158
Compare
AdyenAutomationBot
force-pushed
the
sdk-automation/checkout
branch
from
3a4d158 to
9257db0
Compare
|
1 participant
This PR contains the automated changes for the
checkoutservice.The commit history of this PR reflects the
adyen-openapicommits that have been applied.