diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index 8d65e54..ac9fee3 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -1,6 +1,19 @@ package handlers -import "net/http" +import ( + "html/template" + "net/http" +) + +// AddTemplateForTest registers a template under a page name so that +// the handlers_test package can drive the render path with a +// template of its own. +func (s *Handlers) AddTemplateForTest( + pageTemplate string, + tmpl *template.Template, +) { + s.templates[pageTemplate] = tmpl +} // RenderTemplateForTest exposes renderTemplate for use in the // handlers_test package. diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index ce99774..e26d788 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -3,6 +3,7 @@ package handlers import ( + "bytes" "context" "encoding/json" "errors" @@ -224,13 +225,20 @@ func (s *Handlers) renderTemplate( s.executeTemplate(w, tmpl, wrapper) } -// executeTemplate runs the template and handles errors. +// executeTemplate renders the template into a buffer and writes to +// the response only once rendering has fully succeeded. Executing +// straight into the ResponseWriter commits a partial body and a 200 +// status before a mid-render error can be reported, leaving no way +// to serve a 500. These pages are small, so holding one in memory is +// the right trade. func (s *Handlers) executeTemplate( w http.ResponseWriter, tmpl *template.Template, data any, ) { - err := tmpl.Execute(w, data) + var buf bytes.Buffer + + err := tmpl.Execute(&buf, data) if err != nil { s.log.Error( "failed to execute template", "error", err, @@ -239,5 +247,16 @@ func (s *Handlers) executeTemplate( w, "Internal server error", http.StatusInternalServerError, ) + + return + } + + w.Header().Set("Content-Type", "text/html; charset=utf-8") + + _, err = buf.WriteTo(w) + if err != nil { + s.log.Error( + "failed to write rendered page", "error", err, + ) } } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index ad927c5..3390fe0 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -2,6 +2,8 @@ package handlers_test import ( "context" + "errors" + "html/template" "net/http" "net/http/httptest" "sync" @@ -220,6 +222,68 @@ func TestRenderTemplate(t *testing.T) { ) } +// errMidRender is the failure a test template raises partway through +// rendering. +var errMidRender = errors.New("deliberate mid-render failure") + +// midRenderFailure is template data whose first method renders and +// whose second fails, so the template aborts after output has +// already been produced. +type midRenderFailure struct{} + +// Prefix is the output a streaming renderer would flush before the +// failure below aborts the template. +func (midRenderFailure) Prefix() string { return partialPageMarker } + +// Boom aborts template execution. +func (midRenderFailure) Boom() (string, error) { + return "", errMidRender +} + +// partialPageMarker is content the failing template emits before it +// aborts. +const partialPageMarker = "PARTIAL PAGE CONTENT" + +// TestRenderTemplateMidRenderErrorSendsNoPartialBody proves the +// renderer does not commit output it cannot finish: a template that +// fails partway through must yield a 500 and a body carrying none of +// the content emitted before the failure. Against a renderer that +// executes straight into the ResponseWriter this fails on both +// counts, returning 200 with the prefix already flushed. +func TestRenderTemplateMidRenderErrorSendsNoPartialBody(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + app := newTestApp(t, &h) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + h.AddTemplateForTest("failing.html", template.Must( + template.New("failing").Parse( + `{{.Data.Prefix}}{{.Data.Boom}}TAIL`, + ), + )) + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil) + w := httptest.NewRecorder() + + h.RenderTemplateForTest( + w, req, "failing.html", midRenderFailure{}, + ) + + assert.Equal( + t, http.StatusInternalServerError, w.Code, + "a failed render must report a 500", + ) + assert.Equal( + t, "Internal server error\n", w.Body.String(), + "the response must carry no part of the aborted page", + ) +} + func TestBuildDatabaseTargetConfig_Valid(t *testing.T) { t.Parallel()