Skip to content

fix: caching middleware's partRequest getUpstreamReader range request…#38

Merged
sendya merged 1 commit intomainfrom
fix/part-request-unused-clone-req
Feb 4, 2026
Merged

fix: caching middleware's partRequest getUpstreamReader range request…#38
sendya merged 1 commit intomainfrom
fix/part-request-unused-clone-req

Conversation

@sendya
Copy link
Member

@sendya sendya commented Feb 4, 2026

This pull request simplifies the logic in the getContents function by removing manual HTTP request cloning and range header setting, instead delegating these responsibilities to the getUpstreamReader method.

Refactoring and simplification:

  • Removed manual cloning of c.req and manual setting of the Range header in favor of using getUpstreamReader, which now handles copying the request and setting the appropriate headers. (server/middleware/caching/internal.go) [1] [2]

Copilot AI review requested due to automatic review settings February 4, 2026 13:00
Copy link
Contributor

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 pull request refactors the getContents function in the caching middleware to eliminate code duplication by delegating HTTP request cloning and range header setting to the existing getUpstreamReader method.

Changes:

  • Removed manual request cloning and range header manipulation in favor of using the getUpstreamReader method which already handles these operations
  • Added clarifying comments indicating that request cloning is handled automatically by getUpstreamReader

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

newRange := fmt.Sprintf("bytes=%d-%d", fromByte, toByte)
req.Header.Set("Range", newRange)

// auto-copy request with c.req
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The comment "auto-copy request with c.req" is grammatically awkward. Consider rephrasing to "Request is automatically cloned by getUpstreamReader" or "getUpstreamReader handles request cloning and range header setting" for better clarity.

Suggested change
// auto-copy request with c.req
// getUpstreamReader handles request cloning and range header setting

Copilot uses AI. Check for mistakes.
req := c.req.Clone(context.Background())
req.Header.Set("Range", newRange)

// auto-copy request with c.req
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The comment "auto-copy request with c.req" is grammatically awkward. Consider rephrasing to "Request is automatically cloned by getUpstreamReader" or "getUpstreamReader handles request cloning and range header setting" for better clarity.

Copilot uses AI. Check for mistakes.
@sendya sendya force-pushed the fix/part-request-unused-clone-req branch from e086297 to b037391 Compare February 4, 2026 13:20
@sendya sendya merged commit ce4d992 into main Feb 4, 2026
1 check passed
@sendya sendya deleted the fix/part-request-unused-clone-req branch February 12, 2026 15:53
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.

1 participant