fix: redirect on 308 (Permanent Redirect) too #876
Conversation
googlebot
added
the
cla: yes
google-http-client/src/test/java/com/google/api/client/http/HttpStatusCodesTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| public void testIsRedirect_3xx() { | ||
| assertTrue(HttpStatusCodes.isRedirect(301)); | ||
| assertTrue(HttpStatusCodes.isRedirect(302)); |
There was a problem hiding this comment.
what about 309 to 399? These are all undefined redirect codes according to HTTP.
There was a problem hiding this comment.
I think the method name should really be isDefinedRedirect or such, given where this code is used to handle automatic redirection. The Javadoc at least describes the behavior correctly.
There was a problem hiding this comment.
The behavior for other 3xx codes is not defined in docs or tests. I suspect the current behavior in this case never got much thought.
Think of it this way: if we do see a 315 code what should we do with it? The answer is treat it as a redirect.
There was a problem hiding this comment.
I don't disagree. The library could be modified to redirect on all 3xx, but I don't feel like I will introduce that kind of behavior change myself right now. That sounds like we need more discussions, and I don't feel confident to introduce such a change, as I don't think I have enough expertise on this matter. For now, I will just allow redirecting 308 that at least should have been covered here. Nothing more, just one change that I believe should be fixed at least.
chanseokoh
changed the title
chanseokoh
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes #873.