Skip to content

refact: eliminate duplication in Request/Response via struct embedding #2027

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

Merged
merged 3 commits into from
Jul 5, 2025

Conversation

ksw2000
Copy link
Contributor

@ksw2000 ksw2000 commented Jun 22, 2025

Reduces code duplication by embedding the header struct into both RequestHeader and ResponseHeader to share common fields.

I found that there are many duplicate methods in header.go. We can reduce this repetition by using struct embedding. This PR improves maintainability by allowing shared logic between RequestHeader and ResponseHeader to be implemented in a single method with a single comment.

Additionally, by embedding the Header struct, it implicitly communicates to users which methods on RequestHeader and ResponseHeader share the exact same underlying implementation.

During the refactoring process, I also discovered that the SetProtocol methods in RequestHeader and ResponseHeader are inconsistent.

// SetProtocol sets HTTP request protocol.
func (h *RequestHeader) SetProtocol(method string) {
	h.protocol = append(h.protocol[:0], method...)
	h.noHTTP11 = !bytes.Equal(h.protocol, strHTTP11)
}

// SetProtocolBytes sets HTTP request protocol.
func (h *RequestHeader) SetProtocolBytes(method []byte) {
	h.protocol = append(h.protocol[:0], method...)
	h.noHTTP11 = !bytes.Equal(h.protocol, strHTTP11)
}

// SetProtocol sets response protocol bytes.
func (h *ResponseHeader) SetProtocol(protocol []byte) {
	h.protocol = append(h.protocol[:0], protocol...)
}

The same function name SetProtocol in RequestHeader and ResponseHeader have different inputs, and the ResponseHeader.SetProtocol() does not update the noHTTP11.

@ksw2000 ksw2000 marked this pull request as ready for review June 22, 2025 14:19
@erikdubbelboer erikdubbelboer requested a review from Copilot June 23, 2025 07:00
Copy link

@Copilot Copilot AI left a comment

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 refactors header handling by embedding the common header struct into both RequestHeader and ResponseHeader to eliminate code duplication and improve maintainability. Key changes include updating method calls to use the new ContentLength implementation and adjusting test initialization for secure logging behavior.

  • Replace calls to realContentLength() with ContentLength() in Request.
  • Update test initialization to assign secureErrorLogMessage via field assignment.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
http.go Updated method calls to use ContentLength() for header length checks
header_test.go Changed struct initialization for secureErrorLogMessage in tests
Comments suppressed due to low confidence (2)

header_test.go:2784

  • [nitpick] The change from initializing secureErrorLogMessage via composite literal to a separate field assignment is not immediately obvious; please add a brief inline comment explaining the necessity of this change.
	h := &ResponseHeader{}

http.go:1294

  • Verify that the new ContentLength() covers all cases previously handled by realContentLength(). If there are any behavioral differences, consider documenting them.
	contentLength := req.Header.ContentLength()

@ksw2000 ksw2000 requested a review from erikdubbelboer June 30, 2025 12:52
@erikdubbelboer erikdubbelboer merged commit eb1f908 into valyala:master Jul 5, 2025
11 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

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.

2 participants