Repository navigation
fix: support PAL-based service URLs in _set_url_version (#474) - #511
cobanfurkanx wants to merge 1 commit into
Conversation
Update regex in AdyenClient._set_url_version to match version segments anywhere in the path (/v\d+(?=/|$)) instead of only immediately after .com. This allows version overrides on PAL-based services (Recurring, Payment, Payout, BinLookup) to take effect properly. Also add a guard returning the unmodified endpoint when version is None to prevent overwriting unrelated service URLs. Fixes Adyen#474
|
![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 updates the _set_url_version method in Adyen/client.py to safely handle missing service versions and support multi-digit version numbers using an updated regular expression. It also adds corresponding unit tests in test/DetermineEndpointTest.py. Feedback highlights a potential bug where the service name 'Payout' does not match the 'payouts' key in version_lookup, causing overrides to be ignored, and suggests using self.addCleanup in tests to prevent state pollution.
| version = version_lookup.get(service) | ||
| if version is None: | ||
| return endpoint |
There was a problem hiding this comment.
There is a mismatch between the service name used for the Payout service and the key defined in version_lookup. In _set_credentials (lines 250 and 270), the service name is checked as "Payout" (capitalized, singular). However, in version_lookup (line 312), the key is defined as "payouts" (lowercase, plural).
As a result, version_lookup.get("Payout") will return None, and any version override configured via api_payout_version will be silently ignored for the Payout service.
To fix this, we can fall back to checking "payouts" if the service is "Payout" and not found directly in version_lookup.
| version = version_lookup.get(service) | |
| if version is None: | |
| return endpoint | |
| version = version_lookup.get(service) | |
| if version is None and service == "Payout": | |
| version = version_lookup.get("payouts") | |
| if version is None: | |
| return endpoint |
| def test_set_url_version_pal_recurring(self): | ||
| self.client.api_recurring_version = "70" | ||
| url = self.client._set_url_version("recurring", self.recurring_url + RECURRING_DETAILS) | ||
| expected = self.recurring_url.replace(f"/{self.recurring_version}", "/v70") + RECURRING_DETAILS | ||
| self.assertEqual(url, expected) | ||
| self.client.api_recurring_version = None |
There was a problem hiding this comment.
Modifying shared state (like self.client) inside test methods without guaranteed cleanup can lead to test pollution and flaky tests if an assertion fails before the cleanup line is reached.
Using self.addCleanup ensures that the cleanup is always executed, even if the test fails or raises an exception. This pattern should also be applied to the other new test cases (test_set_url_version_pal_payment, test_set_url_version_checkout, and test_set_url_version_multi_digit).
| def test_set_url_version_pal_recurring(self): | |
| self.client.api_recurring_version = "70" | |
| url = self.client._set_url_version("recurring", self.recurring_url + RECURRING_DETAILS) | |
| expected = self.recurring_url.replace(f"/{self.recurring_version}", "/v70") + RECURRING_DETAILS | |
| self.assertEqual(url, expected) | |
| self.client.api_recurring_version = None | |
| def test_set_url_version_pal_recurring(self): | |
| self.client.api_recurring_version = "70" | |
| self.addCleanup(setattr, self.client, "api_recurring_version", None) | |
| url = self.client._set_url_version("recurring", self.recurring_url + RECURRING_DETAILS) | |
| expected = self.recurring_url.replace(f"/{self.recurring_version}", "/v70") + RECURRING_DETAILS | |
| self.assertEqual(url, expected) |
This branch has not been deployed
1 participant
Description
Fixes #474
AdyenClient._set_url_versionpreviously usedre.sub(r"\.com/v\d{1,2}", f".com/{new_version}", endpoint), which only matched endpoints where the version segment directly followed.com(such as Checkout and Management APIs).As a result, PAL-based services where the version segment is located deeper in the path (e.g.
/pal/servlet/Recurring/v68,/pal/servlet/Payment/v68,/pal/servlet/BinLookup/v54,/pal/servlet/Payout/v68) were never matched, causing version overrides (api_recurring_version,api_payment_version, etc.) to have no effect. Additionally, if only one service version override was configured on the client, other services without an override had their version corrupted tovNone.Changes
r"/v\d+(?=/|$)"to match/v<version>anywhere in the path, supporting multi-digit versions without assuming a.comprefix.Noneguard: Added an early returnif version is None: return endpointto avoid touching endpoints for services that have no version override specified.test/DetermineEndpointTest.pycovering PAL Recurring, PAL Payments, Checkout,Noneguard preservation, and multi-digit version overrides.Tests
Ran the complete unit test suite: