Path prefix must be present

[mxr576]

Make sure the path prefix is actually present even if the request has been altered before.

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#103
Documentation	-
License	MIT

What's in this PR?

Follow up on #103.

Everything worked fine until I refactored my tests and started to use a singleton API client for offline testing. If I call the same API endpoints in the same test with the client then the first API request gets prefixed properly but the second one does not, because the generated $identifier is already exist in the $this->alteredRequests.

[mxr576]

Open for ideas how this can be proved by tests in this library.

[dbu]

i guess if you send the exact same request multiple times, a fix is needed. while that certainly is an edge case, i would expect the client to handle it correctly.

we started using this code rather than comparing the paths because you could happen to have a path that starts with the same characters as the prefix you want to add. in that case, the prefix still needs to be added once. with the change as is, this fix is reverted.

could we instead remove the identifier of the $first request when we get a response back? or would that bring back the original problem of adding the prefix repeatedly on retries?

[dbu]

should we assign $request->getUri()->getPath() and $this->uri->getPath() to variables to not execute the code twice, in the expected case of having to add the prefix?

[joelwurtz]

I agree with @dbu, removing the request identifier when a response is resolved should be a better way of doing this (if it's work of course).

[magnusnordlander]

We're running in to an issue which we think might be caused by #103, and should probably be mitigated by this.

Really, the root of the problem is that spl_object_hash is not appropriate to use in situations like this (and there are no decent substitutes). spl_object_hash is guaranteed to be the same when called twice on an object, but it is not guaranteed to be unique, because hashes may be reused after objects have been deallocated.

From the manual:

This function returns a unique identifier for the object. This id can be used as a hash key for storing objects, or for identifying an object, as long as the object is not destroyed. Once the object is destroyed, its hash may be reused for other objects.

One workaround would be to store a reference to the object, which of course would cause it not to be deallocated, but the downside is of course increased memory usage.

[nicholasruunu]

One fix to the problem (^) might be to save the altered request uris instead and check:
if (!array_key_exists($request->getUri()->getPath(), $this->alteredRequests)

It could possibly create some kind of issue I haven't thought about.

-- edit --
This won't fix the problem since the identifier is found in the alteredRequests array and the problem is that the uri path won't be prepended.

-- edit --
No actually it would work as long as there's not a duplicated uri step or something like /r /responses. Doesn't seem totally robust imo.

[joelwurtz]

Due to PSR7 immutability and the fact that Client request does not have attributes (like the server one), the only viable solution here is to use the headers to store some id generated on the first run of this plugin. However it would send this information to the server.

Maybe we can add a store header managed directly by the PluginClient (named X-HTTPlug-PluginMetadata), which is deleted when passed to the underlying client ? So plugins can safely use this header when in the plugin chain to store some metadata ?

[dbu]

i like that header idea joel. does it mean the plugins potentially need to share that header? alternatively, we could define that the plugin client will scan the headers and remove any headers starting with X-HTTPlug-PluginMetadata- so that each plugin can have its own metadata headers...

[nicholasruunu]

Sounds good @joelwurtz, but do we really need an id for this?
X-HTTPlug-PluginMetadata: addPath-altered=1 in the request should be enough?
Then just remove X-HTTPlug-PluginMetadata all together.

If that's too much work to check there's I think @dbu's solution will suffice but with X-HTTPlug-PluginMetadata-addPath-altered: 1

-- edit --
Actually just X-HTTPlug-PluginMetadata: addPath-altered is valid as an array option.

[joelwurtz]

yes it would be fine @nicholasruunu, certainly it does not need a id

@dbu i think we can start with a single header, as a header can have multiple value it should be fine ATM. We can always change that later (i would make this internal first, external plugin should not use it : we dont provide bc contract)

[dbu]

X-HTTPlug-PluginMetadata: addPath-altered=1

That would be a convention. Maybe that is good enough. Can plugins add such values with psr-7 methods or does it need string parsing?

[nicholasruunu]

@dbu Yeah, you can do it with withAddedHeader('X-HTTPlug-PluginMetadata', 'addPath-altered=1'), which will add a value to the header key or create it if it doesn't exist. Then you can remove it with withoutHeader('X-HTTPlug-PluginMetadata').

Not sure if I'd add =1 though, seems a bit unnecessary since other headers like Cache-control doesn't use equal values when not indicating a useful value.

-- edit --
To check it you could use in_array('addPath-altered', $request->getHeader('X-HTTPlug-PluginMetadata')

[dbu]

@nicholasruunu tested this a bit with guzzle psr-7 and indeed that works fine.

lets do this then! i'd go with just the marker without =1 as that indeed seems unnecessary.

if a plugin ever needs to remove its flags, it can unset its value from the array and then withHeader with the remaining array. lets document this functionality of the plugin client so it can also be used by others and is well defined (namespace convention with the plugin name, you may not remove the whole header but only your own flags)

do you want to start a new pull request for this?

[mxr576]

do you want to start a new pull request for this?

@dbu I'd because my library installs this PR as a patch and I would like test the described solution first in IRL to be sure that it actually solves this problem.

[magnusnordlander]

Using a single (non-unique) flag will mean that there can't be multiple AddPathPlugins. I'm not sure when you'd ever have that, but it is technically possible, so I'd argue that either the flag name should be unique (e.g. addPath-altered-34dfd3, where the last part has been generated randomly in the plugin constructor), or that it should have a value unique to the plugin instance.

[dbu]

oh, good point. the spl_object_hash should be good for this one, right? plugins won't be added and removed from the chain on the fly, so no spl hash collision risk i think. we could use addPath-altered=spl_object_hash($this) maybe?

[nicholasruunu]

@dbu I think that uniqid() will be fine here. There will be no need to compare it to anything but the value itself.

[magnusnordlander]

uniqid() isn't necessarily a bad choice, but it sleeps for a microsecond when you call it though. That's probably not an issue here, but if it is, mt_rand() would be an alternative that doesn't sleep. spl_object_hash($this) is probably a no-go here too, since the value is stored in something that does not have an object reference back.

[dbu]

then i vote for mt_rand

[joelwurtz]

mt_rand is not unique, so you can have the same problem as without it.
We can simply use a private static member incremented by one each time we used it: fast, no sleep and unique.

[nicholasruunu]

Yeah, I agree that mt_rand feels wrong however unlikely it is to fail.
Making a static incrementing method also feel ugly, but I don't have much better options with the current design.
So to solve this pressing issue I'd vote for this option.

I think for the original issue (#103) of being able to resend an altered request I think this is the wrong way to go though. What you want is to resend the original request, not let all the plugins figure out if it's unaltered or not.

[dbu]

good point to ask the fundamental sanity question...

when does the plugin see the "same" request multiple times? why would that not start with the original request but with the one that has already been altered?

[dbu]

@nicholasruunu i went looking up the code and the reason this happens like this is that the plugin method is passed $first which is the first plugin, for when they want to "restart" the request. the plugins only have the request at its current state, not the original unmodified request. so when they want to restart for whatever reason, all plugins are applied a second time to the request. so i think we do need the discussed solution.

@mxr576 do you have time for a pull request to implement the solution with the static increment?

[nicholasruunu]

@dbu Not sure I get it, but if you want to restart a request, shouldn't you create a $first plugin to handle that? Take this first request and reuse that for a retry?

[dbu]

this is how the plugin chain is created:

client-common/src/PluginClient.php

Line 145 in e35c5f3

private function createPluginChain($pluginList, callable $clientCallable)

the plugin does not have access to the original request, and the "first" plugin is not hardwired with the unchanged request. therefore if a plugin restarts the chain, it only has access to the request as it currently looks. changing this would be quite a big change for how the plugin system works.

[dbu]

just found that the redirect plugin is one place to restart a request:

client-common/src/Plugin/RedirectPlugin.php

Line 142 in 5496bf2

return $first($redirectRequest);

that would be an example where its very important to not add the prefix if we already handled the request, regardless of whether the prefix is in the path or not - once the server told us the url we don't want it to change anymore.

[nicholasruunu]

@dbu Yeah I see what you mean... this package is breaking my brain. 😩
My thinking is that AddPath must be a special kind of plugin that only runs on requests sent from the user and only once. Or not being a plugin at all.

[dbu]

another thought: adding a path only makes sense for known hosts. we could change the add path plugin to be configured with a regex for the domains for which to add a prefix.

the only place we ever call $first is in the RedirectPlugin. retry for example does not restart from the beginning, but from the current location. therefore i think instead of trying to track the chain, i'd make AddPath look at the domain and assume that no plugin restarts the same request with the same path. (if authentication needs a separate request, that should not be done with a plugin that restarts the request to do an authentication request, but by implementing Authentication and injecting a client to the auth implementation)

[Nyholm]

I added an alternative solution here: #141

[Nyholm]

There are no good solution. Just solutions that redefine what edge cases we will support.

In 2.x will have a simple approach. See #141

Im closing this as "wont fix"