OpenAPIGenerator upgrade [development-branch] to v7.16.0 for Adyen's .NET Library #1214
Conversation
…ts (#1207) * Added /Core classes and tests in preparations for the v7-generator-upgrade * Remove workflow.yml for .NET6.0 * Include updated core classes for review * Fix workflow file to target current HttpRequestMessageExtension * DateOnlyNullable returns null when the format is not supported
Kwok-he-Chu
changed the title
Summary of ChangesHello @Kwok-he-Chu, 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 represents a significant architectural and infrastructural update for the Adyen .NET library. It upgrades the underlying OpenAPI Generator to version 7.16.0 and consolidates the target framework to .NET 8.0, leveraging modern .NET features. The changes introduce a new '/Core' folder for better modularity, enhance JSON serialization with native System.Text.Json support, and integrate resilience patterns via the Polly library. These updates aim to improve code generation, maintainability, and the overall robustness of the SDK. Highlights
Ignored Files
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 pull request is a significant upgrade, moving the library to .NET 8, introducing a new core structure, and improving serialization with System.Text.Json. The changes are extensive and well-organized. My review focuses on improving the robustness of the new utility classes, enhancing test quality, and ensuring the code is resilient to repeated calls. I've identified several areas where methods could fail if called multiple times and suggested fixes to make them idempotent. Additionally, I've pointed out some issues in the new tests, such as incorrect assertions and misleading test names, along with suggestions to improve them.
| // Add application name if set. | ||
| if (!string.IsNullOrWhiteSpace(ApplicationName)) | ||
| { | ||
| httpRequestMessage.Headers.Add("UserAgent", $"{ApplicationName} {AdyenLibraryName}/{AdyenLibraryVersion}"); |
There was a problem hiding this comment.
Calling Headers.Add will throw an ArgumentException if the header already exists. The corresponding test Given_AddUserAgentToHeaders_When_Called_MultipleTimes_Should_Not_Throw_Any_Exceptions expects that no exception is thrown on multiple calls. To make this method idempotent, you should use TryAddWithoutValidation which does not throw if the header is already present.
httpRequestMessage.Headers.TryAddWithoutValidation("UserAgent", $"{ApplicationName} {AdyenLibraryName}/{AdyenLibraryVersion}");| string value = reader.GetString(); | ||
| return Encoding.UTF8.GetBytes(value); |
There was a problem hiding this comment.
reader.GetString() can return null. Encoding.UTF8.GetBytes(value) will throw an ArgumentNullException if value is null. You should handle the null case to prevent this.
string value = reader.GetString();
return value == null ? null : Encoding.UTF8.GetBytes(value);| private T DeserializeRaw() => JsonSerializer.Deserialize<T>(RawContent, _jsonSerializerOptions)!; | ||
|
|
||
| public T Ok() => DeserializeRaw(); | ||
| public bool TryDeserializeOkResponse(out T? result) { result = string.IsNullOrEmpty(RawContent) ? default : DeserializeRaw(); return result != null; } |
There was a problem hiding this comment.
This implementation of TryDeserializeOkResponse (and other similar TryDeserialize... methods in this class) has a few issues:
- Potential bugs with value types: If
Tis a value type (e.g., a struct),default(T)is notnull. So ifRawContentis empty,result != nullwill betrue, and the method will incorrectly indicate success. - Unhandled exceptions:
JsonSerializer.Deserializecan throw aJsonExceptionif theRawContentis not valid JSON. TheTry...pattern should typically not throw exceptions for expected failure cases like this. - Readability: The single-line implementation is dense and harder to understand.
A more robust implementation would handle exceptions and be clearer. This applies to all TryDeserialize... methods in this class.
Example for TryDeserializeOkResponse:
public bool TryDeserializeOkResponse(out T? result)
{
if (string.IsNullOrEmpty(RawContent))
{
result = default;
return false;
}
try
{
result = JsonSerializer.Deserialize<T>(RawContent, _jsonSerializerOptions);
return result != null;
}
catch (JsonException)
{
result = default;
return false;
}
}| public System.Net.Http.HttpRequestMessage AddHeadersToHttpRequestMessage(System.Net.Http.HttpRequestMessage httpRequestMessage) | ||
| { | ||
| foreach (KeyValuePair<string, string> header in this.Headers) | ||
| httpRequestMessage.Headers.Add(header.Key, header.Value); |
There was a problem hiding this comment.
Using Headers.Add can throw an ArgumentException if the header already exists on the HttpRequestMessage. It's safer to use TryAddWithoutValidation to avoid exceptions if a header is already present.
httpRequestMessage.Headers.TryAddWithoutValidation(header.Key, header.Value);| /// <returns><see cref="RequestOptions"/>.</returns> | ||
| public RequestOptions AddIdempotencyKey(string idempotencyKey) | ||
| { | ||
| this.Headers.Add("Idempotency-Key", idempotencyKey); |
There was a problem hiding this comment.
Using Dictionary.Add will throw an ArgumentException if the key already exists. This means calling this method multiple times will cause a crash. To make this method more robust and idempotent, you should use the indexer to set the value, which will add the key if it doesn't exist or update it if it does. This applies to AddAdditionalHeaders, AddWWWAuthenticateHeader, and AddxRequestedVerificationCodeHeader as well.
this.Headers["Idempotency-Key"] = idempotencyKey;| } | ||
|
|
||
| [TestMethod] | ||
| public void Given_NullToken_When_Read_Then_ThrowsNotSupportedException() |
There was a problem hiding this comment.
The test name Given_NullToken_When_Read_Then_ThrowsNotSupportedException is misleading. The test correctly asserts that null is returned, but the name suggests an exception is expected. The name should be updated to reflect the actual behavior.
public void Given_NullToken_When_Read_Then_Returns_Null()| } | ||
|
|
||
| [TestMethod] | ||
| public async Task Given_TokenProviderWithToken_When_GetCalled_Then_SameInstanceAndValueAreReturned() |
There was a problem hiding this comment.
This test method and Given_TokenProvider_When_GetCalledMultipleTimes_Then_SameInstanceReturnedEachTime are marked as async but don't use any await operators. This will result in a compiler warning. Since the methods' execution is synchronous, you can change their signatures to return void instead of Task.
public void Given_TokenProviderWithToken_When_GetCalled_Then_SameInstanceAndValueAreReturned()| try | ||
| { | ||
| request.AddUserAgentToHeaders(); | ||
| request.AddUserAgentToHeaders(); | ||
|
|
||
| request.AddLibraryNameToHeader(); | ||
| request.AddLibraryNameToHeader(); | ||
|
|
||
| request.AddLibraryVersionToHeader(); | ||
| request.AddLibraryVersionToHeader(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.Fail(); | ||
| } |
There was a problem hiding this comment.
The try-catch block with Assert.Fail() is redundant here. A test method in MSTest will automatically fail if an unhandled exception is thrown. You can simplify this test by removing the try-catch block and just keeping the calls to the extension methods.
request.AddUserAgentToHeaders();
request.AddUserAgentToHeaders();
request.AddLibraryNameToHeader();
request.AddLibraryNameToHeader();
request.AddLibraryVersionToHeader();
request.AddLibraryVersionToHeader();| [DataRow(HttpStatusCode.TooManyRequests, false)] | ||
| [DataRow(HttpStatusCode.UnprocessableEntity, false)] | ||
| [DataRow(HttpStatusCode.InternalServerError, false)] | ||
| public async Task Given_ApiResponse_When_SuccessStatusCode_Then_Result_ShouldMatchHttpResponseMessage(HttpStatusCode code, bool expected) |
There was a problem hiding this comment.
This test method, and others in this file (lines 43, 63, 122), are marked as async but don't use any await operators. This will result in a compiler warning. Since the methods' execution is synchronous, you can change their signatures to return void instead of Task.
public void Given_ApiResponse_When_SuccessStatusCode_Then_Result_ShouldMatchHttpResponseMessage(HttpStatusCode code, bool expected)| try | ||
| { | ||
| using (HMACSHA256 hmac = new HMACSHA256(key)) | ||
| { | ||
| // Compute the hmac on input data bytes | ||
| byte[] rawHmac = hmac.ComputeHash(data); | ||
|
|
||
| // Base64-encode the hmac | ||
| return Convert.ToBase64String(rawHmac); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| throw new Exception("Failed to generate HMAC signature: " + e.Message, e); | ||
| } |
There was a problem hiding this comment.
Catching a generic System.Exception is generally discouraged as it can obscure the original error. It's better to let more specific exceptions propagate. This makes debugging easier as the caller gets more precise information about what went wrong. You can simplify the code by removing the try-catch block.
using (HMACSHA256 hmac = new HMACSHA256(key))
{
// Compute the hmac on input data bytes
byte[] rawHmac = hmac.ComputeHash(data);
// Base64-encode the hmac
return Convert.ToBase64String(rawHmac);
}
Description
This PR addresses the upgrade of the OpenApiGenerator to v7.16.0 used in the Adyen sdk-automation to generate .NET models & services from the Adyen OpenAPI Specifications. This PR addresses
allOfsupport and native support forSystem.Text.Json.This branch/PR contains the changes in: #1207 1208 1209 1213
Release notes (WIP)
Added new functionality
Folder-structure #1207
/Core-folder with the following functionality:/Authcontains classes to add authorization to HttpHeaders./Clientcontains classes that wrap the ApiResponses and help-functionality to construct the LiveUrl (seeUrlBuilderExtensions.cs)/Client/Extensions/contains thePolly-library that allows user to configureTimeouts,RetriesandCircuitBreakers./Converterscontains JsonConverters forDateOnly,DateOnlyNullable,DateTimeandDateTimeNullable/Optionscontains classes to configure theAdyenOptionsorAdyenEnvironment(Test/Live).DateTimeOffSetnew DateTime(2025, 12, 25, 11, 59, 59)new DateTimeOffset(2025, 12, 25, 11, 59, 59, TimeSpan.Zero)DateTimeOnlynew DateTime(2025, 12, 25, 11, 59, 59)new DateOnly(2025, 12, 25)Mustache templates #1208
api_doc.mustache- Allows documentation generation in/docsfolder, only theService-classes are generated, showing developers how to interact with the APIWebhookHandler.mustache- Allows the WebhookHandler.cs to be generated for deserializing webhooksmodelInnerEnum.mustache, all generated enums are now strings and forward compatible, this fixes Discussion: Should we make the enums forward-compatible? #1071modelGeneric.mustache) and services (api.mustache), supportingallOfChanges - Moved classes #1209
.csproj./TerminalApifolderAdyen/Service/TerminalApiAsyncService->Adyen/**TerminalApi**/Service/TerminalApiServiceAdyen/Service/TerminalApiSyncService->Adyen/**TerminalApi**/Service/TerminalSyncServiceAdyen/Service/TerminalApiLocalService->Adyen/**TerminalApi**/Service/TerminaLocalServiceAdyen/Service/TerminalLocalApiUnencrypted->Adyen/**TerminalApi**/Service/TerminalLocalApiUnencryptedAdyen/Service/TerminalCloudApi->Adyen/**TerminalApi**/Service/TerminalCloudApiAdyen/Service/TerminalLocalApi->Adyen/**TerminalApi**/Service/TerminalLocalApiBreaking changes - Renames for consistency
Adyen/Util/HMACValidator.cs->Adyen/Webhooks/HmacValidatorUtility.BalancePlatformWebhookHandler,ClassicPaltformWebhookhandler,ManagementWebhookHandlerReview and merge the following into the
v7-generator/development-branch