Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix HTTPS upload #3

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@frauzufall
Copy link
Member

commented Feb 7, 2019

This is an attempt to fix uploading via HTTPS webdav which multiple people reported.

The webdav uploader contains a reflection hack enabling webdav methods such as PROPFIND. It only works with the HttpURLConnection, not with HttpsURLConnection.

I exchanged it with another reflection hack that is supposed to work for both [src].

Also see
Bug report for Java 8: https://bugs.openjdk.java.net/browse/JDK-7016595
Should be fixed in new API (Java 11): https://dzone.com/articles/java-11-standardized-http-client-api

Still experiencing (new) errors when trying to upload though so this is not a finalized PR.

frauzufall added some commits Feb 6, 2019

Attempt to fix HTTPS Uploading
* removes reflection used for setting connection request method
Adds reflection enabling webdav methods
* this is a reflection method enabling webdav methods both for HTTP and
HTTPS
* source: https://stackoverflow.com/a/39641592
@frauzufall

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

@ctrueden I added a dependency to a SNAPSHOT version of jackrabbit-webdav. Seems that 3.0-SNAPSHOT is from 2014 though. I wanted to change it to a release version, but the older 2.X versions work different and I like the current implementation. Can we keep it or should I investigate further?

@ctrueden

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Can we keep it or should I investigate further?

Two options:

  1. Find out why jackrabbit-webdav was discontinued. Is there a replacement? Is there another wagon that works better these days? If so, test it out, and hopefully switch to it.
  2. Assuming jackrabbit-webdav 3.0-SNAPSHOT is really what we continue to prefer, we can deploy our own release version (fork) to our Maven repository and use that.
@imagejan

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

The latest 2.x version of jackrabbit-webdav is 2.19.1 and it seems to be from Feb 2019:
https://mvnrepository.com/artifact/org.apache.jackrabbit/jackrabbit-webdav
Why do you call it "old"?

And where did you find a 3.0.0-SNAPSHOT, @frauzufall? The link you posted gives a 404 error for me.

<relativePath />
</parent>

<groupId>net.imagej</groupId>
<artifactId>imagej-plugins-uploader-webdav</artifactId>
<version>0.2.3-SNAPSHOT</version>
<version>0.3.0-SNAPSHOT</version>

This comment has been minimized.

Copy link
@imagejan

imagejan Feb 28, 2019

Member

Can we define properties for these and then use them as <version>${imagej-plugins-uploader-webdav.version}</version>, to allow them to be overridden by tooling such as the melting pot script?

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

For the parent and artifact <version> values we do not (and should not) use properties. But for dependency version declarations, we should!

pom.xml Outdated
@@ -98,6 +98,7 @@ Institute of Molecular Cell Biology and Genetics.</license.copyrightOwners>
<dependency>
<groupId>net.imagej</groupId>
<artifactId>imagej-updater</artifactId>
<version>0.10.0-SNAPSHOT</version>

This comment has been minimized.

Copy link
@imagejan

imagejan Feb 28, 2019

Member

Use version <property>, see comment above?

pom.xml Outdated
@@ -124,6 +131,7 @@ Institute of Molecular Cell Biology and Genetics.</license.copyrightOwners>
<artifactId>imagej-updater</artifactId>
<classifier>tests</classifier>
<scope>test</scope>
<version>0.10.0-SNAPSHOT</version>

This comment has been minimized.

Copy link
@imagejan

imagejan Feb 28, 2019

Member

Use version <property>, see comment above?

@frauzufall

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

When we searched for the best way to use webdav from Java, apache jackrabbit came up a lot, and @imagejan you are right, it's updated frequently. I think I searched for the latest version on maven.imagej.net and somehow only saw snapshots so I picked what I thought would be the newest (corrected the link in the text above). That was a bad decision. I know now that the maven mirror only gets populated by what people request.. The 3.0er version is super convenient to use and matched examples I found, that's why I did not recognize the issue earlier. But I will have a closer look at how to migrate to 2.19.1.

EDIT I found a post in the mailing list archive explaining the status of 3.0-SNAPSHOT. I might just ask there what the best strategy is.

POM update: jackrabbit-webdav-2.12.10, using properties for versioning
* jackrabbit-webdav-2.12.10 is on an equivalent dev stage as 3.0-SNAPSHOT
* will need to be updated to latest jackrabbit-webdav version, but removes SNAPSHOT dependency for now
@frauzufall

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Found equivalent release version 2.12.10 for jackrabbit-webdav that is not a SNAPSHOT. Will look into how to update this to the newest version, now that I can follow the release changes. @imagejan I added the properties, thanks for the comment, makes a lot of sense. Trying out the melting pot is on my TODO list..

axtimwalde and others added some commits Feb 28, 2019

Update to Apache HttpClient 4.5.6, jackrabbit-webdav 2.19.1
* rewritten WebDAVUploader to fit Apache HttpClient 4.x API
* tested for digest and basic authentication
* added some tests (which still require url and credentials system
properties, therefore no automated Travis testing?)
* moved the DELETE method from tests to WebDAVUploader
* made a couple of WebDAVUploader method package private for testing
@frauzufall

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Updated the code to the newest jackrabbit-webdav version which meant rewriting the whole thing to be compatible with Apache HttpClient 4. I also updated https://sites.imagej.net/UpdaterBeta/ where one can test it. Now I have no further points on my agenda for this PR.

@axtimwalde I read your last commit about merging the branches on this repo and on juglab and thought I could push to imagej/fix-https-upload, but that created a new branch. So I also pushed to juglab/fix-https-upload to make the changes appear on this page. Sorry for the mess.. Should I delete the redundant imagej/fix-https-upload branch again?

@ctrueden
Copy link
Member

left a comment

Thanks very much for working on this! I made some comments. Minor things, except for the SNAPSHOT dependency which will need to be resolved in the history before we can merge.

<contributor>
<name>Deborah Schmidt</name>
<url>http://imagej.net/User:Frauzufall</url>
<roles><role>maintainer</role></roles>

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

If you are a maintainer, then you are not a <contributor> but rather a <developer>.

This comment has been minimized.

Copy link
@frauzufall

frauzufall Apr 16, 2019

Author Member

I would like to maintain this, yes. Do you agree or should I rather change my role?

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 16, 2019

Member

Wonderful—I wholeheartedly support additional maintainers on whichever components you feel are relevant to your work. Please see the SciJava team roles and add yourself as a <developer> with whichever of those roles are appropriate.

<relativePath />
</parent>

<groupId>net.imagej</groupId>
<artifactId>imagej-plugins-uploader-webdav</artifactId>
<version>0.2.3-SNAPSHOT</version>
<version>0.3.0-SNAPSHOT</version>

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

For the parent and artifact <version> values we do not (and should not) use properties. But for dependency version declarations, we should!


<!-- skip reproducible build enforcer during development -->
<imagej-updater.version>0.10.0-SNAPSHOT</imagej-updater.version>
<enforcer.skip>true</enforcer.skip>

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

The commits built on this snapshot version will need to be rewritten before this PR can be merged.

@@ -0,0 +1,95 @@
/*
* The MIT License (MIT)

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

This file should have our usual license on top, and then the original code's MIT license beneath it. Annoying I know, but that's third party attribution for you...


/**
* A conditional JUnit test for uploading via WebDAV.
*

This comment has been minimized.

Copy link
@ctrueden

ctrueden Apr 11, 2019

Member

Need <p> tags around subsequent paragraphs. Otherwise, the javadoc does not render as expected. And IDE javadoc cleanup functions will mush the lines together.

But thank you very much for writing very nice, detailed javadoc. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.