ARROW-10526: [FlightRPC][C++] Client cookie middleware#8725
ARROW-10526: [FlightRPC][C++] Client cookie middleware#8725lyndonbauto wants to merge 110 commits intoapache:masterfrom
Conversation
lyndonbauto
commented
Nov 20, 2020
lyndonbauto
commented
- Added client cookie middleware which caches and monitors expiry
- Added testing for client cookie middleware
ParseTimestampStrptime returns time units since the epoch, not time units since now.
[2] Fixed a few bugs with parsing and logic of cookie handling
lyndonbauto
changed the title
|
There appears to be a failure in the python build, I think it's caused by something unrelated to this pull request though, I noticed the same here: https://github.com/apache/arrow/runs/1456411775?check_suite_focus=true |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
My main comment here is about the tests - I'd like to see if we can refactor things so that the tests can directly test some of the cookie logic, and so that they don't duplicate said logic in the tests themselves.
Another approach might be to have glass-box tests by having an internal header with the middleware definition, so that tests can directly check the contents of the cookie cache (but so that client code still just gets an opaque middleware instance).
| // This test has functions that allow adding and removing cookies from a list of cookies. | ||
| // It also automatically adds cookies to an internal list of tracked cookies when they | ||
| // are passed to the middleware. | ||
| class TestCookieMiddleware : public ::testing::Test { |
There was a problem hiding this comment.
This is a general comment, but there's a lot of logic here, and some of it duplicates the logic in the middleware class itself. Can we factor out the cookie parsing into an internal header and add direct tests of that logic, and perhaps reuse some of it here (e.g. splitting and parsing cookies) instead of duplicating it? That would also let us directly test the cookie parsing, instead of indirectly through these tests.
There was a problem hiding this comment.
Is there any need to discard here if we have to discard before we send cookies anyways?
There was a problem hiding this comment.
Can this be moved to an actual constant following the naming convention (kCookieExpiresFormat or similar)?
[2] Added tests for all parsing functions [3] Changes existing tests to use cookie functions directly instead of indirectly [4] Removed duplicated logic
… weren't exported.
|
@lidavidm That's a really good point, there is a lot of code that wasn't facing direct testing. I followed suggestion of moving the parsing logic to an internal header (the client_header_internal files seemed appropriate since this code is all related to headers). I added a bunch of tests for all the different internal parsing functions that were added. |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Thanks for the changes! I left a few nits, but this is good to go once fixed.
|
|
||
| #include "arrow/flight/client_middleware.h" | ||
| #include "arrow/result.h" | ||
| #include "arrow/util/optional.h" |
There was a problem hiding this comment.
nit: I don't think value_parsing is used in the header, and we're missing #include <mutex>, and #include <functional> for binary_function, #include <string>, and #include <chrono>
There was a problem hiding this comment.
Oh and #include <map> but perhaps unordered_map is better since you don't care about ordering?
| } | ||
|
|
||
| // Function to take a list of cookies and split them into a vector of individual | ||
| // cookies. This is done because the cookie cache is a map so ordering is not |
There was a problem hiding this comment.
Can we use unordered_map then?
There was a problem hiding this comment.
Yes, good call. Will change this.
3 participants
