Fix redirect handling to respect RFC and HTTP/1.1 RFC 7538

[mikelupo]

Before this change, a POST request redirected with status code
307 or 308 would become a request with GET method.
This check-in changes the behavior to retain the HTTP method
in the case of 307/308 redirect

Checklist

• I've checked that all new and existing tests pass
• [ N/A ] I've updated the documentation if necessary
• I've added an entry in the CHANGELOG to credit myself

Description

Motivation and Context

inadherence to RFC and HTTP specs.

Fixes #262

I ran existing NSURLSession tests. TODO: Create a test case to handle these cases.
Will require some help from @AliSoftware to do so

Thanks to my colleague @sashra for finding this!

[mikelupo]

@AliSoftware Oliver can you peruse this and let me know if any changes are needed? Thanks!

[AliSoftware]

I haven't checked the RFC in full, but wondering if we should only keep the method for 307-308 or if there are any other 3xx status codes that would need to follow the same rule?

For example for 301/302 I read that the RFC had no intent originally for clients to change the method to GET when redirecting, even if most clients are doing so, making it become sort of a de-facto standard which wasn't intended (and making 307/308 come to be created to remove that ambiguity).

RFC 2616 (the updated version of 2068) clarifies that, explicitly stating that clients are not allowed to change the method on 301/302 redirections and that the 307 status code is somewhat a workaround to deal with non-compliant clients:

Note: RFC 1945 and RFC 2068 specify that the client is not allowed to change the method on the redirected request. However, most existing user agent implementations treat 302 as if it were a 303 response, performing a GET on the Location field-value regardless of the original request method. The status codes 303 and 307 have
been added for servers that wish to make unambiguously clear which kind of reaction is expected of the client.

I wonder what the native iOS behaviour (NSHTTPURLProtocol) , as well as Safari, do? Does it mishandle 301/302 like historically most other browsers and change the method to GET while the RFC didn't intent that, or does it properly keep the method like the RFC originally intended, making it behave the same for 301/302 and 307/308?

Speaking of which, I also wonder if there isn't some method out there to create the proper new NSURLRequest from a redirect response, like maybe asking NSHTTPURLProtocol or something, to avoid re-implementing those rules ourselves? Tbh I doubt that it's publicly available, but it's still probably worth looking into

[AliSoftware]

Please add a period + 2 spaces (ending the line with 2 spaces allows the rendered markdown to insert a newline in the same paragraph to make the links be on their own line, see other entries)

[AliSoftware]

Also little nitpicking while I'm at it :P Just for readability I think 307/308 is more understandable than 307/8

[mikelupo]

Fixing this too :)

[AliSoftware]

Please use spaces for indentation (not because I want to start the tabs vs spaces war 😄 but because all the rest of the code uses spaces so we should be consistent)

[mikelupo]

Yes, I keep forgetting that you do that. And my Xcode has auto tabs, so when I save the file, it gets done automatically. I'll fix that.

[AliSoftware]

I think you can override that per project (selecting the project in the left pane of Xcode then change the values of tab width + indent with on the right pane), maybe you should use that, so that whatever settings contributors like you have globally set in their Xcode, it will use spaces for that specific OHHTTPStubs project.

[AliSoftware]

Ok, I've investigated a little bit

• It seems that NSURLSession does preserve method on 301/302 at first glance. Only tried with a HEAD request to http://google.com though (and seing that the redirect was also a HEAD), because I don't have any example server at hand doing a 301/302 on a POST to test. So maybe we should do the same here too and keep the method for 301/302 too (as the original RFC intended even if it's not what historical browsers do, but seems is what Safari does now?)
• When redirecting a POST request, shouldn't we also keep the body of the original request too?

[AliSoftware]

So, in light of the remarks I just did, maybe we should:

• Keep not only the httpMethod but also the body of the original request
• Do that for all 301, 302, 307 and 308 (but not 303)

So in practice:

• Maybe we should be less broad in the test >300 && < 400 above and only handle specific codes
• Do a mutableCopy of the original request and alter its url (instead of allocating a new NS[Mutable]URLRequest from scratch) so it keeps all the other original properties like method & body?
  • If we do that, be sure that making a copy of a request which was potentially already consumed didn't make the body empty, as the body data is quickly turned into the httpBodyStream: NSInputStream property and if that stream has started being consumed maybe a mutableCopy isn't enough to make it work & re-read from the beginning? So be sure to test that use case

[mikelupo]

I kept the test for > 300 && < 400 but then applied special treatment for the special codes.
Instead of creating a new NSMutableRequest, I can (and should have created a mutable copy instead) NSMutableURLRequest* tempRedirectRequest = [self.request mutableCopy];
Just to be safe, I'll make sure to copy the body into the mutable copy as well.

[mikelupo]

@AliSoftware As I'm re-reading your comments of making a mutable copy.

NSURLRequest *redirectRequest = (NSURLRequest*)[tempRedirectRequest copy];

How can we know if the HTTPBody was consumed?
quoting your comment, "... as the body data is quickly turned into the httpBodyStream: NSInputStream property and if that stream has started being consumed maybe a mutableCopy isn't enough to make it work & re-read from the beginning? So be sure to test that use case"
Need suggestions on how I test that use case?

Seems we'll have to return the NSMutableURLRequest instead of copying it back to immutable. I wonder if there would be any unintended side-effect from that. Would we cause upgrading users to hit a snag because of the type change?

[AliSoftware]

Just to be clear, when I said to make sure to copy the body, I didn't mean to make tempRedirectRequest.body = request.body explicitly, I rather meant that, by experience, once the request has been sent thru the iOS URL Loading System, as far as I understand, the content of the httpBody: NSData is converted into an NSInputStream which is then set to the httpBodyStream property of the request, and the httpBody property itself is then set back to nil

