ARROW-10428: [FlightRPC][Java] Add support for HTTP cookies by jduo · Pull Request #8554 · apache/arrow

jduo · 2020-10-30T03:33:20Z

Add a ClientMiddleware that can read HTTP Set-Cookie and Set-Cookie2 headers
from server responses and transmit corresponding Cookie headers

github-actions · 2020-10-30T03:48:04Z

https://issues.apache.org/jira/browse/ARROW-10428

lidavidm

Looks straightforward. Just a comment about the implementation.

lidavidm · 2020-10-30T13:57:43Z

.../flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java

Note that the middleware is instantiated per call, so to have persistent cookie values across multiple calls, you actually want this map to be part of the Factory.

Also, FlightClient can handle concurrent calls, so this should be a ConcurrentHashMap.

Good catch. I'll add an end-to-end test for persistence too.

lidavidm · 2020-10-30T13:58:04Z

.../flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java

Do we actually expect to see this header, as it's deprecated?

I don't really. This isn't supported by any major browsers now: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie2 .

It's there for completeness, but given it's buggy in the JDK, let's remove Set-cookie2 handling.

Removed Set-Cookie2 support.

tifflhl · 2020-10-30T19:22:48Z

.../flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java

Calculate seems to imply that we are generating new cookies here but we are just joining existing cookie strings?

Add a ClientMiddleware that can read HTTP Set-Cookie and Set-Cookie2 headers from server responses and transmit corresponding Cookie headers

- Remove COOKIE2_HEADER - Store cookies in the concurrent hashmap in the factory for persistance. - Added end to end test for cookie persistence

- Fixed test cookieStaysAfterMultipleRequestsEndToEnd. - Cleanup TestCookieHandling resources.

- Update TestCookieHandling to use the test Factory with the ability to intercept the returned ClientCookieMiddleware.

Address code review comments

lidavidm · 2020-11-05T14:07:49Z

Just to link things: #8572 is implementing support for arbitrary headers, perhaps we should think about if cookies should build on top of that support.

jduo · 2020-11-05T16:56:49Z

Just to link things: #8572 is implementing support for arbitrary headers, perhaps we should think about if cookies should build on top of that support.

I'm leaning towards keeping this as just middleware and not involving CallOptions. #8572 is for letting the client arbitrarily add headers at the time of a request, whereas cookie-headers aren't arbitrary and should be automatically sent. The checking of expiration should also be automatic.

We could rework this patch to have a subclass of HeaderCallOption that lazily retrieves the cookie header from the cookie middleware during wrapStub, but I feel this is more difficult/error-prone for app-developers.

lidavidm · 2020-11-06T01:31:27Z

I'm leaning towards keeping this as just middleware and not involving CallOptions.

Sounds good to me.

lidavidm · 2020-11-06T01:32:46Z

.../flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java

