Copilot commented on code in PR #3443:
URL: https://github.com/apache/dubbo-go/pull/3443#discussion_r3435488737
##########
client/options.go:
##########
@@ -1022,3 +1025,25 @@ func WithCallRetries(retries int) CallOption {
opts.Retries = strconv.Itoa(retries)
}
}
+
+// WithResponseHeader configures a target to receive response headers when the
+// protocol supports it.
+//
+// For Triple unary calls, the header is captured from both successful
responses
+// and error responses when response metadata is available.
Review Comment:
The docstring says this option works whenever the protocol supports response
headers, but the implementation currently only propagates/copies metadata for
Triple unary calls. Tightening the wording avoids misleading users (e.g., for
streaming calls where this option is currently ignored).
##########
protocol/triple/triple_invoker.go:
##########
@@ -158,6 +160,20 @@ func (ti *TripleInvoker) Invoke(ctx context.Context,
invocation base.Invocation)
return &result
}
+func responseMetadataTargets(invocation base.Invocation) (*http.Header,
*http.Header) {
+ var responseHeader *http.Header
+ if headerRaw, ok :=
invocation.GetAttribute(constant.ResponseHeaderKey); ok {
+ responseHeader, _ = headerRaw.(*http.Header)
+ }
+
+ var responseTrailer *http.Header
+ if trailerRaw, ok :=
invocation.GetAttribute(constant.ResponseTrailerKey); ok {
+ responseTrailer, _ = trailerRaw.(*http.Header)
+ }
+
+ return responseHeader, responseTrailer
+}
Review Comment:
responseMetadataTargets silently drops the attribute when the stored value
isn’t a *http.Header (type assertion result is ignored). This can make misuses
hard to diagnose. Consider validating the type and logging a warning when it’s
unexpected.
##########
client/options.go:
##########
@@ -1022,3 +1025,25 @@ func WithCallRetries(retries int) CallOption {
opts.Retries = strconv.Itoa(retries)
}
}
+
+// WithResponseHeader configures a target to receive response headers when the
+// protocol supports it.
+//
+// For Triple unary calls, the header is captured from both successful
responses
+// and error responses when response metadata is available.
+func WithResponseHeader(header *http.Header) CallOption {
+ return func(opts *CallOptions) {
+ opts.ResponseHeader = header
+ }
+}
+
+// WithResponseTrailer configures a target to receive response trailers when
the
+// protocol supports it.
+//
+// For Triple unary calls, the trailer is captured from both successful
responses
+// and error responses when response metadata is available.
Review Comment:
Same as WithResponseHeader: the current wording implies broad protocol
support, but only Triple unary calls populate this option today. Clarifying the
comment prevents confusion for callers who pass this option to other call
types/protocols.
##########
protocol/triple/client_test.go:
##########
@@ -20,13 +20,16 @@ package triple
import (
"context"
"net/http"
+ "net/http/httptest"
"testing"
"time"
)
import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
+ "google.golang.org/protobuf/types/known/emptypb"
Review Comment:
There’s an extra blank line inside the import block, which will be removed
by gofmt and can cause unnecessary diffs/formatting churn.
--
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]