This is why we had to add the OHHTTPStubs_httpBody workaround-property for people wanting to do tests on their request body in their stubs, because at the time the stub is called the httpBody property was already turned into an inputStream then set to nil (so as a workaround we duplicate the httpBody: NSData when it's set so we can read it back later).

I don't know how that's gonna work in our current context of redirection, maybe the [request mutableCopy] will have everything already working for the body to be re-used — even it's already transformed into an httpBodyStream, if that stream isn't consumed yet there shouldn't be any problem, the copy will have the copy of the httpBodyStream too and it will work as expected), but given this oddity in how httpBody is auto-transformed into an NSStream by the OS automatically, it's worth checking using some tests that it still works as expected in practice.

[AliSoftware]

Whoops our comments were cross-posted 😉

To answer your question above, I think:

1. It could be fine to return a mutable request, it will be exposed by the API as an NSURLRequest even if under the hood it's mutable, so the only side effect would be if the users were to cast it to mutable directly without making a mutableCopy first, which they are not supposed to do anyway (I mean if later they have a delegate method called, or their stub called, the type of the parameter will be NSURLRequest even if the object itself was in practice NSMutableURLRequest, but they are not supposed to do (NSMutableURLRequest*)request to force-cast anyway
2. If the httpBodyStream is not consumed and is properly copied from the request to the mutableCopy, I don't see any reason why it wouldn't also be properly copied from the mutableCopy back to the immutableCopy we return, so in practice we should still return an immutable copy anyway.

So to be clear:

// We have the immutable "NSURLRequest* request" at that point // (1)
NSMutableURLRequest* mreq = [request mutableCopy]; // (2)
mreq.url = newLocation: // (3)
NSURLRequest* newReq = [mreq copy]; // (4)

• At point (1) we have the request, but I'm guessing the httpBody is already set to nil by the OS and has been turned into the httpBodyStream instead. The big question is, has that httpBodyStream already been consumed or not. Let's assume it hasn't (hyp A).
• So at point (2), the httpBodyStream of the original request should, I guess (hyp B), be copied into the mutable request during the copy
• At point (4), the httpBodyStream of the mutable copy is copied into the immutable request during copy (hyp B)

And:

• I think the hypothesis B is very likely, after all when we ask for a (mutable)Copy there's no reason why it wouldn't make an exact copy of all fields, including the inputStream. But since it's a stream (like a file descriptor you have after some fopen and so on), and thus having multiple copies of the same stream open at once could lead to side effects, it's worth checking
• But the big question is hypothesis A, whether the stream has already been read by the filesystem or not, and if it has can it (and should it) be rewinded so that it's read from the start again for the new redirect request, etc

I know it's a little complex, and maybe after having said all that maybe I'm imagining problems that don't even exist and everything will work automatically as expected without us having to worry about hypothesis A & B, but given that we had that issue that made us create the OHHTTPStubs_httpBody workaround, better be safe and double-check than sorry and break things 😉

[AliSoftware]

I think the simplest way to check that is to:

1. Check if, as I guessed, by the time we are at that redirect stage, httpBody is already back to nil and turned into an httpBodyStream instead. If it's not yet turned into a stream, then all my worries go away as it's just copying the body's NSData, nothing special happens during the mutableCopy/copy then

2. If it's indeed already turned into an httpBodyStream, look a at the httpBodyStream.streamStatus (see doc

• If it's NotOpen for example, then it's ok even if it's already turned into a stream it has never been read yet
• If it's NSStreamStatusAtEnd or Closed, then we might have a problem as that means it has already been consumed (and idk if it can be rewinded to the start so that our request copy that we return for the redirect is able to re-consume the data?)

[mikelupo]

Ok, I can add the code to test that and try it out in my own client.
But if it's the latter, NSStreamStatusAtEnd or Closed, then we've got to decide how best to handle that.
Now, I suppose all this only matters if the original request is of method=POST?

[AliSoftware]

I really hope everything will work magically (and if that's the case, sorry for the false alarm and complex explanations, maybe I'm worrying for nothing).

But if it's indeed the latter:

• It's a good question about how we've to handle that. It seems we can't rewind on an NSInputStream other than a file stream (so not one build from NSData), as far as I read in the docs. So we'll have to re-create it. But the httpBody is back to nil… So we'll have to rely on our OHHTTPStubs_httpBody workaround which kept a copy of the body around? Gross… but not sure we have many more options.

• As for "only apply for method=POST, I'm not sure what the RFC expects. In general, GET requests don't usually have any httpBody — only POST request do — but I have no idea if it's officially valid/accepted for GET request to have an httpBody according to the RFC, and if it is, if that body should also be conserved when redirecting. But I guess the logical thing to do would then be to do the copy of the body for all requests, whatever the method.

Let's test first — if the body is already converted to a stream at that point and if the stream is already open or not — before relying on hypothesis and think of possible solutions (that we might not even have to use if the hypothesis isn't even true), we'll think about the solution depending on the result of the tests then 😉

[mikelupo]

