Implement Transport #4

bbengfort · 2026-01-13T14:54:30Z

Scope of changes

Implements cache based http.Transport.

Fixes SC-36556

Type of change

Author checklist

I have manually tested the change and/or added automation in the form of unit tests or integration tests
I have added new test fixtures as needed to support added tests
Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
I have moved the associated Shortcut story to "Ready for Review"

Copilot

Pull request overview

This PR implements a cache-based HTTP Transport that acts as an http.RoundTripper to cache HTTP responses according to RFC specifications. The implementation includes the Transport struct with comprehensive configuration options, helper functions for header parsing and HTTP method classification, and corresponding unit tests.

Changes:

Added Transport struct with fields for cache configuration (public/private cache, vary header handling, custom caching logic, etc.)
Implemented RoundTrip method with cache lookup, vary header support, and cache invalidation for unsafe HTTP methods
Added helper functions (allHeaderCSVs, isUnsafeMethod) with comprehensive test coverage
Added exports for testing private functions

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 18 comments.

File	Description
httpcache.go	Implemented Transport struct and RoundTrip method with cache-based HTTP request handling, plus helper functions for CSV header parsing and unsafe method detection
httpcache_test.go	Added comprehensive tests for allHeaderCSVs and isUnsafeMethod helper functions
export_test.go	Exported allHeaderCSVs and isUnsafeMethod for testing
cache.go	Added clarifying comments for cacheKeyWithHeaders function

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

httpcache.go

httpcache_test.go

httpcache.go

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 24 comments.

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

Copilot · 2026-01-14T17:14:34Z

httpcache.go

+					// Try with vary-specific key
+					cachedVary, varyErr := cachedResponse(t.Cache, varyKey, req)
+					if varyErr == nil && cachedVary != nil {
+						// Found a variant match use it as the cached value.


Missing comma after the last test case. The test case for "Found a variant match use it as the cached value." on line 139 in httpcache.go has incorrect punctuation - there should be a comma or the sentence should be restructured to "Found a variant match, use it as the cached value."

Copilot · 2026-01-14T17:14:35Z

httpcache.go

+	} else {
+		// RFC 7234 Section 4.4: Invalidate cache on unsafe methods
+		// Delete the request URI immediately for unsafe methods
+		t.Cache.Del(cacheKey)


Potential nil pointer dereference. If t.Cache is nil, calling t.Cache.Del(cacheKey) will panic. While the Transport struct has a Cache field, there's no validation to ensure it's set before use. Consider adding a nil check or documenting that Cache must be set, or validate this in NewTransport or at the start of RoundTrip.

Copilot · 2026-01-14T17:14:35Z

httpcache.go

+}
+
+// Determines if a response can be stored in the cache based on request/response
+// cache-control directives and status code. This method implements RFC 911 compliance.


Typo in comment: "RFC 911" should be "RFC 9111". This references the HTTP Caching specification.

Copilot · 2026-01-14T17:14:35Z

httpcache.go

+	}
+
+	if !cacheable || !t.canStore(rep, req, reqcc, repcc) {
+		t.Cache.Del(cacheKey)


Potential nil pointer dereference. If t.Cache is nil, the call to t.Cache.Del(cacheKey) on line 306 will panic. While the store method checks for nil Cache, this method doesn't. Either add a nil check at the beginning of cacheResponse, or ensure Cache validation happens earlier in the call chain.

Copilot · 2026-01-14T17:14:35Z

httpcache.go

+		Method: http.MethodGet,
+		URL:    url,
+	})
+	t.Cache.Del(getKey)


Potential nil pointer dereference. If t.Cache is nil, calling t.Cache.Del(getKey) will panic. This method is called from invalidateCache which is invoked in the main RoundTrip flow. Add a nil check for t.Cache before attempting cache operations.

Copilot · 2026-01-14T17:14:39Z

export_test.go

+	AllHeaderCSVs         = allHeaderCSVs
+	IsUnsafeMethod        = isUnsafeMethod
+	IsSameOrigin          = isSameOrigin
+	GetOrigin             = getOrigin
 )


The export_test.go file exports internal functions for testing but provides minimal documentation. Consider adding a comment explaining the purpose of each exported function to help maintainers understand what internal behavior is being tested.

Copilot · 2026-01-14T17:14:39Z

cache.go

 // headers in their canonical form. This allows you to differentiate cache entries
 // based on header values such as Authorization or custom headers.
 func cacheKeyWithHeaders(req *http.Request, headers []string) string {
+	// Create base cachekey


Inconsistent word spacing in comment: "cachekey" should be "cache key" (two words). The comment should read "// Create base cache key".

Copilot · 2026-01-14T17:14:40Z

httpcache.go

+func (t *Transport) processCachedResponse(cached *http.Response, req *http.Request, transport http.RoundTripper, cacheKey string) (rep *http.Response, err error) {
+	return nil, nil
+}


The processCachedResponse method is a stub that always returns nil for both the response and error. This will cause the RoundTrip method to return a nil response when a cached response is available, which violates the http.RoundTripper interface contract and will lead to nil pointer dereferences in calling code. This method needs to be fully implemented to process cached responses according to freshness, revalidation logic, and other caching requirements.

Copilot · 2026-01-14T17:14:40Z

io.go

+	if err == io.EOF || n < len(p) {
+		if r.OnEOF != nil {
+			r.OnEOF(bytes.NewReader(r.buf.Bytes()))
+		}
+	}


The OnEOF callback is triggered when err == io.EOF OR n < len(p). However, n < len(p) doesn't necessarily mean EOF has been reached - it could mean the read was smaller than the buffer size for other reasons (e.g., network chunk boundaries). This condition should only trigger when err == io.EOF to ensure the full content has been read. The current implementation may cause premature caching of partial responses.

Copilot · 2026-01-14T17:14:40Z

httpcache.go

+	// Handle Vary headers to store the body across multiple keys.
+	varyHeaders := allHeaderCSVs(rep.Header, Vary)
+	if !t.DisableVaryHeaderSeparation && len(varyHeaders) > 0 {
+		// Store the full response ounder both the variant key and the base key


Typo in comment: "ounder" should be "under". The comment should read "Store the full response under both the variant key and the base key".

Implement Transport

cdf5b3a

Copilot AI review requested due to automatic review settings January 13, 2026 14:54

Copilot started reviewing on behalf of bbengfort January 13, 2026 14:54 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

bbengfort added 2 commits January 14, 2026 10:40

store in cache with directives

1283c93

invalidate cache

d8f655d

Copilot AI review requested due to automatic review settings January 14, 2026 17:09

Copilot started reviewing on behalf of bbengfort January 14, 2026 17:10 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

Implement Transport #4

Are you sure you want to change the base?

Implement Transport #4

Uh oh!

Conversation

bbengfort commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope of changes

Type of change

Author checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bbengfort commented Jan 13, 2026 •

edited

Loading