Update Apache HttpComponents Client dependency adding support for HTTP2. #515

PetrusHahol · 2024-08-20T09:32:28Z

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

dkocher · 2024-08-22T10:36:21Z

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 PROPFIND and the entity is properly sent to the redirect URI.

rPraml · 2024-09-26T09:32:28Z

I just stumbled across the same problem and I would also like this feature.

The redirect strategy is meant to handle the additional HTTP methods from WebDAV that are not handled in the default implementation from HTTP components

I did not check, the code here. But is there a unit test, that verifies this? Maybe I can assist

PetrusHahol · 2024-09-26T10:23:20Z

I just stumbled across the same problem and I would also like this feature.

The redirect strategy is meant to handle the additional HTTP methods from WebDAV that are not handled in the default implementation from HTTP components

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 · 2025-01-10T13:13:56Z

To me it looks like the custom redirect strategy is no longer required as DefaultRedirectStrategy.INSTANCE does

Follow redirects regardless of the request method ¹
Copy any request entity to the redirect request ²

lookfirst · 2025-01-10T17:28:53Z

@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 · 2025-01-10T17:49:57Z

@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.

Do you mean because of the return value of createDefaultRedirectStrategy ¹?

https://github.com/lookfirst/sardine/pull/515/files#diff-d0c6bff269a6dc37447fd635eba385548f6fe3f44394384ffe82ebb496a4ac0fR1172 ↩

lookfirst · 2025-01-10T19:28:11Z

@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.

	void setCredentials(String username, String password);
	void setCredentials(String username, char[] password);

dkocher · 2025-01-10T21:39:22Z

Wondering if this is still interoperable with Sharepoint WebDAV using NTLM.

@deprecated Do not use. the NTLM authentication scheme is no longer supported.

Consider using Basic or Bearer authentication with TLS instead.

dkocher · 2025-01-10T21:16:28Z

src/main/java/com/github/sardine/Sardine.java

@@ -548,6 +547,8 @@ public interface Sardine
 	 */
 	List<String> getPrincipalCollectionSet(String url) throws IOException;

+	void enableHttp2();


Missing Javadoc describing implications.

lookfirst · 2025-01-10T22:02:31Z

src/main/java/com/github/sardine/util/SardineUtil.java

-		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)


yoda logic.

src/main/java/com/github/sardine/util/SardineUtil.java

lookfirst · 2025-01-10T22:03:27Z

src/test/java/com/github/sardine/FunctionalSardineTest.java

-            sardine.delete(url);
-        }
-    }
-


Are we removing this test because it is no longer necessary or just don't want to fix it or...

Too lazy to fix the test…

lookfirst · 2025-01-10T22:04:11Z

src/test/java/com/github/sardine/SardineUtilTest.java

@@ -34,6 +34,7 @@ public class SardineUtilTest
 	@Test
 	public void testParseDate() throws Exception
 	{
+		assertNotNull(SardineUtil.parseDate("1970-01-01T12:00:00Z"));


what's the point of this test? would be good to document the "why"...

No particular reason for this addition. Was accidentally included when transitioning implementation.

lookfirst · 2025-01-10T22:05:12Z

src/main/java/com/github/sardine/impl/SardineImpl.java

 		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),


Why are we removing the constants?

The constants seem no longer to exist.

lookfirst · 2025-01-10T22:06:08Z

src/main/java/com/github/sardine/impl/SardineImpl.java

+						new GzipDecompressingEntity(entity));
+			}
+		});
+


Is there no longer a "simple" way to do this?

Should just toggle contentCompressionDisabled in the client builder instead.

There is no setter in the builder to enable. Can be just a no-op then as it is enabled by default?

src/main/java/com/github/sardine/impl/SardineImpl.java

lookfirst · 2025-01-10T22:07:13Z

src/main/java/com/github/sardine/impl/SardineImpl.java

-		body.setLockscope(scopeType);
-		Locktype lockType = new Locktype();
-		lockType.setWrite(new Write());
-		body.setLocktype(lockType);


Why removed?

Unused. Will open a separate pull request.

src/main/java/com/github/sardine/impl/SardineImpl.java

src/main/java/com/github/sardine/impl/methods/HttpAcl.java

dkocher

Should also review deprecated usage of ConnectionSocketFactory.

dkocher

Not sure if we should maintain a separate branch that retains the 4.x HttpComponents dependency.

nPraml · 2025-01-15T15:19:19Z

Hello everyone,

Thanks for the PR, we would also need sardine with http5.
I tried the http5 version (this branch) in our application and still have authentication issues.

Exception stack trace:

Caused by: com.github.sardine.impl.SardineException: status code: 401, reason phrase: Unexpected response (401 Unauthorized)
	at [email protected]/com.github.sardine.impl.handler.ValidatingResponseHandler.validateResponse(ValidatingResponseHandler.java:50)
	at [email protected]/com.github.sardine.impl.handler.MultiStatusResponseHandler.handleResponse(MultiStatusResponseHandler.java:41)
	at [email protected]/com.github.sardine.impl.handler.MultiStatusResponseHandler.handleResponse(MultiStatusResponseHandler.java:36)
	at [email protected]/org.apache.hc.client5.http.impl.classic.CloseableHttpClient.execute(CloseableHttpClient.java:247)
	at [email protected]/org.apache.hc.client5.http.impl.classic.CloseableHttpClient.execute(CloseableHttpClient.java:188)
	at [email protected]/com.github.sardine.impl.SardineImpl.execute(SardineImpl.java:1103)
	at [email protected]/com.github.sardine.impl.SardineImpl.execute(SardineImpl.java:1067)
	at [email protected]/com.github.sardine.impl.SardineImpl.propfind(SardineImpl.java:443)
	at [email protected]/com.github.sardine.impl.SardineImpl.list(SardineImpl.java:376)
	at [email protected]/com.github.sardine.impl.SardineImpl.list(SardineImpl.java:366)

How we use sardine:

Sardine sardine = new SardineImpl();
sardine.setCredentials(user, password.toCharArray());
sardine.list("http://localhost:34047/", 0); // randomized port number

The port number is randomized, so it's ok that there are different port numbers in the different code blocks, it just took me several runs until I collected all the information.

We create the instance on localhost with:

MiltonWebDAVFileServer server = new MiltonWebDAVFileServer(localFile);
server.setPort(port);
server.getUserCredentials().put("user", "pw"); // optional, defaults to no authentication
server.start();

The log with http5 looks like this:

16:07:13.875 [qtp1781459498-28] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:52832/// start
16:07:13.888 [qtp1781459498-28] WARN  io.milton.http.HandlerHelper -- authorisation declined, requesting authentication: /. resource type: io.github.atetzner.webdav.server.MiltonFolderResource
16:07:13.889 [qtp1781459498-28] INFO  i.milton.http.ResourceHandlerHelper -- authorisation failed. respond with: io.milton.http.webdav.DefaultWebDavResponseHandler resource: io.github.atetzner.webdav.server.MiltonFolderResource
16:07:13.890 [qtp1781459498-28] INFO  i.m.h.h.DefaultHttp11ResponseHandler -- respondUnauthorised: no authenticated user, so return status: HTTP/1.1 401
16:07:13.895 [qtp1781459498-28] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:52832/// finished 18ms, Status:HTTP/1.1 401, Length:null
16:07:13.920 [main] INFO  org.eclipse.jetty.server.Server -- Stopped Server@2eadc9f6{STOPPING}[10.0.11,sto=0]
16:07:13.924 [main] INFO  o.e.jetty.server.AbstractConnector -- Stopped ServerConnector@c6e0f32{HTTP/1.1, (http/1.1)}{0.0.0.0:52832}

The same test with Sardine with http4, log output:

15:54:52.394 [qtp1048128739-28] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:43636/// start
15:54:52.404 [qtp1048128739-28] WARN  io.milton.http.HandlerHelper -- authorisation declined, requesting authentication: /. resource type: io.github.atetzner.webdav.server.MiltonFolderResource
15:54:52.405 [qtp1048128739-28] INFO  i.milton.http.ResourceHandlerHelper -- authorisation failed. respond with: io.milton.http.webdav.DefaultWebDavResponseHandler resource: io.github.atetzner.webdav.server.MiltonFolderResource
15:54:52.406 [qtp1048128739-28] INFO  i.m.h.h.DefaultHttp11ResponseHandler -- respondUnauthorised: no authenticated user, so return status: HTTP/1.1 401
15:54:52.415 [qtp1048128739-28] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:43636/// finished 19ms, Status:HTTP/1.1 401, Length:null
15:54:52.440 [qtp1048128739-20] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:43636/// start
15:54:52.442 [qtp1048128739-20] INFO  i.m.h.h.a.CookieAuthenticationHandler -- Found child handler who supports this request io.milton.http.http11.auth.BasicAuthHandler@4402596e
15:54:52.443 [qtp1048128739-20] WARN  i.m.h.h.a.CookieAuthenticationHandler -- authenticate: auth.tag is not an instance of interface io.milton.principal.DiscretePrincipal, is: class java.lang.String so is not compatible with cookie authentication
15:54:52.485 [qtp1048128739-20] INFO  io.milton.http.HttpManager -- PROPFIND :: localhost:43636/// finished 44ms, Status:HTTP/1.1 207 Multi-Status, Length:781

So the http4 version apparently received a 401 Unauthorized at first, but then it healed itself so it was tried again with BasicAuth. After that we received 207 multi status and everything was fine.

Do you have an idea why our application doesn't yet work with the http5 branch?

Cheers
Noemi

nPraml · 2025-01-15T16:31:27Z

src/main/java/com/github/sardine/impl/SardineImpl.java

+		if (httpMajorVersion != null){
+			request.setVersion(new ProtocolVersion("HTTP", httpMajorVersion, 0));
+		}
+		HttpContext requestLocalContext = new HttpClientContext(context);


Suggested change

HttpContext requestLocalContext = new HttpClientContext(context);

HttpContext requestLocalContext = context;

removing the localContext would solve our auth problems as HttpClient5 seems to cache the previous authentication.
So we get one 401, then HttpClient selects the correct authenticator and from then, we get 2xx responses and our tests pass

Suggested change:

