🔥 feat: Support for SendEarlyHints #3483
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant DefaultCtx
Client->>FiberApp: Makes HTTP request
FiberApp->>DefaultCtx: Handles request
DefaultCtx->>DefaultCtx: SendEarlyHints(hints)
DefaultCtx-->>Client: Sends 103 Early Hints with Link headers
FiberApp-->>Client: Sends final HTTP response
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I can't figure out why the tests are failing. Something not quite right with Fasthttp |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ctx.go (2)
1563-1566: Clarify documentation about Link header handling.The documentation states "Only Link headers already written to the response will be transmitted as Early Hints" but the implementation actually adds the provided hints as Link headers first (line 1575), then calls
EarlyHints(). This seems inconsistent.Consider updating the documentation to accurately reflect the implementation:
-// SendEarlyHints allows the server to hint to the browser what resources a page would need -// so the browser can preload them while waiting for the server's full response. Only Link -// headers already written to the response will be transmitted as Early Hints. +// SendEarlyHints allows the server to hint to the browser what resources a page would need +// so the browser can preload them while waiting for the server's full response. The provided +// hints are added as Link headers to the response before transmitting the Early Hints.
1573-1578: Consider adding input validation for better robustness.The method doesn't validate the input hints slice or individual hint formats. While the underlying fasthttp library may handle some edge cases, adding basic validation would improve robustness and provide clearer error messages.
Consider adding input validation:
func (c *DefaultCtx) SendEarlyHints(hints []string) error { + if len(hints) == 0 { + return nil // No hints to send + } for _, h := range hints { + if h == "" { + continue // Skip empty hints + } c.fasthttp.Response.Header.Add("Link", h) } return c.fasthttp.EarlyHints() }Alternatively, you could validate Link header format according to RFC 8288 if stricter validation is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx.go(1 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
ctx_interface_gen.go (1)
296-306: Interface definition looks correct.The method signature and documentation are consistent with the implementation in
ctx.go. Since this is an auto-generated file (as noted in line 1), any documentation improvements should be made in the source file (ctx.go) and the interface will be updated accordingly when regenerated.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for the HTTP/2+ SendEarlyHints feature allowing the server to hint to the browser which resources to preload.
- Adds a new SendEarlyHints method to the Ctx interface and its implementation in DefaultCtx.
- Provides a new test case in ctx_test.go to validate the SendEarlyHints functionality.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ctx_test.go | Added a unit test for the SendEarlyHints method. |
| ctx_interface_gen.go | Updated the Ctx interface to include the SendEarlyHints method. |
| ctx.go | Implemented the SendEarlyHints method using fasthttp's EarlyHints API. |
Comments suppressed due to low confidence (1)
ctx_test.go:3113
- Consider adding additional test cases with multiple early hints to ensure that all Link headers are correctly processed, including scenarios with duplicates or a mixture of different hint strings.
err := c.SendEarlyHints([]string{"<https://cdn.com>; rel=preload; as=script"})
You have a race condition in the SendEarlyHints test: === Failed
=== FAIL: . Test_Ctx_SendEarlyHints (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xcfe6f4]
goroutine 186 [running]:
testing.tRunner.func1.2({0x1097120, 0x1812bc0})
/opt/hostedtoolcache/go/1.23.9/x64/src/testing/testing.go:1632 +0x3fc
testing.tRunner.func1()
/opt/hostedtoolcache/go/1.23.9/x64/src/testing/testing.go:1635 +0x6b6
panic({0x1097120?, 0x1812bc0?})
/opt/hostedtoolcache/go/1.23.9/x64/src/runtime/panic.go:791 +0x132
github.com/valyala/fasthttp.acquireWriter(0xc0002e1208)
/home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.62.0/server.go:2736 +0x54
github.com/valyala/fasthttp.(*RequestCtx).EarlyHints(0xc0002e1208)
/home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.62.0/server.go:647 +0xab
github.com/gofiber/fiber/v3.(*DefaultCtx).SendEarlyHints(0xc0002f6308, {0xc000264370, 0x1, 0x4bdb1b?})
/home/runner/work/fiber/fiber/ctx.go:1577 +0x1d6
github.com/gofiber/fiber/v3.Test_Ctx_SendEarlyHints(0xc0003bb380)
/home/runner/work/fiber/fiber/ctx_test.go:3123 +0xf9
testing.tRunner(0xc0003bb380, 0x11a8b70)
/opt/hostedtoolcache/go/1.23.9/x64/src/testing/testing.go:1690 +0x227
created by testing.(*T).Run in goroutine 1
/opt/hostedtoolcache/go/1.23.9/x64/src/testing/testing.go:1743 +0x826 |
|
How can I fix it? It's not obvious where the race-condition is. |
Seems the failure comes from Fasthttp? It happens when you call |
I would recommend using the |
ctx_test.go
Outdated
| // go test -race -run Test_Ctx_SendEarlyHints | ||
| func Test_Ctx_SendEarlyHints(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
|
|
||
| c := app.AcquireCtx(&fasthttp.RequestCtx{}) | ||
| defer app.ReleaseCtx(c) | ||
| err := c.SendEarlyHints([]string{"<https://cdn.com>; rel=preload; as=script"}) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we re-write this using app.Test()?
That way it can look something like:
// go test -race -run Test_Ctx_SendEarlyHints
func Test_Ctx_SendEarlyHints(t *testing.T) {
t.Parallel()
app := New()
hints := []string{"<https://cdn.com>; rel=preload; as=script"}
app.Get("/earlyhints", func(c Ctx) error {
return c.SendEarlyHints(hints)
})
req := httptest.NewRequest(MethodGet, "/earlyhints", nil)
resp, err := app.Test(req)
require.NoError(t, err, "app.Test(req)")
require.Equal(t, StatusEarlyHints, resp.StatusCode, "Status code")
require.Equal(t, hints, resp.Header["Link"], "Link header")
}
|
Something is wrong. I changed the code. app.Get("/earlyhints", func(c Ctx) error {
c.SendEarlyHints(hints)
c.Status(StatusBadRequest)
return nil
})
This is how the test should look like: https://github.com/valyala/fasthttp/blob/master/server_test.go#L1922 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3483 +/- ##
==========================================
- Coverage 90.91% 90.87% -0.04%
==========================================
Files 110 110
Lines 11062 11083 +21
==========================================
+ Hits 10057 10072 +15
- Misses 753 757 +4
- Partials 252 254 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ctx_test.go (1)
3114-3114:⚠️ Potential issueFix error handling for SendEarlyHints call.
The error return value from
c.SendEarlyHints(hints)is not being checked, which violates Go best practices and triggers static analysis warnings.Apply this fix:
- c.SendEarlyHints(hints) + err := c.SendEarlyHints(hints) + if err != nil { + return err + }
🧹 Nitpick comments (1)
ctx_test.go (1)
3122-3123: Remove debug print statements.These debug print statements should be removed from the test as they add noise to test output.
- fmt.Println(resp.StatusCode) - fmt.Println(resp.Header) -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctx_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
ctx_test.go
[failure] 3124-3124:
Error return value of c.SendEarlyHints is not checked (errcheck)
🪛 GitHub Actions: golangci-lint
ctx_test.go
[error] 3124-3124: golangci-lint: Error return value of c.SendEarlyHints is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
ctx_test.go
Outdated
| // go test -race -run Test_Ctx_SendEarlyHints | ||
| func Test_Ctx_SendEarlyHints(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
|
|
||
| hints := []string{"<https://cdn.com>; rel=preload; as=script"} | ||
| app.Get("/earlyhints", func(c Ctx) error { | ||
| c.SendEarlyHints(hints) | ||
| c.Status(StatusBadRequest) | ||
| return nil | ||
| }) | ||
|
|
||
| req := httptest.NewRequest(MethodGet, "/earlyhints", nil) | ||
| resp, err := app.Test(req) | ||
|
|
||
| fmt.Println(resp.StatusCode) | ||
| fmt.Println(resp.Header) | ||
|
|
||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, StatusEarlyHints, resp.StatusCode, "Status code") | ||
| require.Equal(t, hints, resp.Header["Link"], "Link header") | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify early hints implementation addresses race condition concerns.
While this test follows the previous suggestion to use app.Test(), the PR comments mention ongoing race condition issues with the SendEarlyHints functionality. Consider adding additional verification to ensure the early hints are properly sent before the final response.
You might want to verify that this test implementation actually resolves the race condition issues mentioned in the PR discussion. Consider adding test cases that verify:
- Early hints are sent before final response
- Multiple early hints can be sent
- The functionality works correctly under concurrent load
🏁 Script executed:
#!/bin/bash
# Description: Check for other early hints tests and race condition handling
# Search for other early hints related tests
rg -A 10 -B 5 "SendEarlyHints|EarlyHints" --type go
# Look for race condition mentions in comments
rg -A 5 -B 5 "race|Race" --type go
# Check for similar test patterns using app.Test
rg -A 5 "app\.Test.*httptest\.NewRequest" --type goLength of output: 69437
Extend SendEarlyHints tests for multiple hints and concurrent use
The current Test_Ctx_SendEarlyHints in ctx_test.go only covers a single hint and a single request. We haven’t found any tests addressing multiple hints or concurrent execution, nor verifying that early hints are flushed before the final status. Please add the following:
• In ctx_test.go (around lines 3107–3128):
– Add a subtest that calls c.SendEarlyHints with multiple values (e.g. two distinct Link headers) and asserts resp.Header["Link"] contains both entries.
– Verify that calling c.Status(...) after SendEarlyHints does not override the 103 response.
• Still in ctx_test.go:
– Introduce Test_Ctx_SendEarlyHints_Concurrent with t.Parallel() and spawn multiple app.Test(httptest.NewRequest(...)) invocations (e.g. via a sync.WaitGroup) to confirm no data races under load.
🧰 Tools
🪛 GitHub Check: lint
[failure] 3124-3124:
Error return value of c.SendEarlyHints is not checked (errcheck)
🪛 GitHub Actions: golangci-lint
[error] 3124-3124: golangci-lint: Error return value of c.SendEarlyHints is not checked (errcheck)
🤖 Prompt for AI Agents
In ctx_test.go around lines 3107 to 3128, extend the Test_Ctx_SendEarlyHints by
adding a subtest that sends multiple early hints using c.SendEarlyHints with
multiple Link header values and assert that resp.Header["Link"] contains all
sent hints. Also verify that calling c.Status after SendEarlyHints does not
override the 103 Early Hints status code. Additionally, add a new test named
Test_Ctx_SendEarlyHints_Concurrent that runs in parallel and uses a
sync.WaitGroup to spawn multiple concurrent app.Test calls with
httptest.NewRequest to ensure no data races occur during concurrent early hints
sending.
Can you try |
|
Didn't work |
|
I tested it locally, and I can confirm that My thoughts are that one of the following is happening:
For example, the HTTP response: HTTP/1.1 103 Early Hints
Link: <https://cdn.com>; rel=preload; as=script
HTTP/1.1 200 OK
Content-Length: 0
Link: <https://cdn.com>; rel=preload; as=scriptCould be parsed as only: HTTP/1.1 103 Early Hints
Link: <https://cdn.com>; rel=preload; as=script
Possibly ignoring the rest of the response, or
If this is working properly locally, I believe that it's |
@pjebs Do you know how a HTTP/2+ client would read in the request? For example, should: HTTP/1.1 103 Early Hints
Link: <https://cdn.com>; rel=preload; as=script
HTTP/1.1 400 Bad Request
Content-Length: 0
Link: <https://cdn.com>; rel=preload; as=scriptbe read as: HTTP/1.1 400 Bad Request
Content-Length: 0
Link: <https://cdn.com>; rel=preload; as=scriptOr would the client read it in some sort of other way? If the above is valid, then I have an idea of a (hacky) solution to get this to work within As of for actual clients, I believe that some sort of proxy that upgrades |
|
Fiber is a http 1.1 server (and client). It should simply ignore interpreting the early hints and associated headers etc until it encounters a 2xx,3xx, 4xx or 5xx I believe. A http 2 client will properly interpret the early hints. But the final status code will not be the early hints. But for testing the server, we still need to verify that a 103 was first sent and then the 400 |
There was a problem hiding this comment.
- add markdown description in ctx.md
- add info about this new feature in whatsnew.md
- check lint errors https://github.com/gofiber/fiber/actions/runs/15266832165/job/42933912986?pr=3483
|
@pjebs thx for the implementation , check my hints |
|
Implementation is stuck right now |
|
because of the problem with http2? if fiber sits behind a reverse proxy and communicates via http2 then the feature should work |
|
The issue is with the Test function and it's population of Response object's status. Also potentially fiber.Client object. Also, a way to read the Conn data sequentially (only for unit test purposes) |
|
is it not possible to test it like in fasthttp ? or with an inmemory listener ? |
|
maybe we should provide another test method that uses buffers so that streaming etc. can be tested |
@ReneWerner87 From my own testing, it is possible to modify our existing
I have tried 1.b, and it seems to work for the unit test with |
|
A similar update for the above could be made for |
|
@pjebs any progress ? |
|
It's blocked by issues in Fiber |
|
Ok, i will test @grivera64 way for the tests |
|
@grivera64 you mean like this ? Patchdiff --git a/app.go b/app.go
index 75d83fd2dcbe7e916067b65a1f1a279ea0f48662..4229cabd3a95bdadf62957613a06b0e9313418cc 100644
--- a/app.go
+++ b/app.go
@@ -1094,60 +1094,74 @@ func (app *App) Test(req *http.Request, config ...TestConfig) (*http.Response, e
channel <- app.server.ServeConn(conn)
returned = true
}()
// Wait for callback
if cfg.Timeout > 0 {
// With timeout
select {
case err = <-channel:
case <-time.After(cfg.Timeout):
conn.Close() //nolint:errcheck // It is fine to ignore the error here
if cfg.FailOnTimeout {
return nil, os.ErrDeadlineExceeded
}
}
} else {
// Without timeout
err = <-channel
}
// Check for errors
if err != nil && !errors.Is(err, fasthttp.ErrGetOnly) && !errors.Is(err, errTestConnClosed) {
return nil, err
}
- // Read response
+ // Read response(s)
buffer := bufio.NewReader(&conn.w)
- // Convert raw http response to *http.Response
- res, err := http.ReadResponse(buffer, req)
- if err != nil {
- if errors.Is(err, io.ErrUnexpectedEOF) {
- return nil, ErrTestGotEmptyResponse
+ var res *http.Response
+ for {
+ // Convert raw http response to *http.Response
+ res, err = http.ReadResponse(buffer, req)
+ if err != nil {
+ if errors.Is(err, io.ErrUnexpectedEOF) {
+ return nil, ErrTestGotEmptyResponse
+ }
+ return nil, fmt.Errorf("failed to read response: %w", err)
+ }
+
+ // Break if this response is non-1xx or there are no more responses
+ if res.StatusCode >= http.StatusOK || buffer.Buffered() == 0 {
+ break
+ }
+
+ // Discard interim response body before reading the next one
+ if res.Body != nil {
+ _, _ = io.Copy(io.Discard, res.Body)
+ res.Body.Close()
}
- return nil, fmt.Errorf("failed to read response: %w", err)
}
return res, nil
}
type disableLogger struct{}
func (*disableLogger) Printf(string, ...any) {
}
func (app *App) init() *App {
// lock application
app.mutex.Lock()
// Initialize Services when needed,
// panics if there is an error starting them.
app.initServices()
// Only load templates if a view engine is specified
if app.config.Views != nil {
if err := app.config.Views.Load(); err != nil {
log.Warnf("failed to load views: %v", err)
}
}
diff --git a/app_test.go b/app_test.go
index 60232160e029885bd1215ee53e3eb05dbf40ddce..9d887d90518d801d5169bddcae0cdd64db9f5243 100644
--- a/app_test.go
+++ b/app_test.go
@@ -1737,50 +1737,71 @@ func Test_App_Test_timeout_empty_response(t *testing.T) {
return nil
})
_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: 100 * time.Millisecond,
FailOnTimeout: false,
})
require.ErrorIs(t, err, ErrTestGotEmptyResponse)
}
func Test_App_Test_drop_empty_response(t *testing.T) {
t.Parallel()
app := New()
app.Get("/", func(c Ctx) error {
return c.Drop()
})
_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: 0,
FailOnTimeout: false,
})
require.ErrorIs(t, err, ErrTestGotEmptyResponse)
}
+func Test_App_Test_EarlyHints(t *testing.T) {
+ t.Parallel()
+
+ app := New()
+ hints := []string{"<https://cdn.com>; rel=preload; as=script"}
+ app.Get("/early", func(c Ctx) error {
+ c.SendEarlyHints(hints)
+ return c.Status(StatusOK).SendString("done")
+ })
+
+ req := httptest.NewRequest(MethodGet, "/early", nil)
+ resp, err := app.Test(req)
+
+ require.NoError(t, err)
+ require.Equal(t, StatusOK, resp.StatusCode)
+ require.Equal(t, hints, resp.Header["Link"])
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "done", string(body))
+}
+
func Test_App_SetTLSHandler(t *testing.T) {
t.Parallel()
tlsHandler := &TLSHandler{clientHelloInfo: &tls.ClientHelloInfo{
ServerName: "example.golang",
}}
app := New()
app.SetTLSHandler(tlsHandler)
c := app.AcquireCtx(&fasthttp.RequestCtx{})
defer app.ReleaseCtx(c)
require.Equal(t, "example.golang", c.ClientHelloInfo().ServerName)
}
func Test_App_AddCustomRequestMethod(t *testing.T) {
t.Parallel()
methods := append(DefaultMethods, "TEST") //nolint:gocritic // We want a new slice here
app := New(Config{
RequestMethods: methods,
})
appMethods := app.config.RequestMethods
// method name is always uppercase - https://datatracker.ietf.org/doc/html/rfc7231#section-4.1
require.Equal(t, len(app.stack), len(appMethods))
diff --git a/ctx_test.go b/ctx_test.go
index 0510f7b47e27f241df7a16411e8447e8bf2b57a0..0de5eea8f4253e3961d8c8922b4b31480143286b 100644
--- a/ctx_test.go
+++ b/ctx_test.go
@@ -3467,62 +3467,62 @@ func Test_Ctx_Download(t *testing.T) {
}()
expect, err := io.ReadAll(f)
require.NoError(t, err)
require.Equal(t, expect, c.Response().Body())
require.Equal(t, `attachment; filename="Awesome+File%21"`, string(c.Response().Header.Peek(HeaderContentDisposition)))
require.NoError(t, c.Res().Download("ctx.go"))
require.Equal(t, `attachment; filename="ctx.go"`, string(c.Response().Header.Peek(HeaderContentDisposition)))
require.NoError(t, c.Download("ctx.go", "файл.txt"))
header := string(c.Response().Header.Peek(HeaderContentDisposition))
require.Contains(t, header, `filename="файл.txt"`)
require.Contains(t, header, `filename*=UTF-8''%D1%84%D0%B0%D0%B9%D0%BB.txt`)
}
// go test -race -run Test_Ctx_SendEarlyHints
func Test_Ctx_SendEarlyHints(t *testing.T) {
t.Parallel()
app := New()
hints := []string{"<https://cdn.com>; rel=preload; as=script"}
app.Get("/earlyhints", func(c Ctx) error {
c.SendEarlyHints(hints)
c.Status(StatusBadRequest)
- return nil
+ return c.SendString("fail")
})
req := httptest.NewRequest(MethodGet, "/earlyhints", nil)
resp, err := app.Test(req)
- fmt.Println(resp.StatusCode)
- fmt.Println(resp.Header)
-
require.NoError(t, err, "app.Test(req)")
- require.Equal(t, StatusEarlyHints, resp.StatusCode, "Status code")
+ require.Equal(t, StatusBadRequest, resp.StatusCode, "Status code")
require.Equal(t, hints, resp.Header["Link"], "Link header")
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "fail", string(body))
}
// go test -race -run Test_Ctx_SendFile
func Test_Ctx_SendFile(t *testing.T) {
t.Parallel()
app := New()
// fetch file content
f, err := os.Open("./ctx.go")
require.NoError(t, err)
defer func() {
require.NoError(t, f.Close())
}()
expectFileContent, err := io.ReadAll(f)
require.NoError(t, err)
// fetch file info for the not modified test case
fI, err := os.Stat("./ctx.go")
require.NoError(t, err)
// simple test case
c := app.AcquireCtx(&fasthttp.RequestCtx{})
err = c.SendFile("ctx.go")
// check expectation
require.NoError(t, err)
require.Equal(t, expectFileContent, c.Response().Body())
|
@ReneWerner87 Yes, the patch LGTM 🚀 We could also consider having a separate Test method that returns the full stream of responses as a slice, but I am not sure if this is necessary. If that's not needed, then I believe this is all we need to merge the PR to main. |
|
@pjebs can you check my patch |
|
Where is the patch? |
@pjebs There's a dropdown labeled "patch" in @ReneWerner87's comment: |
|
Do I have access to the repo with the patch? |
its in the comment
just open it and copy the git patch (everyone can see the comment) |
|
@pjebs should i push the patch changes in the branch ? |
|
i applied the patch to your branch |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app_test.go (1)
1762-1781: Good test coverage for the Early Hints feature.The test effectively validates the integration between
SendEarlyHintsand the modifiedTestmethod. It properly:
- Tests the happy path scenario with Link headers
- Verifies that early hints are preserved in the final response
- Confirms the final response status and body are correct
- Uses appropriate assertions with proper error handling
The test follows established patterns in the file and provides good coverage for the basic functionality.
Consider adding additional test cases to enhance coverage:
func Test_App_Test_EarlyHints_Multiple(t *testing.T) { t.Parallel() app := New() app.Get("/multiple", func(c Ctx) error { c.SendEarlyHints([]string{"<https://cdn.com>; rel=preload; as=script"}) c.SendEarlyHints([]string{"<https://fonts.com>; rel=preload; as=font"}) return c.Status(StatusOK).SendString("done") }) req := httptest.NewRequest(MethodGet, "/multiple", nil) resp, err := app.Test(req) require.NoError(t, err) require.Equal(t, StatusOK, resp.StatusCode) // Verify multiple Link headers are combined require.Len(t, resp.Header["Link"], 2) } func Test_App_Test_EarlyHints_Error_After_Hints(t *testing.T) { t.Parallel() app := New() app.Get("/error", func(c Ctx) error { c.SendEarlyHints([]string{"<https://cdn.com>; rel=preload; as=script"}) return c.Status(StatusBadRequest).SendString("error") }) req := httptest.NewRequest(MethodGet, "/error", nil) resp, err := app.Test(req) require.NoError(t, err) require.Equal(t, StatusBadRequest, resp.StatusCode) // Verify early hints are still present even with error response require.NotEmpty(t, resp.Header["Link"]) }These additional tests would provide more comprehensive coverage of edge cases and multiple early hints scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.go(1 hunks)app_test.go(1 hunks)ctx_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ctx_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
app_test.go (2)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
app.go (5)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: repeated
- GitHub Check: lint
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: Analyse
🔇 Additional comments (1)
app.go (1)
1119-1143: LGTM! Excellent implementation of multi-response handling.The modification correctly implements support for processing multiple HTTP responses, which is essential for the Early Hints feature. The loop logic properly:
- Reads responses sequentially until a non-1xx response is found
- Handles resource cleanup by discarding interim response bodies
- Maintains robust error handling for edge cases
- Preserves backward compatibility with existing functionality
The implementation aligns well with the PR objective to support HTTP 103 Early Hints while maintaining the existing Test method behavior.
@ReneWerner87 @pjebs the codecov workflow wants more coverage for the Should we add a new |
|
@grivera64 can you add this ? |
Description
Please provide a clear and concise description of the changes you've made and the problem they address. Include the purpose of the change, any relevant issues it solves, and the benefits it brings to the project. If this change introduces new features or adjustments, highlight them here.
Fixes #3211
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md