Closes #1976, HttpUrlConnection now allowed to follow redirects #2027
Conversation
|
Hey @grubdragon – we're on vacation for the holidays so at worst, we'll get to this when we come back around 1/5. btw, from a glance, the code looks good but I'd like to test it, which is why I can't just land it now. |
|
Yeah cool! I'll try to fix some other bugs till then! 😄 |
@grubdragon Have you found enough bugs or do you want me to find something for you? I'll get around to reviewing this soon. |
There was a problem hiding this comment.
Turns out this was happening by default (as mentioned in the docs) but it's good to be explicit!
I tested this with redirection on these redirect pages: https://jigsaw.w3.org/HTTP/300/Overview.html
Looks good with the change I suggested!
| @@ -219,6 +219,7 @@ private void showServerError(final ManualAddSearchEngineSettingsFragment that) { | |||
| HttpURLConnection connection = null; | |||
| try { | |||
| connection = (HttpURLConnection) searchURL.openConnection(); | |||
| connection.setInstanceFollowRedirects(true); | |||
| connection.setConnectTimeout(SEARCH_QUERY_VALIDATION_TIMEOUT_MILLIS); | |||
| connection.setReadTimeout(SEARCH_QUERY_VALIDATION_TIMEOUT_MILLIS); | |||
There was a problem hiding this comment.
A few lines below this, we check against responseCode < 400 – we should check < 300 now because we're following redirects. If we get a 300 even after following redirects, something must be crazy.
There was a problem hiding this comment.
Ahh right, covering all bases
|
@mcomella Hey could you suggest me some bugs? |
| @@ -223,8 +223,8 @@ private void showServerError(final ManualAddSearchEngineSettingsFragment that) { | |||
| connection.setConnectTimeout(SEARCH_QUERY_VALIDATION_TIMEOUT_MILLIS); | |||
| connection.setReadTimeout(SEARCH_QUERY_VALIDATION_TIMEOUT_MILLIS); | |||
|
|
|||
| // A non-error HTTP response is good enough as a sanity check, some search engines redirect to https. | |||
| return connection.getResponseCode() < 400; | |||
| // Now that redirects are followed, 300 is a better and stronger sanity check, checks for a non error and non redirect response | |||
There was a problem hiding this comment.
nit: general coding tip: this comment references the old state of the code "now that redirects...", "...is a better..." but someone reading this code has no idea what the previous code looked like. Thus, comments should either:
- Only reference the current code
- Explain the previous state of the code and describe why the current code doesn't do that anymore.
In this case, a dev doesn't need to understand the old state to understand the code so I'd only reference the current code (e.g. "We follow redirects, check for success codes only"). Note that your comment could be a good extended commit message though! That's usually the place to write, "I changed this code for these reasons".
That being said, I don't think this is worth creating another commit for - I'm just going to land it.
@grubdragon I saw you commenting on #1085 and #1433 – those sound like they'd be good to tackle! Thanks for hacking on code with us! :) |
| // A non-error HTTP response is good enough as a sanity check, some search engines redirect to https. | ||
| return connection.getResponseCode() < 400; | ||
| // Now that redirects are followed, 300 is a better and stronger sanity check, checks for a non error and non redirect response | ||
| return connection.getResponseCode() < 300; |
There was a problem hiding this comment.
Actually, this change I request breaks the tests: https://travis-ci.org/mozilla-mobile/focus-android/builds/325730045?utm_source=github_status&utm_medium=notification since the test explicitly checks request codes in 3xx – do you mind updating the tests too? Thanks!
There was a problem hiding this comment.
I'm not sure if I've done that right though
| // Currently we accept redirects as valid. We might want to follow the redirect (Issue #1976) | ||
| assertTrue(isValidSearchQueryURL(it.rootUrl())) | ||
| fun `URL using HTTP redirect is invalid`() = withMockWebServer(responseWithStatus(301)) { | ||
| // We now follow redirects(Issue #1976). This test now asserts false. |
There was a problem hiding this comment.
nit: similar to my comment above, this comment references the past state of the code, "We now follow redirects... This test now asserts...". I think a better description might be: "We follow redirects so we should never receive a 3xx. If we do, we want to fail." Or something.
But I don't think it's worth the turn around time to fix now.
There was a problem hiding this comment.
Will keep in mind the next time 😄 Thanks for helping me throughout!
@pocmo pointed out that instead of accepting a redirect as successful, we could follow redirects.