feat(java/driver/flight-sql): add basic auth #572
Conversation
|
Hey - it's gonna be a few weeks before I can review just fyi, C/Go use just "username" and "password" so best to stick to that if possible |
|
Okay, I'll push ahead with dremio suite then and you can review it at the end |
See some recent Java commits/issues that change up how allocators are handled (the current implementation had leaks): e69723a, #563, #564
Only URIs for now (C/Go have settled on "uri" as the standard parameter, Java needs to align with that too)
I will have to dig later but possibly/probably upstream needs to be fixed first; I don't think any of that was implemented to really do anything other than support the JDBC driver specifically
It seems you could factor out a factory function that can be used to create a one-off main client as well as cached clients? |
| @@ -48,6 +50,8 @@ public AdbcDatabase initDatabase(BufferAllocator allocator) throws AdbcException | |||
|
|
|||
| final Map<String, Object> parameters = new HashMap<>(); | |||
| parameters.put(AdbcDriver.PARAM_URL, url); | |||
| parameters.put("adbc.username", System.getenv(FLIGHT_SQL_USER_ENV_VAR)); | |||
There was a problem hiding this comment.
As mentioned, let's use just "username" for consistency with other implementations
| FlightClient flightClient = | ||
| FlightClient.builder(allocator, loc) | ||
| .intercept( | ||
| new FlightClientMiddleware.Factory() { |
There was a problem hiding this comment.
nit: could we pull this out into a static inner class instead of having it inline here?
There was a problem hiding this comment.
That would make Factory a doubly nested inner class, probably best if I split it into an entirely separate file.
There was a problem hiding this comment.
That would be fine IMO. Once the code starts creeping too much to the right it gets a little hard to read.
| (String) parameters.get("adbc.username"), | ||
| (String) parameters.get("adbc.password")); |
There was a problem hiding this comment.
It might be better to pass the parameters instead. The FlightSqlDatabase will need to know about all the params to decide how to build the FlightClient. For example, all the TLS options.
It would also be nice to have dataclasses or static objects with all the option values https://arrow.apache.org/adbc/main/driver/go/flight_sql.html#client-options
|
@tokoko , this issue hasn't been updated since May. May I help finish getting this one merged? |
|
FYI the JDBC Flight SQL Driver implements optional sharing of the bearer token on spawned connections. It does so when building the driver -- it reads the token from the middle ware then on subsequent FlightClients appends it as a header parameter to the request. |
|
Completed in #1487 |
@lidavidm I want a quick feedback on some of the changes.
FlightSqlDatabaseFlightSqlDatabasetoFlightSqlConnection. In the existing version after connect() is called a new client object is created, but all of them shared the same allocator. That was probably a bug, right?adbc.usernameandadbc.password(to be added inAdbcDriver). Are there any plans to have some sort of connection string syntax like JDBC?authenticateBasicTokenbecause I couldn't find a way to extract bearer value from CredenialCallOption. Maybe there's a simpler way to accomplish this???clientCache. Right now the main FlightClient is created independently and only Endpoint clients are obtained from cache. If changed, main client will always have to be obtained from cache before usage, otherwise it might have expired.