OpenAPIGenerator upgrade to v7.16.0 - [4] Added tests for all services/models #1213
Conversation
Remove old mustache templates
Removed old /Adyen/Model using the previous models/services Removed old /Adyen/Service using the previous models/services Removed old /Adyen/Service.Test using the previous models/services Removed old /IntegrationTests using the previous models/services
Kwok-he-Chu
changed the base branch from
main
to
v7-generator/development-branch
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 is a crucial step in upgrading the OpenAPI Generator to version 7.16.0, which is essential for maintaining up-to-date and robust .NET models and services. The core of this PR involves a complete refactoring of the integration test suite, moving from an outdated structure to a modern, maintainable setup that fully supports the new generator's output and a more logical library organization. This ensures that all newly generated components are thoroughly validated and seamlessly integrated into 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 introduces a significant and positive modernization of the Adyen .NET library. The upgrade of the OpenAPI generator, the addition of a comprehensive suite of new tests, and the architectural shift towards Microsoft.Extensions.DependencyInjection, IHttpClientFactory, and System.Text.Json are excellent improvements. The refactoring also includes better code organization, such as the new Core project and the restructuring of the test projects.
However, there are a few critical points to address. The most significant is the change in target frameworks, which drops support for .NET 6, .NET Framework, and .NET Standard, making this a major breaking change that should be clearly communicated. I've also identified a minor bug in logging arguments and some areas for code cleanup, such as fragile URL construction logic and commented-out test files that should be removed.
Overall, this is a substantial step forward for the library. Addressing the identified issues will ensure a smoother transition for users and improve the long-term maintainability of the codebase.
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net8.0;net6.0;net462;netstandard2.0</TargetFrameworks> | ||
| <TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
The change from <TargetFrameworks>net8.0;net6.0;net462;netstandard2.0</TargetFrameworks> to <TargetFramework>net8.0</TargetFramework> is a significant breaking change. It drops support for .NET 6, .NET Framework 4.6.2, and .NET Standard 2.0, making the library compatible only with .NET 8. This should be clearly communicated in the pull request description and release notes. If this is not the intended outcome, the previous target frameworks should be restored.
| events.OnPayments += (sender, eventArgs) => | ||
| { | ||
| ApiResponse apiResponse = eventArgs.ApiResponse; | ||
| logger.LogInformation("{TotalSeconds,-9} | {Path} | {StatusCode} |", (apiResponse.DownloadedAt - apiResponse.RequestedAt).TotalSeconds, apiResponse.StatusCode, apiResponse.Path); |
There was a problem hiding this comment.
The arguments for the structured log message are in the wrong order. The format string is "{TotalSeconds,-9} | {Path} | {StatusCode} |", but the arguments are provided as TotalSeconds, apiResponse.StatusCode, and apiResponse.Path. This will cause the status code to be logged as the path and vice versa. The arguments should be swapped to match the format string.
logger.LogInformation("{TotalSeconds,-9} | {Path} | {StatusCode} |", (apiResponse.DownloadedAt - apiResponse.RequestedAt).TotalSeconds, apiResponse.Path, apiResponse.StatusCode);| @@ -0,0 +1,504 @@ | |||
| // using System; | |||
There was a problem hiding this comment.
This entire test file is commented out. If these tests are not planned to be implemented as part of this pull request, the file should be removed to avoid cluttering the codebase with dead code. If they are planned for a future PR, a TODO comment explaining this would be helpful.
| @@ -0,0 +1,124 @@ | |||
| // using System; | |||
There was a problem hiding this comment.
This entire test file is commented out. Similar to BalancePlatformWebhookHandlerTest.cs, this file should either be removed to avoid cluttering the codebase, or a TODO comment should be added to clarify its status if it's intended for future work.
| public static string ConstructLiveUrl(string liveEndpointUrlPrefix, string url) | ||
| { | ||
| // Change base url for Live environment | ||
| if (url.Contains("pal-")) // Payment API prefix | ||
| { | ||
| if (liveEndpointUrlPrefix == null) | ||
| { | ||
| throw new InvalidOperationException("LiveEndpointUrlPrefix is null - please configure your AdyenOptions.LiveEndpointUrlPrefix"); | ||
| } | ||
|
|
||
| url = url.Replace("https://pal-test.adyen.com/pal/servlet/", | ||
| "https://" + liveEndpointUrlPrefix + "-pal-live.adyenpayments.com/pal/servlet/"); | ||
| } | ||
| else if (url.Contains("checkout-")) // Checkout API prefix | ||
| { | ||
| if (liveEndpointUrlPrefix == null) | ||
| { | ||
| throw new InvalidOperationException("LiveEndpointUrlPrefix is null - please configure your AdyenOptions.LiveEndpointUrlPrefix"); | ||
| } | ||
|
|
||
| if (url.Contains("possdk")) | ||
| { | ||
| url = url.Replace("https://checkout-test.adyen.com/", | ||
| "https://" + liveEndpointUrlPrefix + "-checkout-live.adyenpayments.com/"); | ||
| } | ||
| else | ||
| { | ||
| url = url.Replace("https://checkout-test.adyen.com/", | ||
| "https://" + liveEndpointUrlPrefix + "-checkout-live.adyenpayments.com/checkout/"); | ||
| } | ||
| } | ||
| else if (url.Contains("https://test.adyen.com/authe/api/")) // SessionAuthentication | ||
| { | ||
| url = url.Replace("https://test.adyen.com/authe/api/", | ||
| "https://authe-live.adyen.com/authe/api/"); | ||
| } | ||
|
|
||
| // If no prefix is required, we replace "test" -> "live" | ||
| url = url.Replace("-test", "-live"); | ||
| return url; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in this method for constructing live URLs is complex and relies on string manipulation (Contains, Replace), which can be fragile and error-prone. For example, the final url.Replace("-test", "-live") is broad and could have unintended side effects if "-test" appears elsewhere in the URL. The repeated null check for liveEndpointUrlPrefix can also be simplified. Consider refactoring this to use a more structured approach, perhaps using a dictionary or a configuration object to map test URLs to live URLs for different services, to improve maintainability and reduce the risk of errors.
PR-4 of #1204
Description
This PR adds the tests needed for the generation of the new models and services using the new OpenApiGenerator to v7.16.0 used in the Adyen sdk-automation to generate .NET models & services from the Adyen OpenAPI Specifications.
This won't compile until we run the sdk-automation (see PR) with our new templates.