Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

Closes #1976, HttpUrlConnection now allowed to follow redirects #2027

Merged
merged 3 commits into from
Jan 9, 2018
Merged

Closes #1976, HttpUrlConnection now allowed to follow redirects #2027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c96fe3
Closes #1976, HttpUrlConnection now allowed to follow redirects
grubdragon Dec 28, 2017
902c150
Changed the sanity check on the HttpURLConnection, now compares with 300
grubdragon Jan 6, 2018
829806c
Changed SearchEngineValidationTest, since redirects are followed, 3xx…
grubdragon Jan 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/src/main/java/org/mozilla/focus/settings/ManualAddSearchEngineSettingsFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,12 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right, covering all bases


// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return connection.getResponseCode() < 300;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I've done that right though


} catch (final IOException e) {
// Don't log exception to avoid leaking URL.
Expand Down
6 changes: 3 additions & 3 deletions app/src/test/java/org/mozilla/focus/settings/SearchEngineValidationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ class SearchEngineValidationTest {
}

@Test
fun `URL using HTTP redirect is valid`() = withMockWebServer(responseWithStatus(301)) {
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@grubdragon grubdragon Jan 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will keep in mind the next time 😄 Thanks for helping me throughout!

assertFalse(isValidSearchQueryURL(it.rootUrl()))
}

@Test
Expand Down