diff --git a/bench_test.go b/bench_test.go index c2b546b..7d02a18 100644 --- a/bench_test.go +++ b/bench_test.go @@ -29,6 +29,14 @@ func (writer *discardWriter) Write(data []byte) (int, error) { return len(data), nil } +// WriteString is what *http.response offers, so the double offers it too: +// without it the benchmark measures a string copy no real server makes. +func (writer *discardWriter) WriteString(data string) (int, error) { + writer.written += len(data) + + return len(data), nil +} + func (writer *discardWriter) WriteHeader(status int) { writer.status = status } diff --git a/call.go b/call.go index 6b5fd1c..33dc92e 100644 --- a/call.go +++ b/call.go @@ -22,6 +22,15 @@ import ( const sessionCookieName = "govalin-session" +// The response Content-Type is one of a fixed set and "Content-Type" is already +// the canonical form net/http would derive, so the header is assigned rather +// than built per response. +var ( + contentTypeTextPlain = []string{contenttypes.TextPlain + "; charset=" + charsets.UTF8} + contentTypeTextHTML = []string{contenttypes.TextHTML + "; charset=" + charsets.UTF8} + contentTypeJSON = []string{contenttypes.ApplicationJSON + "; charset=" + charsets.UTF8} +) + type raw struct { W *http.ResponseWriter Req *http.Request @@ -38,49 +47,41 @@ type Call struct { config *Config status int bypassLifecycle bool - w *responseWriter + w responseWriter + rawWriter http.ResponseWriter req *http.Request pathParams map[string]string bodyBytes []byte bodyErr error formParsed bool formErr error - charset string session session.Session Raw raw // Raw contains the raw request and response } -func newCallFromRequest(w http.ResponseWriter, req *http.Request, config *Config, pathParams map[string]string) Call { - govalinIDHeader := req.Header[headers.XGovalinID] - +func newCallFromRequest(w http.ResponseWriter, req *http.Request, config *Config, pathParams map[string]string) *Call { var uniqueID string - if govalinIDHeader == nil { - uniqueID = newCallID() - } else { + if govalinIDHeader := req.Header[headers.XGovalinID]; govalinIDHeader != nil { uniqueID = govalinIDHeader[0] } - // Even Raw.W goes through the recording writer, so govalin sees a commit whoever made it. - recordingWriter := newResponseWriter(w) - var rawWriter http.ResponseWriter = recordingWriter - - call := Call{ + call := &Call{ id: uniqueID, config: config, - w: recordingWriter, + w: responseWriter{ResponseWriter: w}, req: req, status: 0, bypassLifecycle: false, pathParams: pathParams, - charset: charsets.UTF8, - Raw: raw{ - W: &rawWriter, - Req: req, - }, } + // Even Raw.W goes through the recording writer, so govalin sees a commit whoever + // made it. Both point into the call itself, so a request is one allocation. + call.rawWriter = &call.w + call.Raw = raw{W: &call.rawWriter, Req: req} + if config.server.sessionsEnabled { - initiateSessionFromCall(&call) + initiateSessionFromCall(call) } return call @@ -385,8 +386,18 @@ func (call *Call) writtenStatus() int { return call.status } -// ID gives an UUIDv4 string that's unique to the call. +// ID gives an UUIDv4 string that's unique to the call, or the caller's own ID +// when the request carried an X-Govalin-Id header. +// +// The ID is minted on first use, so a request nobody asks the ID of never pays +// for one; every later call on the same request gets the same string. Like the +// rest of Call it belongs to the goroutine serving the request, so a handler +// that fans out should read the ID before it does. func (call *Call) ID() string { + if call.id == "" { + call.id = newCallID() + } + return call.id } @@ -442,7 +453,7 @@ func (call *Call) isSecure() bool { func (call *Call) Cookie(name string, cookies ...*http.Cookie) (*http.Cookie, error) { if len(cookies) > 0 { cookies[0].Name = name - http.SetCookie(call.w, cookies[0]) + http.SetCookie(&call.w, cookies[0]) call.cachePrivate() return cookies[0], nil @@ -642,7 +653,19 @@ func (call *Call) Status(statusCode ...int) int { // forbids one is not worth reporting: a handler that produced a body after // NotModified matched wasted its own work, and the response is still correct. func (call *Call) writeBody(data []byte) { - if _, err := call.w.Write(data); err != nil && !errors.Is(err, http.ErrBodyNotAllowed) { + _, err := call.w.Write(data) + call.reportBodyError(err) +} + +// writeBodyString writes a response body that is already a string, without the +// copy a []byte conversion would make of it. +func (call *Call) writeBodyString(text string) { + _, err := call.w.WriteString(text) + call.reportBodyError(err) +} + +func (call *Call) reportBodyError(err error) { + if err != nil && !errors.Is(err, http.ErrBodyNotAllowed) { slog.Error(fmt.Sprintf("Error when trying write to response, %v", err)) } } @@ -652,9 +675,9 @@ func (call *Call) writeBody(data []byte) { // Text will set the content-type of the response as text/plain and write it to the response. // If no other status has been given the response, it will write a 200 OK to the response. func (call *Call) Text(text string) { - call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.TextPlain, call.charset)) + call.w.Header()[headers.ContentType] = contentTypeTextPlain call.sendStatusOrDefault() - call.writeBody([]byte(text)) + call.writeBodyString(text) } // Send text as HTML to response @@ -662,9 +685,9 @@ func (call *Call) Text(text string) { // HTML will set the content-type of the response as text/html and write it to the response. // If no other status has been given the response, it will write a 200 OK to the response. func (call *Call) HTML(text string) { - call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.TextHTML, call.charset)) + call.w.Header()[headers.ContentType] = contentTypeTextHTML call.sendStatusOrDefault() - call.writeBody([]byte(text)) + call.writeBodyString(text) } // Send obj as JSON to response @@ -673,7 +696,7 @@ func (call *Call) HTML(text string) { // object as JSON, and writes it to the response. If no other status has been given the response, // it will write a 200 OK to the response. func (call *Call) JSON(obj interface{}) { - call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.ApplicationJSON, charsets.UTF8)) + call.w.Header()[headers.ContentType] = contentTypeJSON jsonBytes, err := json.Marshal(obj) if err != nil { slog.Error(fmt.Sprintf("error when trying to JSON marshall object, %v", err)) diff --git a/call_test.go b/call_test.go index 4c9f68a..72156af 100644 --- a/call_test.go +++ b/call_test.go @@ -11,6 +11,7 @@ import ( "github.com/pkkummermo/govalin" "github.com/pkkummermo/govalin/govalintest" + "github.com/pkkummermo/govalin/internal/http/contenttypes" "github.com/pkkummermo/govalin/internal/http/headers" "github.com/stretchr/testify/assert" ) @@ -403,6 +404,22 @@ func TestRequestID(t *testing.T) { }) } +// TestRequestIDIsStableAcrossReads covers the ID being minted on first use: a +// handler that reads it more than once gets one ID, not one per read. +func TestRequestIDIsStableAcrossReads(t *testing.T) { + app := newTestApp() + app.Get("/govalin", func(call *govalin.Call) { + call.Text(call.ID() + " " + call.ID()) + }) + + govalintest.Test(t, app, func(client *govalintest.Client) { + reads := strings.Fields(client.Get("/govalin")) + + assert.Len(t, reads, 2, "Should have read the ID twice") + assert.Equal(t, reads[0], reads[1], "Should give the same ID on every read of a call") + }) +} + func TestRedirect(t *testing.T) { app := newTestApp() app.Get("/govalin", func(call *govalin.Call) { @@ -653,3 +670,25 @@ func TestUninferableBodyTargetDoesNotCompile(t *testing.T) { assert.Contains(t, string(output), "type user of u does not match *T") assert.Contains(t, string(output), "in call to call.BodyAs, cannot infer T") } + +// TestBodyWriterReplacesAnExistingContentType covers a handler that set the +// content type itself before writing: the response carries the one the body +// writer chose, not both. +func TestBodyWriterReplacesAnExistingContentType(t *testing.T) { + app := newTestApp() + app.Get("/greeting", func(call *govalin.Call) { + call.Header(headers.ContentType, contenttypes.ApplicationJSON) + call.Text("hei") + }) + + govalintest.Test(t, app, func(client *govalintest.Client) { + response := client.GetResponse("/greeting") + + assert.Equal( + t, + []string{contenttypes.TextPlain + "; charset=utf-8"}, + response.Header.Values(headers.ContentType), + "Should send the content type of the body it actually wrote, and only that one", + ) + }) +} diff --git a/internal/http/headers/headers.go b/internal/http/headers/headers.go index c04c807..2e5b344 100644 --- a/internal/http/headers/headers.go +++ b/internal/http/headers/headers.go @@ -77,8 +77,3 @@ const ( XHttpMethodOverride = "X-HTTP-Method-Override" XPermittedCrossDomainPolicies = "X-Permitted-Cross-Domain-Policies" ) - -// ContentTypeHeader generates a Content-Type header value based on given content type and charset. -func ContentTypeHeader(contentType string, charset string) string { - return contentType + "; charset=" + charset -} diff --git a/perf_test.go b/perf_test.go index beb4ca0..0b9c1a7 100644 --- a/perf_test.go +++ b/perf_test.go @@ -31,13 +31,13 @@ var allocationBudgets = []allocationBudget{ { name: "text", target: "/text", - allowed: 8, + allowed: 1, build: func(app *App) { app.Get("/text", func(call *Call) { call.Text("Hello world") }) }, }, { name: "json", target: "/json", - allowed: 9, + allowed: 3, build: func(app *App) { type payload struct { Name string `json:"name"` @@ -50,13 +50,13 @@ var allocationBudgets = []allocationBudget{ { name: "status only", target: "/status", - allowed: 5, + allowed: 1, build: func(app *App) { app.Get("/status", func(call *Call) { call.Status(http.StatusNoContent) }) }, }, { name: "before and after handlers", target: "/text", - allowed: 8, + allowed: 1, build: func(app *App) { app.Before("/*", func(_ *Call) bool { return true }) app.Get("/text", func(call *Call) { call.Text("Hello world") }) @@ -66,13 +66,13 @@ var allocationBudgets = []allocationBudget{ { name: "not found", target: "/missing", - allowed: 33, + allowed: 27, build: func(app *App) { app.Get("/text", func(call *Call) { call.Text("Hello world") }) }, }, { name: "raw handler", target: "/raw", - allowed: 6, + allowed: 2, build: func(app *App) { app.HTTPServe("/raw", func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("Hello world")) @@ -82,7 +82,7 @@ var allocationBudgets = []allocationBudget{ { name: "static file", target: "/static/sub/test.html", - allowed: 39, + allowed: 35, build: func(app *App) { app.Static("/static", func(_ *Call, staticConfig *StaticConfig) { staticConfig.WithStaticPath("internal/testdata/static") diff --git a/responsewriter.go b/responsewriter.go index 4a38286..c2ce3bc 100644 --- a/responsewriter.go +++ b/responsewriter.go @@ -20,10 +20,6 @@ type responseWriter struct { committed bool } -func newResponseWriter(writer http.ResponseWriter) *responseWriter { - return &responseWriter{ResponseWriter: writer} -} - // WriteHeader records the status and passes it on unchanged. A repeated call is // still forwarded, so net/http keeps reporting a genuine double write; the // framework's own status flush is guarded by the committed flag instead. @@ -44,6 +40,19 @@ func (writer *responseWriter) Write(data []byte) (int, error) { return writer.ResponseWriter.Write(data) } +// WriteString keeps net/http's string path: *http.response implements +// io.StringWriter, and without this a string body would be copied into a byte +// slice of its own before every write. +func (writer *responseWriter) WriteString(data string) (int, error) { + writer.markCommitted() + + if stringWriter, ok := writer.ResponseWriter.(io.StringWriter); ok { + return stringWriter.WriteString(data) + } + + return writer.ResponseWriter.Write([]byte(data)) +} + // ReadFrom keeps net/http's sendfile path: io.Copy prefers an io.ReaderFrom, and // without this every streamed body would fall back to a buffered copy. func (writer *responseWriter) ReadFrom(reader io.Reader) (int64, error) { diff --git a/responsewriter_test.go b/responsewriter_test.go index 42ee450..09d17fb 100644 --- a/responsewriter_test.go +++ b/responsewriter_test.go @@ -58,6 +58,12 @@ func TestResponseWriterTracksCommitment(t *testing.T) { committed: true, status: http.StatusOK, }, + { + name: "a WriteString commits an implicit 200", + act: func(writer *responseWriter) { _, _ = writer.WriteString("body") }, + committed: true, + status: http.StatusOK, + }, { name: "a ReadFrom commits an implicit 200", act: func(writer *responseWriter) { _, _ = writer.ReadFrom(strings.NewReader("body")) }, @@ -86,7 +92,7 @@ func TestResponseWriterTracksCommitment(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - writer := newResponseWriter(httptest.NewRecorder()) + writer := &responseWriter{ResponseWriter: httptest.NewRecorder()} test.act(writer) @@ -99,3 +105,23 @@ func TestResponseWriterTracksCommitment(t *testing.T) { }) } } + +// writeOnly hides the WriteString a recorder happens to have, leaving the plain +// http.ResponseWriter that third-party middleware may hand the wrapper. +type writeOnly struct { + http.ResponseWriter +} + +// TestResponseWriterWritesStringsWithoutAStringWriter covers the wrapped writer +// not taking strings: the body still has to reach it. +func TestResponseWriterWritesStringsWithoutAStringWriter(t *testing.T) { + recorder := httptest.NewRecorder() + writer := &responseWriter{ResponseWriter: writeOnly{recorder}} + + if _, err := writer.WriteString("body"); err != nil { + t.Fatalf("WriteString returned %v", err) + } + if recorder.Body.String() != "body" { + t.Errorf("body is %q, expected %q", recorder.Body.String(), "body") + } +} diff --git a/server.go b/server.go index ca28199..81901d1 100644 --- a/server.go +++ b/server.go @@ -10,6 +10,7 @@ import ( "os" "time" + "github.com/pkkummermo/govalin/internal/http/headers" "github.com/pkkummermo/govalin/internal/validation" ) @@ -433,9 +434,11 @@ func (server *App) matchAfterHandlers(call *Call) { } } +var serverHeader = []string{"govalin"} + func (server *App) rootHandlerFunc(w http.ResponseWriter, req *http.Request) { incomingRequestTime := time.Now() - w.Header().Add("Server", "govalin") + w.Header()[headers.Server] = serverHeader call := newCallFromRequest( w, @@ -447,7 +450,7 @@ func (server *App) rootHandlerFunc(w http.ResponseWriter, req *http.Request) { // Deferred so a bypassed or short-circuited request is logged like any other. if server.config.server.accessLogEnabled { defer func() { - server.logAccessLog(&call, float64(time.Since(incomingRequestTime))/float64(time.Millisecond)) + server.logAccessLog(call, float64(time.Since(incomingRequestTime))/float64(time.Millisecond)) }() } @@ -459,23 +462,23 @@ func (server *App) rootHandlerFunc(w http.ResponseWriter, req *http.Request) { } }() - if !server.matchBeforeHandlers(&call) || call.bypassLifecycle { + if !server.matchBeforeHandlers(call) || call.bypassLifecycle { return } - server.matchHandlers(&call) + server.matchHandlers(call) if call.bypassLifecycle { return } - server.matchAfterHandlers(&call) + server.matchAfterHandlers(call) if call.bypassLifecycle { return } // A handler that wrote without setting a status has handled it; a 404 body would corrupt it. if call.Status() == 0 && !call.committed() { - server.notFoundHandler(&call) + server.notFoundHandler(call) } } diff --git a/stream.go b/stream.go index 844c9ab..51bfbf6 100644 --- a/stream.go +++ b/stream.go @@ -34,7 +34,7 @@ func (call *Call) Stream(contentType string, reader io.Reader) error { // io.Copy cannot say which side failed, and the two sides call for opposite answers. source := &sourceReader{reader: reader} - if _, err := io.Copy(call.w, source); err != nil { + if _, err := io.Copy(&call.w, source); err != nil { if source.err != nil { return fmt.Errorf("failed to read the streamed body. %w", source.err) } @@ -72,7 +72,7 @@ func (source *sourceReader) Read(buffer []byte) (int, error) { // http.ServeContent picks the status itself, so a status buffered with Status is // not used. The response is committed here and the lifecycle leaves it alone. func (call *Call) ServeContent(name string, modTime time.Time, content io.ReadSeeker) { - http.ServeContent(call.w, call.req, name, modTime, content) + http.ServeContent(&call.w, call.req, name, modTime, content) // After handlers read the buffered status, so it has to match what went out. call.status = call.w.status