Update Apache HttpComponents Client dependency adding support for HTTP2. #515
Conversation
|
The redirect strategy is meant to handle the additional HTTP methods from WebDAV that are not handled in the default implementation from HTTP components. Thus a server can reply with a redirect for |
|
I just stumbled across the same problem and I would also like this feature.
I did not check, the code here. But is there a unit test, that verifies this? Maybe I can assist |
Unfortanatly there is no unit tests for it, I would be very happy if you would find a proper solution for redirect stratagy functionality. I am following up this thread and ready to help you to test it. |
dkocher
force-pushed
the
feature/ApacheClient5
branch
from
f34c128 to
a3531b6
Compare
|
To me it looks like the custom redirect strategy is no longer required as
Footnotes
|
dkocher
force-pushed
the
feature/ApacheClient5
branch
from
a3531b6 to
49a37c9
Compare
dkocher
changed the title
|
@dkocher I agree with this, but it changes the Sardine API. I just want to confirm that this will be a MAJOR version change on the library. |
|
@dkocher That link isn't loading for me, but what I'm thinking about are the few places where things change from String -> char[]. Essentially all the chances in Sardine.java. |
|
Wondering if this is still interoperable with Sharepoint WebDAV using NTLM.
|
| @@ -548,6 +547,8 @@ public interface Sardine | |||
| */ | |||
| List<String> getPrincipalCollectionSet(String url) throws IOException; | |||
|
|
|||
| void enableHttp2(); | |||
There was a problem hiding this comment.
Missing Javadoc describing implications.
| return DateUtils.parseDate(value, SUPPORTED_DATE_FORMATS); | ||
| final Instant parsedInstant = DateUtils.parseDate(value, Arrays.stream(SUPPORTED_DATE_FORMATS) | ||
| .map(pattern -> DateTimeFormatter.ofPattern(pattern, Locale.US).withZone(ZoneId.of("GMT"))).toArray(DateTimeFormatter[]::new)); | ||
| if (null == parsedInstant) |
| // Backward compatibility | ||
| failure.initCause(e); | ||
| throw failure; | ||
| throw new IOException("Not a valid DAV response", e); |
| sardine.delete(url); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Are we removing this test because it is no longer necessary or just don't want to fix it or...
| @@ -34,6 +34,7 @@ public class SardineUtilTest | |||
| @Test | |||
| public void testParseDate() throws Exception | |||
| { | |||
| assertNotNull(SardineUtil.parseDate("1970-01-01T12:00:00Z")); | |||
There was a problem hiding this comment.
what's the point of this test? would be good to document the "why"...
| if (username != null) | ||
| { | ||
| provider.setCredentials( | ||
| new AuthScope(AuthScope.ANY_HOST, AuthScope.ANY_PORT, AuthScope.ANY_REALM, AuthSchemes.NTLM), | ||
| new AuthScope(null, null, -1, null, StandardAuthScheme.NTLM), |
There was a problem hiding this comment.
Why are we removing the constants?
| new GzipDecompressingEntity(entity)); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Is there no longer a "simple" way to do this?
| @@ -390,7 +399,6 @@ public List<DavResource> list(String url, int depth, java.util.Set<QName> props) | |||
| prop.setGetcontenttype(objectFactory.createGetcontenttype()); | |||
| prop.setResourcetype(objectFactory.createResourcetype()); | |||
| prop.setGetetag(objectFactory.createGetetag()); | |||
| prop.setLockdiscovery(objectFactory.createLockdiscovery()); | |||
| body.setLockscope(scopeType); | ||
| Locktype lockType = new Locktype(); | ||
| lockType.setWrite(new Write()); | ||
| body.setLocktype(lockType); |
| @@ -861,25 +868,25 @@ public void put(String url, InputStream dataStream, String contentType, boolean | |||
| @Override | |||
| public void put(String url, InputStream dataStream, String contentType, boolean expectContinue, long contentLength) throws IOException | |||
| { | |||
| InputStreamEntity entity = new InputStreamEntity(dataStream, contentLength); | |||
| InputStreamEntity entity = new InputStreamEntity(dataStream, contentLength, contentType != null ? ContentType.create(contentType) : null); | |||
There was a problem hiding this comment.
Should we add a helper function since this is duplicated a bunch of times? Would be good to effectively document this.
| { | ||
| this(URI.create(uri)); | ||
| this(URI.create(url)); |
There was a problem hiding this comment.
Why rename this to url when it is called by URI.create(...)?
Hi,
Here are the changes to support Apache Client 5. As discussed in the related issue, some methods now function quite differently. I have successfully migrated the main business logic and tests, but there is still one class that I am unable to rewrite due to a lack of knowledge about the previous implementation (src/main/java/com/github/sardine/impl/SardineRedirectStrategy.java).
I would appreciate any ideas from the community on how to address this issue or any assistance in resolving it.
Additionally, I introduced the enableHttp2 method in the Sardine interface.
I will monitor this pull request discussion and aim to finalize it as soon as possible.
Fixes #333