lidavidm commented on code in PR #387:
URL: https://github.com/apache/arrow-adbc/pull/387#discussion_r1087071562
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -324,7 +345,19 @@ func doGet(ctx context.Context, cl *flightsql.Client,
endpoint *flight.FlightEnd
}
func (c *cnxn) SetOption(key, value string) error {
+ if strings.HasPrefix(key, OptionRPCCallHeaderPrefix) {
+ name := strings.TrimPrefix(key, OptionRPCCallHeaderPrefix)
+ if value == "" {
+ c.hdrs.Delete(name)
+ } else {
+ c.hdrs.Append(name, value)
+ }
+ return nil
+ }
+
switch key {
+ case OptionAuthorizationHeader:
Review Comment:
Do we want to accept this at the connection level?
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -635,6 +673,9 @@ func (c *cnxn) NewStatement() (adbc.Statement, error) {
alloc: c.db.alloc,
cl: c.cl,
clientCache: c.clientCache,
+ // don't copy the headers so that calling SetOption to add more
+ // headers on the connection still propagates
Review Comment:
But conversely, that means all the statements are tied together, which I
think isn't desirable - consider multiple independent statements and each of
them tries to set an OpenTelemetry span, for instance.
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -204,19 +227,16 @@ func (d *database) SetOptions(cnOptions
map[string]string) error {
}
type bearerAuthMiddleware struct {
- token string
+ hdrs metadata.MD
}
func (b *bearerAuthMiddleware) StartCall(ctx context.Context) context.Context {
- if b.token != "" {
- return metadata.AppendToOutgoingContext(ctx, "authorization",
b.token)
- }
-
- return ctx
+ md, _ := metadata.FromOutgoingContext(ctx)
+ return metadata.NewOutgoingContext(ctx, metadata.Join(md, b.hdrs))
}
func getFlightClient(ctx context.Context, loc string, d *database)
(*flightsql.Client, error) {
- authMiddle := &bearerAuthMiddleware{}
+ authMiddle := &bearerAuthMiddleware{hdrs: d.hdrs.Copy()}
Review Comment:
(Well, we also don't want changes in the children to propagate to the
parent.)
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -204,19 +227,16 @@ func (d *database) SetOptions(cnOptions
map[string]string) error {
}
type bearerAuthMiddleware struct {
- token string
+ hdrs metadata.MD
}
func (b *bearerAuthMiddleware) StartCall(ctx context.Context) context.Context {
- if b.token != "" {
- return metadata.AppendToOutgoingContext(ctx, "authorization",
b.token)
- }
-
- return ctx
+ md, _ := metadata.FromOutgoingContext(ctx)
+ return metadata.NewOutgoingContext(ctx, metadata.Join(md, b.hdrs))
}
func getFlightClient(ctx context.Context, loc string, d *database)
(*flightsql.Client, error) {
- authMiddle := &bearerAuthMiddleware{}
+ authMiddle := &bearerAuthMiddleware{hdrs: d.hdrs.Copy()}
Review Comment:
Copying means that setting a header after construction won't propagate to
children...maybe that's desirable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
