Skip to content

fix(triple): canonicalize context metadata headers#3428

Merged
Alanxtl merged 5 commits into
apache:developfrom
leno23:codex/triple-metadata-example-2422
Jun 16, 2026
Merged

fix(triple): canonicalize context metadata headers#3428
Alanxtl merged 5 commits into
apache:developfrom
leno23:codex/triple-metadata-example-2422

Conversation

@leno23

@leno23 leno23 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

What

Canonicalize Triple context metadata header keys when storing incoming and outgoing headers. This keeps NewOutgoingContext and AppendToOutgoingContext values under the same key and makes standard http.Header.Get/Values access work as documented.

Also document how unary clients read response headers and trailers from the response wrapper.

Refs #2422

Validation

  • go test ./protocol/triple/triple_protocol
  • go test ./protocol/triple
  • git diff --check

Signed-off-by: wuyangfan <yangfan.wu@succaiss.com>
@leno23 leno23 force-pushed the codex/triple-metadata-example-2422 branch from 96914fa to 901942d Compare June 15, 2026 04:55
@codecov-commenter

codecov-commenter commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.55%. Comparing base (60d1c2a) to head (7fd64c2).
⚠️ Report is 845 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3428      +/-   ##
===========================================
+ Coverage    46.76%   53.55%   +6.78%     
===========================================
  Files          295      493     +198     
  Lines        17172    38386   +21214     
===========================================
+ Hits          8031    20557   +12526     
- Misses        8287    16176    +7889     
- Partials       854     1653     +799     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR canonicalizes Triple context metadata header keys when storing incoming/outgoing headers so that headers added via NewOutgoingContext and AppendToOutgoingContext unify under the same key and can be reliably accessed via standard http.Header methods (e.g., Get/Values). It also clarifies how unary clients can read response headers and trailers from the response wrapper.

Changes:

  • Canonicalize context metadata keys using http.CanonicalHeaderKey in outgoing/incoming context helpers.
  • Make IsReservedHeader case-insensitive by lowercasing input before classification, with a focused unit test.
  • Add documentation/examples for response header/trailer access and outgoing context usage.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
protocol/triple/triple_protocol/triple.go Documents unary client usage for reading response headers/trailers via the response wrapper.
protocol/triple/triple_protocol/protocol_grpc.go Lowercases inputs in IsReservedHeader to ensure reserved-header detection is case-insensitive.
protocol/triple/triple_protocol/protocol_grpc_test.go Adds a test ensuring IsReservedHeader canonicalizes (lowercases) input.
protocol/triple/triple_protocol/header.go Canonicalizes stored context header keys (incoming/outgoing) and aligns append behavior.
protocol/triple/triple_protocol/header_test.go Adds an example verifying outgoing context + append results are accessible via Get/Values.
protocol/invocation/rpcinvocation.go Lowercases extracted header keys before storing as invocation attachments for consistency.
protocol/invocation/rpcinvocation_test.go Updates assertions to use http.Header.Values instead of direct map indexing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread protocol/triple/triple_protocol/header.go
@Alanxtl Alanxtl added ☢️ Bug 3.3.2 version 3.3.2 labels Jun 16, 2026
@Alanxtl

Alanxtl commented Jun 16, 2026

Copy link
Copy Markdown
Member

This looks like a reasonable compromise to me. Since these APIs expose http.Header, storing context headers in canonical form makes standard Header.Get/Values work as users would expect. At the same time, converting keys back to lowercase when merging into Invocation attachments keeps the Dubbo/gRPC metadata semantics aligned with grpc-go, dubbo-java, and HTTP/2 wire behavior.

Could we add a short comment around the CanonicalHeaderKey / ToLower boundary to make this intentional layering explicit? Something like: context headers use canonical http.Header keys for Go API compatibility, while invocation attachments remain lowercase metadata keys for Dubbo/gRPC compatibility.

@leno23

leno23 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Added short comments around the canonical context-header storage and lowercase exported-attachment boundary in 7fd64c2. Local validation passes: go test ./protocol/triple/triple_protocol, git diff --check, and golangci-lint run ./protocol/triple/triple_protocol --timeout=10m.

@sonarqubecloud

Copy link
Copy Markdown

@Alanxtl Alanxtl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Alanxtl Alanxtl merged commit e741e0e into apache:develop Jun 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.3.2 version 3.3.2 ☢️ Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Triple header/trailer usage pattern proposal

4 participants