[sessionauthentication] Code generation: update services and models#869
[sessionauthentication] Code generation: update services and models#869AdyenAutomationBot wants to merge 1 commit intomainfrom
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 integrates the new session authentication API into the SDK by adding a dedicated service and a comprehensive set of data models. These changes enable programmatic interaction with the session authentication functionalities, streamlining the process of generating and managing session tokens for various Adyen components. 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 new sessionauthentication service with its models and API client, apparently auto-generated from an OpenAPI specification. The changes are extensive, adding many new files. My review focuses on correctness and maintainability.
I've identified a few critical issues that need to be addressed:
- The
toArray()method in the baseResourcemodel class is implemented in a way that prevents correct serialization of child class properties. - The base URL for the new
SessionAuthenticationApiis not compatible with the existing logic for switching between test and live environments, which will cause all requests to be sent to the test environment. - The
ObjectSerializerhas a hardcoded namespace that will cause deserialization of polymorphic types to fail.
Additionally, there is significant code duplication in the toArray() method across many of the new model classes, which I've flagged as a medium-severity maintainability issue.
| // If a discriminator is defined and points to a valid subclass, use it. | ||
| $discriminator = $class::DISCRIMINATOR; | ||
| if (!empty($discriminator) && isset($data->{$discriminator}) && is_string($data->{$discriminator})) { | ||
| $subclass = '\Adyen\Model\\' . $data->{$discriminator}; |
There was a problem hiding this comment.
The namespace for deserializing polymorphic types is hardcoded to \Adyen\Model\. This will fail for models in other namespaces, such as \Adyen\Model\SessionAuthentication\. The namespace should be determined dynamically from the parent class's namespace.
$subclass = substr($class, 0, strrpos($class, '\')) . '\' . $data->{$discriminator};| public function toArray(): array | ||
| { | ||
| $array = []; | ||
| foreach (self::$openAPITypes as $propertyName => $propertyType) { |
There was a problem hiding this comment.
Using self::$openAPITypes here will only serialize properties defined in the Resource class. When toArray() is called on a child class instance, it will not include properties from the child class. To ensure all properties (including inherited ones) are serialized, you should use static::openAPITypes() which resolves to the openAPITypes() method of the called class at runtime.
With this change, child classes like AccountHolderResource will not need to override the toArray() method, and their current implementations can be removed.
foreach (static::openAPITypes() as $propertyName => $propertyType) {| parent::__construct($client); | ||
|
|
||
| // Create the baseUrl based on live/test and optional live-url-prefix | ||
| $this->baseURL = $this->createBaseUrl("https://test.adyen.com/authe/api/v1"); |
There was a problem hiding this comment.
The base URL for the session authentication service is hardcoded in a way that will not work correctly with the createBaseUrl method for the live environment. The createBaseUrl method expects a URL pattern with -test to replace it with -live, but the current URL https://test.adyen.com/authe/api/v1 does not follow this pattern. This will result in all requests being sent to the test environment, even when configured for live.
Please update the URL to follow the established pattern, for example: https://authe-test.adyen.com/api/v1.
$this->baseURL = $this->createBaseUrl("https://authe-test.adyen.com/api/v1");| public function toArray(): array | ||
| { | ||
| $array = []; | ||
| foreach (self::$openAPITypes as $propertyName => $propertyType) { | ||
| $propertyValue = $this[$propertyName]; | ||
| if ($propertyValue !== null) { | ||
| // Check if the property value is an object and has a toArray() method | ||
| if (is_object($propertyValue) && method_exists($propertyValue, 'toArray')) { | ||
| $array[$propertyName] = $propertyValue->toArray(); | ||
| // Check if it's type datetime | ||
| } elseif ($propertyValue instanceof \DateTime) { | ||
| $array[$propertyName] = $propertyValue->format(DATE_ATOM); | ||
| // If it's an array type we should check whether it contains objects and if so call toArray method | ||
| } elseif (is_array($propertyValue)) { | ||
| $array[$propertyName] = array_map(function ($item) { | ||
| return $item instanceof ModelInterface ? $item->toArray() : $item; | ||
| }, $propertyValue); | ||
| } else { | ||
| // Otherwise, directly assign the property value to the array | ||
| $array[$propertyName] = $propertyValue; | ||
| } | ||
| } | ||
| } | ||
| return $array; | ||
| } |
There was a problem hiding this comment.
The toArray() method is duplicated across many model classes in this pull request that do not share a common base class with this logic (e.g., AuthenticationSessionRequest, AuthenticationSessionResponse, Policy, etc.). To improve maintainability and reduce code duplication, consider extracting this method into a reusable trait that these classes can then use.
AdyenAutomationBot
force-pushed
the
sdk-automation/sessionauthentication
branch
4 times, most recently
from
e0537f9 to
9d9a97a
Compare
AdyenAutomationBot
force-pushed
the
sdk-automation/sessionauthentication
branch
4 times, most recently
from
958e6ff to
7d63238
Compare
AdyenAutomationBot
force-pushed
the
sdk-automation/sessionauthentication
branch
from
7d63238 to
61bcea1
Compare
AdyenAutomationBot
force-pushed
the
sdk-automation/sessionauthentication
branch
from
61bcea1 to
1602323
Compare
|
1 participant
This PR contains the automated changes for the
sessionauthenticationservice.The commit history of this PR reflects the
adyen-openapicommits that have been applied.