ARROW-10428: [FlightRPC][Java] Add support for HTTP cookies #8554
Conversation
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Looks straightforward. Just a comment about the implementation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch. I'll add an end-to-end test for persistence too.
There was a problem hiding this comment.
Do we actually expect to see this header, as it's deprecated?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed Set-Cookie2 support.
There was a problem hiding this comment.
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
|
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. |
Sounds good to me. |
| // 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); |
There was a problem hiding this comment.
Should we handle IllegalArgumentException here from HttpCookie.parse?
There was a problem hiding this comment.
Also instead of a lambda it may be clearer to define Factory#updateFromSetCookieHeader or something like that.
| cookieMiddleware.onHeadersReceived(headersToSend); | ||
|
|
||
| // Verify that the k cookie was discarded because the server told the client it is expired. | ||
| Assert.assertTrue(cookieMiddleware.getValidCookiesAsString().isEmpty()); |
There was a problem hiding this comment.
It seems this test doesn't always succeed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I added a note about this in ClientCookieMiddleware's class description.
There was a problem hiding this comment.
Ah! That's fun. Do you think it's worth filing a JDK bug? Though I'm surprised it's been around so long.
| Assert.assertEquals("k=\"v\"", cookieMiddleware.getValidCookiesAsString()); | ||
|
|
||
| try { | ||
| Thread.sleep(5000); |
There was a problem hiding this comment.
This is a little icky, though it appears there's no way to mock time for HttpCookie.
There was a problem hiding this comment.
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.
| cookieMiddleware.onHeadersReceived(headersToSend); | ||
|
|
||
| // Verify that the k cookie was discarded because the server told the client it is expired. | ||
| Assert.assertTrue(cookieMiddleware.getValidCookiesAsString().isEmpty()); |
There was a problem hiding this comment.
Ah! That's fun. Do you think it's worth filing a JDK bug? Though I'm surprised it's been around so long.
|
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]>
4 participants
Add a ClientMiddleware that can read HTTP Set-Cookie and Set-Cookie2 headers
from server responses and transmit corresponding Cookie headers