protected <T> T execute(HttpClientContext context, HttpUriRequestBase request, HttpClientResponseHandler<T> responseHandler) throws IOException { Integer httpMajorVersion = (Integer) context.getAttribute(HTTP_MAJOR_VERSION); if (httpMajorVersion != null){ request.setVersion(new ProtocolVersion("HTTP", httpMajorVersion, 0)); } try { if (responseHandler != null) { return this.client.execute(request, context, responseHandler); } else { return (T) this.client.execute(request, context); } } catch (HttpResponseException e) { // Don't abort if we get this exception, caller may want to repeat request. throw e; } catch (IOException e) { request.abort(); throw e; } }

nPraml · 2025-01-16T13:13:03Z

I implemented a unittest and a fix in PetrusHahol#1
@dkocher FYI

dkocher · 2025-01-17T07:29:17Z

I implemented a unittest and a fix in PetrusHahol#1 @dkocher FYI

That would most probably cause regressions for the following issues

dkocher marked this pull request as draft August 22, 2024 10:21

dkocher force-pushed the feature/ApacheClient5 branch from f34c128 to a3531b6 Compare January 10, 2025 13:13

PetrusHahol and others added 3 commits January 10, 2025 15:07

Update Apache HttpComponents Client dependency adding support for HTTP2.

2c632d8

Use default redirect strategy handling PROPFIND just fine.

f9fe96d

Convert custom supported date format strings.

49a37c9

dkocher force-pushed the feature/ApacheClient5 branch from a3531b6 to 49a37c9 Compare January 10, 2025 14:07

dkocher marked this pull request as ready for review January 10, 2025 14:07

dkocher changed the title ~~Feature/apache client5~~ Update Apache HttpComponents Client dependency adding support for HTTP2. Jan 10, 2025

dkocher requested a review from lookfirst January 10, 2025 14:09

dkocher added 3 commits January 10, 2025 22:25

Delete unused.

4e62e7f

Replace deprecated usage.

d081c09

Use lambda.

2e9b09a

dkocher reviewed Jan 10, 2025

View reviewed changes

dkocher added 2 commits January 10, 2025 22:40

UTF-8 is always used.

2bac00e

Remove redundant type arguments.

7dbd51d

lookfirst reviewed Jan 10, 2025

View reviewed changes

src/main/java/com/github/sardine/util/SardineUtil.java Outdated Show resolved Hide resolved

lookfirst reviewed Jan 10, 2025

View reviewed changes

src/main/java/com/github/sardine/impl/SardineImpl.java Outdated Show resolved Hide resolved

lookfirst reviewed Jan 10, 2025

View reviewed changes

src/main/java/com/github/sardine/impl/SardineImpl.java Outdated Show resolved Hide resolved

lookfirst reviewed Jan 10, 2025

View reviewed changes

src/main/java/com/github/sardine/impl/methods/HttpAcl.java Outdated Show resolved Hide resolved

dkocher reviewed Jan 13, 2025

View reviewed changes

dkocher added 5 commits January 13, 2025 10:34

Rename parameter.

8c9017c

Extract common function.

03a8bc7

Formatting.

b1b438b

Reintroduce but mark as deprecated.

47800d7

Revert lock discovery.

cdcd134

dkocher reviewed Jan 13, 2025

View reviewed changes

dkocher marked this pull request as draft January 15, 2025 15:24

nPraml reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Apache HttpComponents Client dependency adding support for HTTP2. #515

Update Apache HttpComponents Client dependency adding support for HTTP2. #515

PetrusHahol commented Aug 20, 2024

dkocher commented Aug 22, 2024

rPraml commented Sep 26, 2024

PetrusHahol commented Sep 26, 2024

dkocher commented Jan 10, 2025

lookfirst commented Jan 10, 2025

dkocher commented Jan 10, 2025

lookfirst commented Jan 10, 2025

dkocher commented Jan 10, 2025

dkocher Jan 10, 2025

lookfirst Jan 10, 2025

lookfirst Jan 10, 2025

dkocher Jan 13, 2025

lookfirst Jan 10, 2025

dkocher Jan 13, 2025

lookfirst Jan 10, 2025 •

edited

Loading

dkocher Jan 13, 2025

lookfirst Jan 10, 2025

dkocher Jan 13, 2025

dkocher Jan 13, 2025

lookfirst Jan 10, 2025

dkocher Jan 13, 2025

dkocher left a comment

dkocher left a comment

nPraml commented Jan 15, 2025

nPraml Jan 15, 2025

nPraml commented Jan 16, 2025

dkocher commented Jan 17, 2025

	HttpContext requestLocalContext = new HttpClientContext(context);
	HttpContext requestLocalContext = context;

Update Apache HttpComponents Client dependency adding support for HTTP2. #515

Are you sure you want to change the base?

Update Apache HttpComponents Client dependency adding support for HTTP2. #515

Conversation

PetrusHahol commented Aug 20, 2024

dkocher commented Aug 22, 2024

rPraml commented Sep 26, 2024

PetrusHahol commented Sep 26, 2024

dkocher commented Jan 10, 2025

Footnotes

lookfirst commented Jan 10, 2025

dkocher commented Jan 10, 2025

Footnotes

lookfirst commented Jan 10, 2025

dkocher commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lookfirst Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkocher left a comment

Choose a reason for hiding this comment

dkocher left a comment

Choose a reason for hiding this comment

nPraml commented Jan 15, 2025

Choose a reason for hiding this comment

nPraml commented Jan 16, 2025

dkocher commented Jan 17, 2025

lookfirst Jan 10, 2025 •

edited

Loading