+    // The server can also update a cookie to have an Expiry in the past or negative age
+    // to signal that the client should stop using the cookie immediately.
+    final Consumer<String> handleSetCookieHeader = (headerValue) -> {
+      final List<HttpCookie> parsedCookies = HttpCookie.parse(headerValue);


Should we handle IllegalArgumentException here from HttpCookie.parse?

Also instead of a lambda it may be clearer to define Factory#updateFromSetCookieHeader or something like that.

lidavidm · 2020-11-06T01:36:35Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/client/TestCookieHandling.java

+    cookieMiddleware.onHeadersReceived(headersToSend);
+
+    // Verify that the k cookie was discarded because the server told the client it is expired.
+    Assert.assertTrue(cookieMiddleware.getValidCookiesAsString().isEmpty());


It seems this test doesn't always succeed.

So I looked into this and found that on my Mac, the JDK explicitly checks if Max-Age == -1 (MAX_AGE_UNSPECIFIED), treat the max age as unset.
In the OpenJDK 11 source code though, they treat any negative number as max age unset:
https://github.com/openjdk/jdk11u-dev/blob/75a42602826c32a1053b10188ec3826eb9c0c898/src/java.base/share/classes/java/net/HttpCookie.java#L236

The RFC states that 0, and any value less than zero is supposed to be treated as immediately expired.

I've changed this test to use Max-Age==0 and added a comment.

This bug is still in OpenJDK 15: https://github.com/openjdk/jdk15u/blob/a96a5c59ac49ffb063b093a2674ede2deed87b13/src/java.base/share/classes/java/net/HttpCookie.java#L242

I added a note about this in ClientCookieMiddleware's class description.

Ah! That's fun. Do you think it's worth filing a JDK bug? Though I'm surprised it's been around so long.

lidavidm · 2020-11-06T01:37:36Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/client/TestCookieHandling.java

+    Assert.assertEquals("k=\"v\"", cookieMiddleware.getValidCookiesAsString());
+
+    try {
+      Thread.sleep(5000);


This is a little icky, though it appears there's no way to mock time for HttpCookie.

I'll mark this test as @ignore, since this is essentially just testing that we call isExpired correctly, which we do in the max-age -2 test.

lidavidm · 2020-11-06T23:14:16Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/client/TestCookieHandling.java

+    cookieMiddleware.onHeadersReceived(headersToSend);
+
+    // Verify that the k cookie was discarded because the server told the client it is expired.
+    Assert.assertTrue(cookieMiddleware.getValidCookiesAsString().isEmpty());


Ah! That's fun. Do you think it's worth filing a JDK bug? Though I'm surprised it's been around so long.

lidavidm · 2020-11-06T23:14:29Z

Will merge on green.

Add a ClientMiddleware that can read HTTP Set-Cookie and Set-Cookie2 headers from server responses and transmit corresponding Cookie headers Closes apache#8554 from jduo/cookie-handling Lead-authored-by: James Duong <[email protected]> Co-authored-by: Keerat Singh <[email protected]> Co-authored-by: Keerat Singh <[email protected]> Signed-off-by: David Li <[email protected]>

lidavidm reviewed Oct 30, 2020

View reviewed changes

tifflhl reviewed Oct 30, 2020

View reviewed changes

ARROW-10428: [FlightRPC][Java] Add support for HTTP cookies

227e1af

Add a ClientMiddleware that can read HTTP Set-Cookie and Set-Cookie2 headers from server responses and transmit corresponding Cookie headers

jduo force-pushed the cookie-handling branch from f598170 to 227e1af Compare November 2, 2020 17:07

Keerat Singh and others added 4 commits November 3, 2020 16:02

Address code review comments

8aa1ff7

- Remove COOKIE2_HEADER - Store cookies in the concurrent hashmap in the factory for persistance. - Added end to end test for cookie persistence

Fixed test cookieStaysAfterMultipleRequestsEndToEnd

520a5e4

- Fixed test cookieStaysAfterMultipleRequestsEndToEnd. - Cleanup TestCookieHandling resources.

Address code review feedback

1108830

- Update TestCookieHandling to use the test Factory with the ability to intercept the returned ClientCookieMiddleware.

Merge pull request #12 from jduo/cookie-handling-review-comments

1457e9a

Address code review comments

lidavidm reviewed Nov 6, 2020

View reviewed changes

Address PR comments

b13c24d

jduo force-pushed the cookie-handling branch from 79493e4 to c8614a0 Compare November 6, 2020 21:50

Explicitly remove cached cookies when max-age is negative

485ee13

jduo force-pushed the cookie-handling branch from 432a22c to 485ee13 Compare November 6, 2020 22:56

Add user-facing javadoc about the negative max-age issues

5a298ff

lidavidm approved these changes Nov 6, 2020

View reviewed changes

lidavidm closed this in 47f2e0c Nov 6, 2020

asfimport mentioned this pull request Nov 6, 2020

[FlightRPC][Java] Add support for HTTP cookies #26407

Closed

Conversation

jduo commented Oct 30, 2020

Uh oh!

github-actions bot commented Oct 30, 2020

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Nov 5, 2020

Uh oh!

jduo commented Nov 5, 2020

Uh oh!

lidavidm commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants