`c197c28`

S39: renderer — recursive partial walk + fail-loud on undefined template refs

Authored by

espadonne 4 days ago

SHA: c197c289ffdfa35bcf6e42319edb71ca38d2f40a
Parents: 24a480c
Tree: 4f249a2

2 changed files

Status	File	+	-
M	`internal/web/render/render.go`	93	42
A	`internal/web/render/render_test.go`	164	0

internal/web/render/render.gomodified

  	"io/fs"
  	"net/http"
  	"path"
 +	"sort"
  	"strings"
 +	"text/template/parse"
  	"time"
  	"github.com/tenseleyFlow/shithub/internal/web/middleware"
  	Octicons OcticonResolver
+ }
 -// New parses every page template under tmplFS. A "page template" is any file
 -// at the root of tmplFS that does NOT begin with an underscore. Files that
 -// begin with an underscore (e.g. "_layout.html") are partials, parsed once
 -// into every page.
 +// New parses every page template under tmplFS.
 +//
 +// Naming contract — read this before adding files to internal/web/templates/:
 +//
 +//   - **Pages** are .html files whose basename does NOT begin with an
 +//     underscore. A page at `repo/tree.html` is registered under the
 +//     lookup name `repo/tree`. Render that name from a handler.
 +//   - **Partials** are .html files whose basename begins with an
 +//     underscore (`_layout.html`, `profile/_tabs.html`). Partials are
 +//     parsed into *every* page so the `{{ define "name" }}` blocks
 +//     they declare are resolvable from any page template.
 +//
 +// Both pages and partials are picked up recursively. Earlier versions
 +// of this loader walked only the root for partials, which caused a
 +// page that referenced a subdir partial (`profile/_tabs.html`'s
 +// `{{ define "tabs" }}`) to render blank — html/template silently
 +// ignored the missing-template ref at exec time. We now also validate
 +// that every `{{ template "name" }}` action in every parsed page
 +// resolves; an undefined ref fails loud at startup with the offending
 +// page + the missing name.
  func New(tmplFS fs.FS, opts Options) (*Renderer, error) {
 -	entries, err := fs.ReadDir(tmplFS, ".")
 -	if err != nil {
 -		return nil, fmt.Errorf("read template root: %w", err)
 -	}
+-
  	var (
 -		partialNames []string
 -		pageNames    []string
 -		errorPages   []string
 +		partialPaths []string
 +		pagePaths    []string
+ 	)
 -	for _, e := range entries {
 -		if e.IsDir() {
 -			continue
 -		}
 -		name := e.Name()
 -		if !strings.HasSuffix(name, ".html") {
 -			continue
 -		}
 -		if strings.HasPrefix(name, "_") {
 -			partialNames = append(partialNames, name)
 -		} else {
 -			pageNames = append(pageNames, name)
 -		}
 -	}
+-
 -	// Recursively pick up files in subdirectories like errors/.
 -	// Each subdirectory file is registered as `<dir>/<name>` (without
 -	// suffix) for Render lookups.
  	if err := fs.WalkDir(tmplFS, ".", func(p string, d fs.DirEntry, walkErr error) error {
  		if walkErr != nil {
  			return walkErr
  		if d.IsDir() || !strings.HasSuffix(p, ".html") {
  			return nil
+ 		}
 -		if !strings.Contains(p, "/") {
 -			return nil
 +		if strings.HasPrefix(path.Base(p), "_") {
 +			partialPaths = append(partialPaths, p)
 +		} else {
 +			pagePaths = append(pagePaths, p)
+ 		}
 -		errorPages = append(errorPages, p)
  		return nil
  	}); err != nil {
  		return nil, fmt.Errorf("walk templates: %w", err)
+ 	}
 +	sort.Strings(partialPaths)
 +	sort.Strings(pagePaths)
  	r := &Renderer{
 -		pages:   make(map[string]*template.Template, len(pageNames)+len(errorPages)),
 +		pages:   make(map[string]*template.Template, len(pagePaths)),
  		octicon: opts.Octicons,
+ 	}
 -	parse := func(displayName string, primary string) error {
 +	parsePage := func(displayName, primary string) error {
  		t := template.New(path.Base(primary)).Funcs(funcMap(r.octicon))
 -		all := append([]string{}, partialNames...)
 +		all := append([]string{}, partialPaths...)
  		all = append(all, primary)
  		parsed, err := t.ParseFS(tmplFS, all...)
  		if err != nil {
  			return fmt.Errorf("parse %s: %w", displayName, err)
+ 		}
 +		if missing := undefinedTemplateRefs(parsed); len(missing) > 0 {
 +			return fmt.Errorf("page %q references undefined template(s): %s", displayName, strings.Join(missing, ", "))
 +		}
  		r.pages[displayName] = parsed
  		return nil
+ 	}
 -	for _, page := range pageNames {
 -		if err := parse(strings.TrimSuffix(page, ".html"), page); err != nil {
 +	for _, page := range pagePaths {
 +		if err := parsePage(strings.TrimSuffix(page, ".html"), page); err != nil {
  			return nil, err
+ 		}
+ 	}
 -	for _, page := range errorPages {
 -		if err := parse(strings.TrimSuffix(page, ".html"), page); err != nil {
 -			return nil, err
 +	return r, nil
 +}
++
 +// undefinedTemplateRefs returns the names of every `{{ template "name" }}`
 +// action in any parsed sub-template that does not resolve to a defined
 +// template within `t`. Empty slice means every reference is satisfied.
 +//
 +// The standard library does not validate this at parse time — html/template
 +// happily parses a page with a dangling `{{ template "missing" }}` and
 +// silently emits nothing at exec time. This helper closes that hole.
 +func undefinedTemplateRefs(t *template.Template) []string {
 +	defined := map[string]bool{}
 +	for _, child := range t.Templates() {
 +		defined[child.Name()] = true
 +	}
 +	seen := map[string]bool{}
 +	var missing []string
 +	for _, child := range t.Templates() {
 +		if child.Tree == nil {
 +			continue
+ 		}
 +		walkTemplateRefs(child.Tree.Root, func(name string) {
 +			if defined[name] || seen[name] {
 +				return
 +			}
 +			seen[name] = true
 +			missing = append(missing, name)
 +		})
 +	}
 +	sort.Strings(missing)
 +	return missing
 +}
++
 +func walkTemplateRefs(n parse.Node, visit func(name string)) {
 +	if n == nil {
 +		return
 +	}
 +	switch x := n.(type) {
 +	case *parse.ListNode:
 +		if x == nil {
 +			return
 +		}
 +		for _, c := range x.Nodes {
 +			walkTemplateRefs(c, visit)
 +		}
 +	case *parse.IfNode:
 +		walkTemplateRefs(x.List, visit)
 +		walkTemplateRefs(x.ElseList, visit)
 +	case *parse.RangeNode:
 +		walkTemplateRefs(x.List, visit)
 +		walkTemplateRefs(x.ElseList, visit)
 +	case *parse.WithNode:
 +		walkTemplateRefs(x.List, visit)
 +		walkTemplateRefs(x.ElseList, visit)
 +	case *parse.TemplateNode:
 +		visit(x.Name)
+ 	}
 -	return r, nil
+ }
  // Render writes the named page to w using data as the template root context.

internal/web/render/render_test.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package render
++
 +import (
 +	"bytes"
 +	"net/http/httptest"
 +	"strings"
 +	"testing"
 +	"testing/fstest"
 +)
++
 +// Each test builds a tiny in-memory template tree to exercise one
 +// invariant of New(). Keep these focused; broader render flows are
 +// covered by handler-level tests.
++
 +func TestNew_RegistersRootPages(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html": &fstest.MapFile{Data: []byte(`{{ define "layout" }}<html>{{ template "body" . }}</html>{{ end }}`)},
 +		"home.html":    &fstest.MapFile{Data: []byte(`{{ define "body" }}home page{{ end }}`)},
 +	}
 +	r, err := New(fsys, Options{})
 +	if err != nil {
 +		t.Fatalf("New: %v", err)
 +	}
 +	var buf bytes.Buffer
 +	if err := r.Render(&buf, "home", nil); err != nil {
 +		t.Fatalf("render: %v", err)
 +	}
 +	if !strings.Contains(buf.String(), "home page") {
 +		t.Errorf("rendered output missing body: %q", buf.String())
 +	}
 +}
++
 +func TestNew_RegistersSubdirPages(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html":    &fstest.MapFile{Data: []byte(`{{ define "layout" }}<html>{{ template "body" . }}</html>{{ end }}`)},
 +		"errors/404.html": &fstest.MapFile{Data: []byte(`{{ define "body" }}not found{{ end }}`)},
 +	}
 +	r, err := New(fsys, Options{})
 +	if err != nil {
 +		t.Fatalf("New: %v", err)
 +	}
 +	var buf bytes.Buffer
 +	if err := r.Render(&buf, "errors/404", nil); err != nil {
 +		t.Fatalf("render: %v", err)
 +	}
 +	if !strings.Contains(buf.String(), "not found") {
 +		t.Errorf("rendered output missing body: %q", buf.String())
 +	}
 +}
++
 +// Regression test for the inbound deferral from S30 dogfood: a partial
 +// at `profile/_tabs.html` that defines `{{ define "tabs" }}` was
 +// silently registered as an unparsed page. A page that called
 +// `{{ template "tabs" . }}` then rendered blank.
 +func TestNew_LoadsSubdirPartials(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "layout" }}<html>{{ template "body" . }}</html>{{ end }}`,
 +		)},
 +		"profile/_tabs.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "tabs" }}TAB CONTENT{{ end }}`,
 +		)},
 +		"profile.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "body" }}{{ template "tabs" . }}{{ end }}`,
 +		)},
 +	}
 +	r, err := New(fsys, Options{})
 +	if err != nil {
 +		t.Fatalf("New: %v", err)
 +	}
 +	var buf bytes.Buffer
 +	if err := r.Render(&buf, "profile", nil); err != nil {
 +		t.Fatalf("render: %v", err)
 +	}
 +	if !strings.Contains(buf.String(), "TAB CONTENT") {
 +		t.Errorf("subdir partial not loaded — body was %q", buf.String())
 +	}
 +}
++
 +// A page that references a template name nothing defines should fail
 +// LOUDLY at New() time, not silently render blank at exec time.
 +func TestNew_FailsOnUndefinedTemplateRef(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "layout" }}<html>{{ template "body" . }}</html>{{ end }}`,
 +		)},
 +		"broken.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "body" }}{{ template "does-not-exist" . }}{{ end }}`,
 +		)},
 +	}
 +	_, err := New(fsys, Options{})
 +	if err == nil {
 +		t.Fatal("New: expected error for undefined template ref, got nil")
 +	}
 +	if !strings.Contains(err.Error(), "does-not-exist") {
 +		t.Errorf("error should name the missing template; got %v", err)
 +	}
 +	if !strings.Contains(err.Error(), "broken") {
 +		t.Errorf("error should name the offending page; got %v", err)
 +	}
 +}
++
 +// Sanity: refs into partials still resolve (not flagged as undefined).
 +func TestNew_AcceptsRefsResolvedByPartials(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "layout" }}{{ template "header" . }}{{ template "body" . }}{{ end }}`,
 +		)},
 +		"_header.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "header" }}HDR{{ end }}`,
 +		)},
 +		"page.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "body" }}body{{ end }}`,
 +		)},
 +	}
 +	r, err := New(fsys, Options{})
 +	if err != nil {
 +		t.Fatalf("New: %v", err)
 +	}
 +	var buf bytes.Buffer
 +	if err := r.Render(&buf, "page", nil); err != nil {
 +		t.Fatalf("render: %v", err)
 +	}
 +	got := buf.String()
 +	if !strings.Contains(got, "HDR") || !strings.Contains(got, "body") {
 +		t.Errorf("missing partial or page output: %q", got)
 +	}
 +}
++
 +// RenderPage preserves any Viewer / CSRFToken the handler set itself,
 +// rather than overwriting them. The auto-inject is for handlers that
 +// hand RenderPage an empty map; explicit handlers stay in control.
 +func TestRenderPage_PreservesExplicitViewer(t *testing.T) {
 +	t.Parallel()
 +	fsys := fstest.MapFS{
 +		"_layout.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "layout" }}{{ template "body" . }}{{ end }}`,
 +		)},
 +		"page.html": &fstest.MapFile{Data: []byte(
 +			`{{ define "body" }}viewer={{ .Viewer }}{{ end }}`,
 +		)},
 +	}
 +	r, err := New(fsys, Options{})
 +	if err != nil {
 +		t.Fatalf("New: %v", err)
 +	}
 +	req := httptest.NewRequest("GET", "/", nil)
 +	rw := httptest.NewRecorder()
 +	if err := r.RenderPage(rw, req, "page", map[string]any{
 +		"Viewer": "explicit",
 +	}); err != nil {
 +		t.Fatalf("render: %v", err)
 +	}
 +	if !strings.Contains(rw.Body.String(), "viewer=explicit") {
 +		t.Errorf("explicit Viewer was overwritten; body=%q", rw.Body.String())
 +	}
 +}