Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: search params in returned links break iteration #405

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

janpaepke
Copy link
Collaborator

@janpaepke janpaepke commented Feb 11, 2025

Fixes #404

buildURL expects a pathname (or originAndPathname) to be passed, which does not contain any search parameters.

If it does, however, the result will contain a broken url.
This happened in the example of the getRefunds helper, which used the returned URL which included ?testmode and appended limit params. The resulting URL looked like this:
https://api.mollie.com/v2/payments/tr_paymentID/refunds?testmode=true?limit=128

The solution was to parse the URL into its pathname and query components, before passing it into .iterate.

This issue concerned all helpers, which returned iterators.
Helpers, which only used .get did (accidentally) work and could have remained untouched, but I still updated them for consistency.

My initial impulse was to also update buildUrl, which caused the concatenation issue.
There are two options: allow existing SearchParams in the pathname and merge them or throw if there are params in the pathname.
I was leaning towards the latter, as the whole client consistently handles pathnames and query params separately.

Ultimately I decided to leave it as is, since any change (merge or throw) could lead to unexpected behaviour for users, if any networkClient call using it got overlooked.
That would make it a breaking change.

We might want to keep this in mind for the next major, though.

@janpaepke janpaepke added the bug Unexpected behaviour. label Feb 11, 2025
@janpaepke janpaepke requested a review from edorivai February 11, 2025 10:38
Copy link
Collaborator

@edorivai edorivai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM.

I agree with the approach of this PR; fixing all the callsites to ensure query params are broken out before calling the networkClient, and not making any potentially breaking changes yet.

In terms of future changes, we discussed

  1. Throwing if the pathname contains query params
  2. Parsing the pathname and merging query params if the pathname contains any

I have strong feelings against (1), because if a wrong pathname is passed, this is likely an internal bug on our part, not something the user did wrong. And in that case I wouldn't want to introduce runtime failures in downstream apps.

I can see (2) working. If we want to consider it, we should outline possible scenarios where merging could potentially break or cause unexpected behavior.

@janpaepke janpaepke merged commit 51f19b0 into master Feb 17, 2025
5 checks passed
@janpaepke janpaepke deleted the fix/parse-provided-links branch February 17, 2025 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

payment.getRefunds() doesn't seem to respect testmode properly
2 participants