Skip to content

🐛 Fix authority header handling for HTTP/2#2522

Merged
AlexV525 merged 8 commits into
mainfrom
fix/h2-authority
May 24, 2026
Merged

🐛 Fix authority header handling for HTTP/2#2522
AlexV525 merged 8 commits into
mainfrom
fix/h2-authority

Conversation

@AlexV525
Copy link
Copy Markdown
Member

Fixes #2521

Signed-off-by: Alex Li <github@alexv525.com>
@AlexV525
Copy link
Copy Markdown
Member Author

@copilot Add authority tests in the dio_test package

Agent-Logs-Url: https://github.com/cfug/dio/sessions/7ab6cdf7-b3f4-4b5c-97f5-6ef57a306b44

Co-authored-by: AlexV525 <15884415+AlexV525@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot Add authority tests in the dio_test package

Implemented in commit 34dd6c1.

I added shared authority regression coverage in dio_test (http2AuthorityTests) and wired it into plugins/http2_adapter/test/test_suite_test.dart so the HTTP/2 adapter now validates :authority includes the non-default port via a local HTTP/2 test server. No UI changes in this PR, so there is no screenshot to provide.

@AlexV525
Copy link
Copy Markdown
Member Author

@copilot The test should not only targeting H2, but as a general test about header values

Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot The test should not only targeting H2, but as a general test about header values

Updated in bae1841.

I refactored this into a general shared header-value test under headers_tests (instead of a dedicated HTTP/2-only test hook). The new shared test checks non-default port preservation in host/authority header values across adapters, with VM conditional helpers for HTTP/1 and HTTP/2 paths. No UI changes here, so there’s no screenshot to provide.

@AlexV525
Copy link
Copy Markdown
Member Author

@copilot Can't the test check the value directly without determining the adapter? If not, convert the test from general to the H2 adapter package only (and move under its test folder)

Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot Can't the test check the value directly without determining the adapter? If not, convert the test from general to the H2 adapter package only (and move under its test folder)

Updated in f339797.

I moved this regression test to plugins/http2_adapter/test/authority_header_test.dart and removed the adapter-branching general header test from dio_test, so it now directly validates :authority for the H2 adapter only. No UI changes in this PR.

@AlexV525
Copy link
Copy Markdown
Member Author

@copilot

  1. Remove unnecessary dep adding in dio_test
  2. Use headers_test.dart as the filename and gracefully group tests for future addition.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot

  1. Remove unnecessary dep adding in dio_test
  2. Use headers_test.dart as the filename and gracefully group tests for future addition.

Addressed in 9cce64b.

I removed the unnecessary http2 dependency from dio_test/pubspec.yaml, renamed the H2 test file to plugins/http2_adapter/test/headers_test.dart, and wrapped it in a group('headers', ...) for future additions. No UI changes in this PR.

@AlexV525
Copy link
Copy Markdown
Member Author

@copilot Format the test file

Agent-Logs-Url: https://github.com/cfug/dio/sessions/f4008f24-1a56-4d9a-b040-334ce73e6082

Co-authored-by: AlexV525 <15884415+AlexV525@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot Format the test file

Done in f372ff7 — I formatted plugins/http2_adapter/test/headers_test.dart.

@AlexV525
Copy link
Copy Markdown
Member Author

@copilot Add changelog entry

Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

@copilot Add changelog entry

Done in b591053 — added the changelog entry under plugins/http2_adapter/CHANGELOG.md in ## Unreleased.

@AlexV525 AlexV525 marked this pull request as ready for review May 23, 2026 10:26
@AlexV525 AlexV525 requested a review from a team as a code owner May 23, 2026 10:26
@github-actions
Copy link
Copy Markdown
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
plugins/http2_adapter/lib/src/connection_manager_imp.dart 🟢 82.14% 🟢 84.29% 🟢 2.15%
plugins/http2_adapter/lib/src/http2_adapter.dart 🟠 74.31% 🟢 75.69% 🟢 1.38%
Overall Coverage 🟢 88.61% 🟢 88.88% 🟢 0.27%

Minimum allowed coverage is 0%, this run produced 88.88%

@AlexV525 AlexV525 merged commit 589c099 into main May 24, 2026
5 checks passed
@AlexV525 AlexV525 deleted the fix/h2-authority branch May 24, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[dio_http2_adapter] Http2Adapter omits non-default port from :authority

3 participants