zeroshade commented on code in PR #1324:
URL: https://github.com/apache/iceberg-go/pull/1324#discussion_r3493507234
##########
catalog/rest/rest.go:
##########
@@ -223,9 +229,11 @@ const emptyStringHash =
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
for k, v := range s.defaultHeaders {
- // Skip Content-Type if it's already set in the request
- // to avoid duplicate headers (e.g., when using PostForm)
- if http.CanonicalHeaderKey(k) == "Content-Type" &&
r.Header.Get("Content-Type") != "" {
+ // Per-request headers override session defaults. The check is
on key
+ // *presence*, not value, so suppressRequestHeaders can opt out
of a
+ // default by setting a present-but-empty entry. Keep this in
sync with
+ // suppressRequestHeaders.
+ if _, ok := r.Header[http.CanonicalHeaderKey(k)]; ok {
Review Comment:
Consumer side of the suppression trick: this has to test key *presence* (not
value) so that a present-but-`nil` entry both blocks the default and emits
nothing. That presence-not-value invariant is what I'd like to retire (see the
review summary) — with an explicit suppress set carried on the request context,
this stays a plain "explicit per-request override wins" check and the nil-entry
coupling disappears.
##########
catalog/rest/rest.go:
##########
@@ -389,6 +440,24 @@ func doPostAllowNoContent[Payload, Result any](ctx
context.Context, baseURI *url
return ret, err
}
+func setRequestHeaders(req *http.Request, headers map[string]string) {
+ for k, v := range headers {
+ req.Header.Set(k, v)
+ }
+}
+
+// suppressRequestHeaders prevents the named session-default headers from being
+// sent on this request. It writes a present-but-empty header entry rather than
+// calling Header.Del: sessionTransport.RoundTrip decides whether to apply a
+// default by testing key *presence* (not value), so a present-but-empty key
+// both blocks the default and emits nothing, whereas deleting the key would
let
+// the default reappear. Keep RoundTrip's presence check in sync with this.
+func suppressRequestHeaders(req *http.Request, headers []string) {
+ for _, h := range headers {
+ req.Header[http.CanonicalHeaderKey(h)] = nil
Review Comment:
Producer side of the same trick: writing a `nil` entry to signal "suppress"
overloads `req.Header` with two meanings. Prefer threading `suppressHeaders`
through the request context so `RoundTrip` consults an explicit set, rather
than inferring intent from a nil header value.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