@AliSoftware This is what I observed in our own redirect tests. Does it amply answer your question? (Please note that this code a local test and isn't submitted here on Github at this time.)

[AliSoftware]

@mikelupo This is a really good news, that means that everything should work magically, and I worried for nothing 😅

The mreq = [request mutableCopy] should copy the stream, which is not open so has never been consumed, then newReq = [mreq copy] will do the same, and we'll return our new duplicated request with the same stream not yet consumed, and the iOS Loading System will be able to open and consume it only later when the request is finally really consumed.

Tbh I'm a bit (happily) surprisedly this result, as the request and all its body theoretically would have been sent to the real server at that stage, so that the server can return a 3xx redirect, so at that point, at least in real (not-stubbed) conditions, I guess the OS has already consumed the body (as it has sent the full request to the server). But in our case as we intercepted everything we never consumed it, so we should be totally good :-)

It's actually easy to confirm now that we know it's NotOpen and should work magically: implement as we said (just mutableCopy, change the url, then copy and call [client wasRedirected…] etc) and let it run its course. If, after the redirection, the stub you set in your app ends up receiving the final response with the expected body (and not an empty one) then it's ok.

Bottom line: I probably worried for nothing, sorry for all the alarms and complex explanations for nothing ^^

[mikelupo]

@AliSoftware, no need to apologize. This is the exact purpose of this forum. To discuss (sometimes in detail) the change that is being presented. We learned some good things from this exercise.

So with that said, is this code ok as-is now? or do I need to make some more changes?
Please advise...

Thanks!

[AliSoftware]

Well we still need to make the mutableCopy + url = redirectLocationURL + copy dance, as for now the code still create a new request and set the method instead of doing a copy of the original request.

[AliSoftware]

This code means that 303 will not work anymore as the URL will not be replaced or changed in the case of 303

[AliSoftware]

This would be nice to have unit tests for these redirect behaviors, especially to not forget cases like 303 and ensure we don't break those RFC implementations in the future.

[mikelupo]

Good catch!

…

Sent from my iPhone

On Oct 16, 2017, at 2:29 PM, AliSoftware ***@***.***> wrote: @AliSoftware requested changes on this pull request. This code means that 303 will not work anymore as the URL will not be replaced or changed in the case of 303 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[AliSoftware]

This isn't needed anymore as we do a mutableCopy on line 427 now so the HTTPMethod is already the same as the original

[AliSoftware]

So in summary the behaviour we want to implement I think is:

• 301, 302, 307, 307: do the mutableCopy + mReq.URL = redirectLocationURL + copy dance like you already do here
• default, especially for 303: redirectRequest = [NSURLRequest requestWithURL:redirectLocationURL]; — instead of the copy (as, according to the RFC, 303 exists to tells that "the original request has been accepted and processed by the server and we now need to go to a new location to continue", so we must forget the old request (and not copy anything from it) and create a brand new one

[mikelupo]

Got it. I'll make that change. My code and your comment crossed on the wire.

[AliSoftware]

… and don't forget to add tests for all this, given the time we took to discuss it and check the expected behaviour, better not break it in the future 😉

[AliSoftware]

303 shouldn't make a copy of the original request, as the intent according to RFC is to tell that the first request has been processed correctly and that we should now continue to a new request

[AliSoftware]

This = nil; isn't needed as all path below will affect it a value anyway. (By not affecting it to nil, the compiler will warn us in case we later change the code and forget to affect a value in one of the code paths)

[mikelupo]

I almost didn't do that. I'll change it and resubmit. A clever strategy... Thanks, Mike

-----Original Message----- From: AliSoftware <notifications@github.com> To: AliSoftware/OHHTTPStubs <OHHTTPStubs@noreply.github.com> Cc: mikelupo <mikelupo@aol.com>; Mention <mention@noreply.github.com> Sent: Mon, Oct 16, 2017 3:41 pm Subject: Re: [AliSoftware/OHHTTPStubs] Fix redirect handling to respect RFC and HTTP/1.1 RFC 7538 (#263) @AliSoftware commented on this pull request. In OHHTTPStubs/Sources/OHHTTPStubs.m:

@@ -424,7 +424,28 @@ - (void)startLoading

// Notify if a redirection occurred if (((responseStub.statusCode > 300) && (responseStub.statusCode < 400)) && redirectLocationURL) { - NSURLRequest* redirectRequest = [NSURLRequest requestWithURL:redirectLocationURL]; + NSURLRequest *redirectRequest = nil; + NSMutableURLRequest *mReq = nil; This = nil; isn't needed as all path below will affect it a value anyway. (By not affecting it to nil, the compiler will warn us in case we later change the code and forget to affect a value in one of the code paths) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[mikelupo]

Once we settle on this, Shall I squash the commits down into a single thing and resubmit?

-----Original Message----- From: AliSoftware <notifications@github.com> To: AliSoftware/OHHTTPStubs <OHHTTPStubs@noreply.github.com> Cc: mikelupo <mikelupo@aol.com>; Mention <mention@noreply.github.com> Sent: Mon, Oct 16, 2017 3:41 pm Subject: Re: [AliSoftware/OHHTTPStubs] Fix redirect handling to respect RFC and HTTP/1.1 RFC 7538 (#263) @AliSoftware commented on this pull request. In OHHTTPStubs/Sources/OHHTTPStubs.m:

@@ -424,7 +424,28 @@ - (void)startLoading

// Notify if a redirection occurred if (((responseStub.statusCode > 300) && (responseStub.statusCode < 400)) && redirectLocationURL) { - NSURLRequest* redirectRequest = [NSURLRequest requestWithURL:redirectLocationURL]; + NSURLRequest *redirectRequest = nil; + NSMutableURLRequest *mReq = nil; This = nil; isn't needed as all path below will affect it a value anyway. (By not affecting it to nil, the compiler will warn us in case we later change the code and forget to affect a value in one of the code paths) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[AliSoftware]

I don't mind keeping the commit history.

Don't hesitate if you need help implementing the unit tests 😉

[mikelupo]

I definitely need help writing the unit tests. Current unit tests seem to be only checking one status. A 301.

[AliSoftware]

I think basically you can generalise the unit test for 301 to:

• Turn that test into a more generic function taking the httpMethod to use for the testing request, the optional body, and the status code to return.
• Then make some tests calling that function, respectively with { 301, 302, 307 and 308 } and with { GET, HEAD, POST, PATCH } (so 4x4 = 16 tests but which are simply all calling that generic function with different parameters). That way we'll not only test just 301+GET, but all other combinations
• Improve that function so it also add some assertions to check that the body of the returned redirect is the same as the original, and same for the method…

[mikelupo]

Thanks @AliSoftware. I am actively working on it between other tasks so my submission may be delayed.

[mikelupo]

@AliSoftware I have generalized the _test_redirect method and it's now accepting parameters. Where I'm lacking is the improvements that you mentioned,

"add some assertions to check that the body of the returned redirect is the same as the original, and same for the method".

Could use some help with that part.

[AliSoftware]

Maybe here instead of returning an empty NSData, we could use a data for the redirect that is computed from the original httpBody (maybe not exactly the same data to be sure not to confuse them both, but maybe one derived from it) — And for those tests, don't use the json parameter then.

So make those stub be so that:

• for the original request (the stub on that line 123), we send a redirect response (with httpStatusCode, being 301, 302, …) with the body derived from the request: stubbedRedirectResponse.httpBody = f(originalRequest.httpBody).
• Then the second stub handling the redirect (below, line ~129), we make it so the stubbed final response has a body re-derived from the redirect body: finalResponse.httpBody = g(redirectResponse.httpBody) = g(f(originalRequest.httpBody))

That way, once in the completion block at the call site, we can look at the resulting body and do some assertions there.

For example f can be so it returns a simple JSON representation of a dictionary like { "originalBody": originalBodyHere, "originalMethod": originalMethodHere }. Then g can be so it returns a JSON wrapping that first JSON, like { "redirectedBody": { "originalBody": originalBodyHere, "originalMethod": originalMethodHere }, "redirectedMethod": redirectedMethodHere }.
That would make the assertions in the actual tests below easy to ensure we didn't lose anything in the way.

[mikelupo]

I had a comment here, but it became a bit irrelevant after taking a break. I've been looking at this code for too long now. ;) So I deleted it.

[AliSoftware]

Haha too late, I saw it in my email notifications 😂
(Thanks for telling me you deleted it though, as I would probably have gone crazy trying to find it on GitHub after receiving the email notification otherwise 😄)

[mikelupo]

So do we need to follow redirects in the context of these tests? I would think so.

[AliSoftware]

Indeed I think so too

[mikelupo]

ok, we have two "withStubResponse" methods here. One for the original redirect, one for the second response (from the redirected request) for the path, @"/elsewhere". In the first redirected response, I'm trying to hijack the data payload with my own JSON stuff to keep track of originating request stuff (I think this is what you were wanting me to do). Then put that hijacked data payload on the wire for the redirected request, and hence, when we finally get the 200 response, we should have our built up JSON. However, it seems that apple is ignoring my change in data. I'm getting the original data posted all the way through. This isn't bad behavior and it's preventing man-in-the-middle attacks. It seems I won't be able tweak the HTTPBody in the response.
Thoughts?

[AliSoftware]

I was actually thinking about it the other day. The way we implemented it is that when there is a redirect, we do redirect with the same body as the original. That's the whole point of your PR. That's what you implemented. When there's a 301, we redirect to the new URL but with the original request's body. So that's our implementation after all.

So it's logical, from that implementation we did, that, whatever body we return in the redirection response, our implementation redirects to the new URL with the original body after all 😉

More than that, I think in our reasoning for those tests we confused the request and the response. In the real world, when you post a request to the server with some http body B1, if the server returns a 301 response, that 301 response doesn't have any http body itself. It's the server's redirect response after all

So in our specific context:

• the first stub should just return an OHHTTPStubsResponse with status code 301 (and no http body whatsoever, that body would be ignored anyway, and we respect the RFC on that)
• then the second stub is supposed to be the final response that the server would return, when queried on the redirected URL with that original body. So there we could make our fake response to be some JSON wrapping the original body in that case (like if we had a real server whose only role was to echo what we send to it, just wrapped inside a JSON)

[mikelupo]

For the other response codes as well ...should we return nil for the body in the first stub or only in 301? Edit: We can't return nil for data as the stub won't allow returning a nil,

[AliSoftware]

I think all redirect responses are supposed to have an empty body

[AliSoftware]

Don't forget to remove that NSLog once you've done with experimenting on that test before we merge 😉

[mikelupo]

It's already out in my code. 🥇

[AliSoftware]

So for example here if we tweak our test_redirect_NSURLSession… method so that it returns a response body derived from the dataToPost, we can now check if the jsonResponse — which should be derived from that g(f(dataToPost)) that we initially sent — matches what is expected.

If our redirect lost the httpBody during that redirect, that means that we would lose the response derived from it and the assertion would fail.

---

That's just a random idea, I'm on the train rn with little data so haven't had the leisure of looking into it in details, I hope you'll see the idea.

[AliSoftware]

@mikelupo ping 😉

Any chance you'll find some time to continue working on it soon?
I'd love to make a new release at some point and would like to include your PR in it 👍

[mikelupo]

Hi, I keep planning to work on it, but my daily job keeps getting in the way! ;P I'll make a push to try and get this done before middle next week. Feel free to ping me any time to cattle-prod me. Mike

…

-----Original Message----- From: AliSoftware <notifications@github.com> To: AliSoftware/OHHTTPStubs <OHHTTPStubs@noreply.github.com> Cc: mikelupo <mikelupo@aol.com>; Mention <mention@noreply.github.com> Sent: Thu, Oct 26, 2017 3:06 pm Subject: Re: [AliSoftware/OHHTTPStubs] Fix redirect handling to respect RFC and HTTP/1.1 RFC 7538 (#263) @mikelupo ping 😉 Any change you'll find some time to continue working on it soon? I'd love to make a new release at some point and would like to include your PR in it 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[AliSoftware]

👌

[mikelupo]

@AliSoftware I posted Unit Tests for review. I hope that I have correct thinking as far as this part of the PR goes.
Thanks!
Edit: I also see the conflict on CHANGELOG.md and will resolve it when we're ready to merge the commit.

[AliSoftware]

Wait… now that I read your implementation, I realize that we can in fact do that test in a way simpler way: no need for that JSON dictionary with originalMethod & originalBody and all. What we just want to assert is that once we've replied with a redirect, the next request will have the same body as the original

So basically we can remove all that new code with your __block variables and the build JSON dict and just do something like (pseudo-code, as I'm in the train and live-typing that from the web interface):

__block NSData* request1Body = nil;
__block NSString* request1Method = nil;
// Reply with a redirect
[OHHTTPStubs stub path "" with : {
  // Memorize the body and method of the original request
  request1Body = request.ohhttpstubs_HTTPBody;
  request1Method  request.HTTPMethod

  // Then just reply with header "Location: …/elsewhere" and empty HTTPBody.
  // i.e. just as usual redirect, in fact what we previously did before that PR, no JSON dict to build or whatnot
  // A normal, official redirect in HTTP is just a 3xx header and no body anyway
  return OHHTTPStubsResponse(headers, 3xx code, empty body)
}

// Then also stub the final response done after the redirect
[OHHTTPStubs stub "/elsewhere" with: {
  // That's what we want to test:
  // that once OHHTTPStubs has handled the redirection,
  // the new request created to the new location has the same
  // HTTPBody (and potentially method) than the original request, so:
  XCTAssertEqual(request.ohhttpstubs_HTTPBody, originalRequestBody)
  XCTAssertEqual(request.ohhttpstubs_HTTPBody, originalRequestMethod)
  // then continue the code as it was before the PR

So basically, the easiest way to go there is to undo the changes you did to _test_redirect_NSURLSession (sorry for the misleading!) and instead just store the method+body when stubbing the original request, and assert they're the same in the redirected request on the second stub. The rest (especially the response those stubs return) should be unchanged (same as before the PR), the only change is to take advantage of the stub to additionally store the original request method/body (first stub) / additionally compare the new request's method/body to the stored one (second stub).

Hope that's more clear with this. No more temp JSON with "originalBody"/"originalMethod" keys, no change to the responses returned by the stubs, no need to pass an httpBody parameter to that _test_redirect_NSURLSession helper method, none of that.

Sorry again for the confusion, I think we both mixed everything up with all those requests and responses and all

[AliSoftware]

You can probably use our request.ohhttpstubs_HTTPBody extension there instead of reading the stream and then avoid this whole else block?

[AliSoftware]

Curious why you used NSJSONSerialization for going from Data to JSON on line 143 but used NSKeyedArchived (and not NSJSONSerialization dataFromJSONObject) to go the other way around?

Also, you might just use the base64 of the body instead of deserializing it (in case that original body is not actual JSON for example it would still work)

[mikelupo]

Thanks for taking a hard look at the code. I'll address your interesting comments soon.
Recovering here from a bad storm and multi-day power outage.

[mikelupo]

Checking in. I've not forgotten. I'm just busy on some other tasks that are directly work related.

[AliSoftware]

No problem. Just hope you saw I took a stab at it so it's implemented now… and so we've swapped roles then, as it'll be your turn to review 😁

Good luck with work

[AliSoftware]

pinging 😉

[AliSoftware]

@mikelupo I took the liberty to rebase and give a try at those tests. It's indeed easy to get lost and lose track between all those multiple requests & responses in all redirects when trying to figure how to write that… 😓
I hope I got it right. I'll let you review my commit when you have some time 😉

[Liquidsoul]

LGTM
I've just a comment about some assert texts 🤓

[Liquidsoul]

Maybe it's me but, I find the message of this assert not clear. Something more like "Expected the method to be unchanged"?

[AliSoftware]

Good point, fixed.

[AliSoftware]

Seems like Travis-CI randomly fail 50% of the time on:

✗ test_NSURLSessionDefaultConfig_MethodAndDataRetentionOnRedirect, ((jsonError) == nil) failed: "Error Domain=NSCocoaErrorDomain Code=3840 "No value." UserInfo={NSDebugDescription=No value.}" - Unexpected error deserializing JSON response

At first I thought it was just Travis being Travis (it has been quite unstable for some time) but even when restarting jobs, some pass and some continue failing with the same "No value" CocoaError, so maybe that's a real problem that we gonna have to investigate (likely a race condition between all the requests and redirects done simultaneously but the test only having one shared variable at some place in that test method?!)

[mikelupo]

@AliSoftware Thank you for taking this up in my absence! 👍