Dont add path to an url we already added a path to.

[Nyholm]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #118, fixes #113
Documentation
License	MIT

What's in this PR?

What if we stored all the results of our rewrite and make sure we never rewrite a url that were stored as a result?

[mxr576]

I'll try out this patch next week. IIRC I tried a similar approach before without any luck when I worked on my library.

[dbu]

that idea makes sense to me. we need to cleanup however, imagine i call the same url repeatedly in the same php process: the rewrite only happens once. with async, i am not sure we can even expect the promise to be resolved already. so we'd need to combine the spl hash and the url, and cleanup in the then of a promise. the request will live until the promise is resolved (because there is still a reference to it) so the spl hash can't be reused until then. tracking the url that we rewrote prevents us from rewriting it again.

[Nyholm]

I dont understand, Why would it be any different for async?

[dbu]

because i could have open promises that have not yet been resolved, and already request the same url again. (yeah, an edge case, but would be good to not fail on edge cases this time)

[Nyholm]

I still dont understand. Im sorry.

If i add the path /api

T0: An async request (A) to https://example.com/foo
T1: A normal request (B) to https://example.com/bar
T2: Request (B) will be rewritten to https://example.com/api/bar

T3: A normal request (C) to https://example.com/api/foo
T4: Request (C) will be rewritten to https://example.com/api/api/foo (this is expected)

T5: Request (A) will be resolved and rewritten to https://example.com/api/foo
T6: Same request (C) again to https://example.com/api/foo
T7: Request (C) will not be rewritten at all. It will go to https://example.com/api/foo

Issue. T4 and T7.

---

Same would happen if:

T0: A normal request (A) to https://example.com/api/foo
T1: Request (A) will be rewritten to https://example.com/api/api/foo

T2: A normal request (B) to https://example.com/foo
T3: Request (B) will be rewritten to https://example.com/api/foo

T4: A normal request (C) to https://example.com/api/foo
T5: Request (C) will NOT be rewritten.
Issue: T1 and T5

---

I mean, async or not does not matter.

[Nyholm]

The issue above is acceptable.
You use the AddPath plugin to always add a path. Ie, if your api endoints always starts with /api/v1 then you may remove that from all your calls.

My PR make sure we never add /api/v1` twice.

The issue I describe is an edge case that will only happen if:

• A) You call the API with your path included OR
• B) If you have two endpoints; one where the "path to add" is repeated and one where "path o add" is only once. Ie
• • https://example.com/api/v1/api/v1/foo vs https://example.com/api/v1/foo
• • https://example.com/api/api/foo vs https://example.com/api/foo

A is just using the plugin wrong.
B is super unlikely. If you do have B you need to call them both and in the wrong order to have an issue.

[dbu]

the current code will fail when we do

• Request http://example.com/path
• Request http://example.com/path

The first time, we add the prefix, the second time we don't. The plugin is not reset on each request.

If we add a then to remove the hash from our map when the request is finished, i think it will still not work if i do:

• Request A http://example.com/path
• Request B http://example.com/path
• Wait for A
• Wait for B

[Nyholm]

The first time, we add the prefix, the second time we don't. The plugin is not reset on each request.

No, I think you are reading the code wrong.

I removed a test that was not working that probably confused you.

[Nyholm]

I added a PHP unit test to prove that Im correct.

My PHPspec knowledge is too poor. If anyone wants to write write my phpunit test to phpspec, then be my guest.

[dbu]

sorry, i indeed missed that we now store the resulting url, not what we had as a starting point. that makes more sense, but i still think its problematic. in your test example with /api it would indeed be very strange to repeat the prefix, but i am sure somebody will have a situation where they expect the prefix to be added.

with your change, we depend on the order of how requests have been executed. if we say we do not want to support the case of the prefix being the same as the start of the supplied path, i think it would be better to check if the prefix is already present, as that would lead to context-free behaviour.

[dbu]

imho we can chose between:

• a) go back to the original approach of checking the prefix and declare that there is an edge case that is not supported by this plugin
• b) implement the request identifier to only prefix once per request, even when restarting

given the scenarios and use cases, i think a) is better. with b), there is also the issue that retry would be broken if redirecting comes after the retry plugin. a) will also work fine with redirects, as those most likely already have the right prefix.

[sagikazarmark]

I would also go with option a. It covers 98% of the use cases (/api/api is highly unlikely). There will always be cases that are too specific that we cannot support them.

[Nyholm]

Sure! Let's do that!

[Nyholm]

I've updated the PR

[dbu]

cool, thanks!

lets also update the documentation a bit to well explain the edge case where this now does not work as expected.

[dbu]

maybe: "AddPathPlugin: Do not add the prefix if the URL already has the same prefix."

[Nyholm]

How about this?

[Nyholm]

lets also update the documentation a bit to well explain the edge case where this now does not work as expected.

There is no documentation :/

[dbu]

i think we can simplify this test a lot now.

[dbu]

There is no documentation :/

found it here: https://github.com/php-http/documentation/blob/master/plugins/request-uri-manipulations.rst

[Nyholm]

I rebased this PR on 2.x

[dbu]

documentation: php-http/documentation#245

[nicholasruunu]

Good job guys. 👏

[dbu]

thanks. this was really tricky - and i think we came full circle on the approach we use...