format: Allow credentials to be supplied via a callback

[CurtHagenlocher]

What feature or improvement would you like to see?

Credentials are increasingly of limited duration, which introduces the possibility of them expiring in the middle of an operation. While one approach is to give the driver enough information so that it can refresh the credential on its own, there are systems where such delegation is difficult or impossible. For these cases, it would be nice for the driver to call back into the host to ask for an updated credential.

One possible approach:

/// \brief A credential handler function.
///
/// The handler must not call any ADBC functions other than the
/// ConnectionSetOption functions and releasing the warning.
///
/// There are no requirements on ordering or concurrency of calls to the
/// handler; the driver may call the handler at any time from any thread,
/// including calling the handler concurrently.
///
/// \param[in] details Additional details about the credential required.
///   The application is responsible for releasing the details, but the
///   pointer itself will not be valid after the handler returns.
/// \param[in] user_data The user_data pointer.
typedef void (*AdbcCredentialHandler)(const struct AdbcError* details, void* user_data);


  AdbcStatusCode (*ConnectionSetCredentialHandler)(struct AdbcConnection*,
                                                   AdbcCredentialHandler handler,
                                                   void* user_data, struct AdbcError*);```

[zeroshade]

While I can agree with the sentiment, I'm not particularly fond of this particular approach. We shouldn't re-use the AdbcError struct for the details like that and this isn't particularly generic and would require a user to have a separate one of these per-driver they utilize. The better solution (IMHO) is for this to be handled by the driver if necessary and should just require enough information at the initial connection state to be able to refresh (or vend) credentials. Such as passing a private JWT/PEM file, or OAuth2 secret/principal/etc.

/// The handler must not call any ADBC functions other than the
/// ConnectionSetOption functions and releasing the warning.

The handler doesn't have the connection itself, so how would it call ConnectionSetOption? Additionally, most drivers only process credential options before establishing the connection and wouldn't re-process credentials upon calling ConnectionSetOption.

Isn't it sufficient to just update the credentials and open a new connection in that scenario? (which is what the driver would have to do internally anyways).

[CurtHagenlocher]

The handler doesn't have the connection itself, so how would it call ConnectionSetOption?

I originally had the connection in the signature and wavered between that and assuming the caller would stash it in user_data.

Isn't it sufficient to just update the credentials and open a new connection in that scenario? (which is what the driver would have to do internally anyways).

Consider the case where the query has finished running and the driver is now fetching the results from cloud storage. If the results are large enough, simply reading them could take longer than the lifetime of the previous token. You'd want to keep fetching without opening a new connection.

most drivers only process credential options before establishing the connection and wouldn't re-process credentials upon calling

It's an option. Presumably most drivers wouldn't support (or need to support) this.

We shouldn't re-use the AdbcError struct for the details like that

"Reduce, reuse, recycle" :P. But yeah, that was sheer expedience and I'm not generally opposed to declaring a new container structure for specifically this purpose.

[CurtHagenlocher]

what the driver would have to do internally anyways

In our increasingly HTTP-centric world, the driver could keep using the same HTTP connection and simply put the new credential in the header of the next request. This is presumably what it would do if it were refreshing credentials on its own.

The better solution (IMHO) is for this to be handled by the driver if necessary

I agree that this would be ideal but have run into situations where it's not possible. For instance, minting the token might require access to a certificate behind a custom service and having the driver know about the service isn't practical.

[CurtHagenlocher]

Anyway, I'll rework the details.

[lidavidm]

I think the ask is reasonable here. Though whether provided directly or indirectly, I'm not too big a fan of the recursive SetOption calls, but perhaps that's unavoidable if we want to be flexible (I assume that's the justification for not using a return value or out parameter).

[CurtHagenlocher]

I started to rework the details and it reminded me of some of the reasons I'd picked AdbcError. Semantically, it's like returning a 401 from an HTTP request: "this request was an error, but if you retry with new credentials it might work". It also happens that the AdbcErrorDetail API is a cheap and flexible way to pass arbitrary key-value pairs, and it would be annoying to create another instance of basically the same thing.

As far as updating the credential property or properties, I agree that calling SetOption on the connection might require extra complexity in the driver to deal with reentrancy. But nearly any other approach is going to have a heavier API footprint; at a minimum, I'm going to need to be able to return at least one value which means that someone will have to free that buffer afterwards. The simplest way to avoid this is to reuse an existing structure which already accommodates cleanup. Here too, AdbcError is a possibility -- but one that's even more dubious than the previous use I suggested. Another alternative would be to treat this like a Bind and have the host populate an ArrowArray and ArrowSchema with the details.

(Side note: I'm curious now why AdbcErrorDetail wasn't implemented in terms of ArrowArray and ArrowSchema. This would be more generic because it would incorporate explicitly-typed data whereas -- AFAICT -- AdbcErrorDetail types have to be inferred by the host.)

Anyhow, I iterated a bit on this idea and came up with

typedef AdbcStatusCode (*AdbcCredentialHandler)(struct AdbcError* details,
                                      struct ArrowSchema* schema, struct ArrowArray* data,
                                      void* user_data);

AdbcStatusCode (*ConnectionSetCredentialHandler)(struct AdbcConnection*,
                                                 AdbcCredentialHandler handler,
                                                 void* user_data, struct AdbcError*);

I don't know how we feel about in/out arguments, but the conceptual idea is this:
The driver gets an error from the data source that it identifies as a possible credential expiration error. It populates the error details as an AdbcError and then puts the relevant credential-related parameters into a single-row record batch. It passes these to the callback. One of three things happens:

1. The callback looks at the error and other properties and decides whether or not it can refresh the credential. If it can't, then it returns ADBC_STATUS_CANCELLED to show that it did not attempt a credential refresh. It doesn't touch the other parameters. The driver releases all the data structures when control returns to it.
2. The callback tries to refresh the credential but the refresh fails. The callback returns ADBC_STATUS_UNAUTHENTICATED to show this. It may optionally release the error details and populate it with new information about the credential refresh failure. Again, the driver releases all the data structures when control returns to it.
3. The refresh succeeds. The callback releases data and populates the new credential values in that array. It returns ADBC_STATUS_OK. The driver reads the new credential properties from the data and then releases all the data structures.

[lidavidm]

(Side note: I'm curious now why AdbcErrorDetail wasn't implemented in terms of ArrowArray and ArrowSchema. This would be more generic because it would incorporate explicitly-typed data whereas -- AFAICT -- AdbcErrorDetail types have to be inferred by the host.)

I think I was mostly thinking of gRPC/Googleapi's StatusDetail.

I think the proposal here is reasonable, using an ArrowArray feels like overkill, but well there's no C Data version of Arrow scalars so this is the best we can do.

[CurtHagenlocher]

Does this seem okay enough to others that I might feel comfortable prematurely adding it to the C# implementation as something like

    /// <summary>
    /// Handler to be called by the driver when it needs an updated credential.
    /// </summary>
    /// <param name="exception">A description of the error driving the request</param>
    /// <param name="recordBatch">Current credential parameter values</param>
    /// <returns>Updated credential parameter values. Must have the same Schema as the input recordBatch.</returns>
    public delegate RecordBatch CredentialHandler(AdbcException exception, RecordBatch recordBatch);

    public partial class AdbcConnection {
            /// <summary>
            /// Gets or sets the credential handler for this connection.
            /// </summary>
            public CredentialHandler CredentialHandler { get; set; }
    }

[davidhcoe]

We did something similar in the BigQuery driver a while back. At the time we were very C# focused and returned a Task to execute from UpdateToken and focused on setting the adbc.bigquery.access_token option but I like the idea of having the definition in the spec for all drivers to participate.