UpdaterUtil: follow HTTP redirects (issue #65)

[carandraug]

Recursively follow redirects in case of 301, 302, and 307 codes. Make
construction of user agent value separate to avoid repeated calls.

This seems to fix the issue for the case of the SIMcheck update site which is how I'm testing it. However, it seems to have introduced another bug which I can't make sense of. To reproduce this new issue:

1. change the URL for the SIMcheck update site to http (to force the redirect)
2. re-enable the SIMcheck site
3. modify the SIMcheck URL to use https
4. this is failing with:

Exception in thread "AWT-EventQueue-0" java.lang.IllegalStateException: connect in progress
	at sun.net.www.protocol.http.HttpURLConnection.setRequestMethod(HttpURLConnection.java:550)
	at sun.net.www.protocol.https.HttpsURLConnectionImpl.setRequestMethod(HttpsURLConnectionImpl.java:383)
	at net.imagej.updater.util.UpdaterUtil.getLastModified(UpdaterUtil.java:382)
	at net.imagej.ui.swing.updater.SitesDialog.validURL(SitesDialog.java:441)
	at net.imagej.ui.swing.updater.SitesDialog$1$1.stopCellEditing(SitesDialog.java:148)
	at javax.swing.plaf.basic.BasicTableUI$Actions.actionPerformed(BasicTableUI.java:503)
[...]

Reading online, this seems to often be caused by reusing HttpURLConnection but it doesn't look like I've doing it (but I'm not much of a Java developer and I may be missing something obvious).

[carandraug]

I just noticed that while this means that the redirect is used to get the db.xml.gz file, I get an "Already Connected" error when it actually tries to download the updates. I'm guessing this is the same thing that's causing the "connect in progress" error.

[carandraug]

I think I found all the issues and seems to be working now. The problem is that openConnection now returns a connection that already happened. This is required to find out whether that connection is a redirect. This means that one can't configure the returned connection such as the request type. This makes the change backwards incompatible.

[carandraug]

There was one other case where it tried to modify the open connection which I removed now.

An alternative to not have the backwards incompatible issue, is to have openConnection follow any redirects, and then open a new connection with the final URL once there's no more redirects to do.

[carandraug]

ping?

[ctrueden]

@frauzufall What do you think? Should we merge this?

[frauzufall]

It's time for sure ;-) I did not already do it because of this note:

I think I found all the issues and seems to be working now. The problem is that openConnection now returns a connection that already happened. This is required to find out whether that connection is a redirect. This means that one can't configure the returned connection such as the request type. This makes the change backwards incompatible.

I like the proposed (not yet implemented) solution by @carandraug to follow redirects and then reopen the connection with the final URL, but that would mean we would have twice as many update requests as we currently have. What do you think about that @ctrueden? In case you agree, I'd like to add this and clean up the commits. How can I do this best if I don't have permission to write to the branch, just work with @carandraug's commits on a new branch in this repo?

[imagejan]

@frauzufall wrote:

How can I do this best if I don't have permission to write to the branch

If @carandraug left this little box "Allow edits from maintainers" checked (which by default it is) when opening the PR, you should have push permission to this branch. Did you try?

Concerning the number of update requests: that's only true for installations that still have http configured and have to follow the redirect, right? Installations with an updated https URL (i.e. no redirect needed) could proceed with the original connection, no?

[frauzufall]

Our move from HTTP to HTTPS does not involve redirects. This PR is relevant to sites not hosted on imagej.net which could include redirects.

The openConnection method as implemented in this PR already establishes the connection here to find out if the return code is a redirect. Which means he had to remove instances where there the request method is modified after calling openConnection (e.g. here). So the suggestion was to try if the return code is 200 and if it is, return a new non established connection where one can still append headers which would be a backwards compatible change. But that would make openConnection always calling the final address twice. Correct me if I'm wrong.

Another (non backwards compatible) idea is to add a openHeaderConnection covering the cases in the updater where we are changing the request after calling openConnection and for both methods return the final established connection.

If @carandraug left this little box "Allow edits from maintainers" checked (which by default it is) when opening the PR, you should have push permission to this branch. Did you try?

Awesome, sorry I did not know about that, will try.

[frauzufall]

Maybe relevant to the discussion: java.net.URL::openConnection also does not establish the connection:

It should be noted that a URLConnection instance does not establish the actual network connection on creation. This will happen only when calling URLConnection.connect().

So I'm leaning towards having two new methods (establishConnection and e.g. establishHeaderConnection or establishConnection(String requestType)) which follow redirects and deprecate openConnection (or keep it the way it is, have to see how implementing this will play out)

[ctrueden]

Regarding doubling the number of requests, I do not feel strongly. And HEAD requests are fast. But I am no expert on web performance so maybe it is not a good idea. It seems intuitively obvious that if we can avoid the second request in the common case where no redirect needs to be followed, we should do so.

[carandraug]

@frauzufall wrote:

Our move from HTTP to HTTPS does not involve redirects. This PR is
relevant to sites not hosted on imagej.net which could include
redirects.

URL redirection is pretty standard. If imagej.net ever needs to be
restructured, having the updater already supporting redirects will be
useful.

---

I don't think I can offer much more input. Would be nice to see this
merged even though we no longer need it (it was needed more than a
year ago when we moved our sites but by now, all affected users have
reinstalled ImageJ to get the new update site).