Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add server DisableHeaderNamesNormalizing option #839

Closed

Conversation

li-jin-gou
Copy link
Member

@li-jin-gou li-jin-gou commented Jul 3, 2023

What type of PR is this?

feat

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>.
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo.

(Optional) Translate the PR title into Chinese.

  • 添加 DisableHeaderNamesNormalizing 选项

(Optional) Which issue(s) this PR fixes:

(Optional) The PR that updates user documentation:

@li-jin-gou li-jin-gou requested review from a team as code owners July 3, 2023 07:02
@li-jin-gou li-jin-gou changed the title feat: add server DisableHeaderNamesNormalizing option 【WIP】feat: add server DisableHeaderNamesNormalizing option Jul 3, 2023
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage: 65.21% and project coverage change: -0.05% ⚠️

Comparison is base (a3770c3) 80.30% compared to head (f18543a) 80.25%.
Report is 2 commits behind head on develop.

❗ Current head f18543a differs from pull request most recent head d3bccb1. Consider uploading reports for the commit d3bccb1 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #839      +/-   ##
===========================================
- Coverage    80.30%   80.25%   -0.05%     
===========================================
  Files           97       97              
  Lines         9602     9620      +18     
===========================================
+ Hits          7711     7721      +10     
- Misses        1408     1413       +5     
- Partials       483      486       +3     
Files Changed Coverage Δ
pkg/protocol/http1/server.go 74.27% <ø> (ø)
pkg/route/engine.go 59.79% <0.00%> (-0.76%) ⬇️
pkg/protocol/http1/ext/common.go 70.32% <66.66%> (+0.19%) ⬆️
pkg/app/server/option.go 96.00% <100.00%> (+0.22%) ⬆️
pkg/common/config/option.go 100.00% <100.00%> (ø)
pkg/common/utils/chunk.go 93.93% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@li-jin-gou li-jin-gou force-pushed the feat/add_disable_normalizing branch 2 times, most recently from 4835aac to 6dc350f Compare July 11, 2023 12:42
@li-jin-gou li-jin-gou force-pushed the feat/add_disable_normalizing branch from 6dc350f to e26d309 Compare July 11, 2023 13:30
@li-jin-gou li-jin-gou changed the title 【WIP】feat: add server DisableHeaderNamesNormalizing option feat: add server DisableHeaderNamesNormalizing option Jul 12, 2023
@@ -179,6 +180,12 @@ func (s Server) Serve(c context.Context, conn network.Conn) (err error) {
internalStats.Record(ti, stats.ReadHeaderFinish, err)
})
}

if s.DisableHeaderNamesNormalizing {
ctx.Request.Header.DisableNormalizing()
Copy link
Member

Choose a reason for hiding this comment

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

可以统一放在ctx从池子初始化的地方做,这个应该是一个”只需做一次的配置“,放在这里会影响每一个请求。
另外这个配置需要区分req/resp吗?

Copy link
Member Author

Choose a reason for hiding this comment

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

我确定下

Copy link
Member Author

@li-jin-gou li-jin-gou Aug 31, 2023

Choose a reason for hiding this comment

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

done,另外区分了 request 和 response 配置

Copy link
Member

Choose a reason for hiding this comment

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

之前应该想的有问题,这个配置最终生效在ctx.Request.Header的field上,这个是一个每次请求结束都会被重置的字段,看起来应该只能是每次解析请求前配置一次

@welkeyever
Copy link
Member

move to #940

@welkeyever welkeyever closed this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants