Fachschaft/sprechstunden-go
7
1
Fork 0
Code Issues 8 Pull requests Releases Activity

preparations for styling: refactor template rendering #1

Merged
Gonne merged 7 commits from styling into main 2022-09-26 10:46:05 +00:00
Conversation 2 Commits 7 Files changed 36 +18 -5
1 changed files with 18 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit a755a0b3e9 - Show all commits

23
controllers/templates.go
View file

@ -3,6 +3,7 @@ package controllers
import ( import (
"fmt" "fmt"
"html/template" "html/template"
"log"
"net/http" "net/http"
"officeHours/models" "officeHours/models"
"os" "os"
@ -14,33 +15,45 @@ var funcs = template.FuncMap{
} }
var baseTemplate = template.Must(template.ParseFiles("templates/base.html")).New("base.html").Funcs(funcs) var baseTemplate = template.Must(template.ParseFiles("templates/base.html")).New("base.html").Funcs(funcs)
// Execute a rendered full HTML page.
//
// If you just want to fill a template snippet, use the RawTemplates object.
//
// Parameters:
// - name is the name of the template file inside templates/, without .html suffix.
// - data is passed through to the template
func (b *BaseHandler) serveTemplate(w http.ResponseWriter, name string, data any) { func (b *BaseHandler) serveTemplate(w http.ResponseWriter, name string, data any) {
full_name := "templates/" + name + ".html" full_name := "templates/" + name + ".html"
Gonne commented 2022-09-21 20:56:41 +00:00
Outdated
Review
Copy link

Here we should consider checking whether the name is restricted to something like [a-zA-Z0-9] to prevent directory traversal. Possibly it is sufficient to forbid / and/or .

Here we should consider checking whether the name is restricted to something like `[a-zA-Z0-9]` to prevent directory traversal. Possibly it is sufficient to forbid `/` and/or `.`
johannes commented 2022-09-24 21:44:42 +00:00
Outdated
Review
Copy link

This is not longer relevant, since all available templates are listed in one place inside our code and we do not parse filenames from anywhere else.

This is not longer relevant, since all available templates are listed in one place inside our code and we do not parse filenames from anywhere else.
// check that template exists // check that template exists
info, err := os.Stat(full_name) info, err := os.Stat(full_name)
if (err != nil && os.IsNotExist(err)) || info.IsDir() { if (err != nil && os.IsNotExist(err)) || info.IsDir() {
errMsg := fmt.Sprintf("Template %s nicht gefunden", full_name)
johannes marked this conversation as resolved Outdated
Gonne commented 2022-09-21 21:02:46 +00:00
Outdated
Review
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent.
log.Println(errMsg)
w.WriteHeader(http.StatusNotFound) w.WriteHeader(http.StatusNotFound)
johannes marked this conversation as resolved Outdated
Gonne commented 2022-09-21 21:03:53 +00:00
Outdated
Review
Copy link

I think, that StatusNotFound is for user supplied references (e.g. a course or a room id), but this should be an internal server error.

I think, that StatusNotFound is for user supplied references (e.g. a course or a room id), but this should be an internal server error.
w.Write([]byte("404: Template nicht gefunden.")) w.Write([]byte(errMsg))
return return
} }
tmpl, err := baseTemplate.ParseFiles(full_name) tmpl, err := baseTemplate.ParseFiles(full_name)
Gonne commented 2022-09-21 21:08:17 +00:00
Outdated
Review
Copy link

Using this construction each template is parsed again and again, while it would be sufficient to parse them all on startup and later just execute them.

Using this construction each template is parsed again and again, while it would be sufficient to parse them all on startup and later just execute them.
johannes commented 2022-09-24 21:46:18 +00:00
Outdated
Review
Copy link

Fixed (and spent too much time implementing this ^^)

Fixed (and spent too much time implementing this ^^)
if err != nil { if err != nil {
// TODO: log.Printf errMsg := fmt.Sprintf("Template %s konnte nicht geparst werden : %s", full_name, err.Error())
Gonne commented 2022-09-21 21:05:23 +00:00
Outdated
Review
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

Furthermore error wrapping is something cool I just learnt.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent. Furthermore [error wrapping](https://blog.boot.dev/golang/wrapping-errors-in-go-how-to-handle-nested-errors/) is something cool I just learnt.
johannes commented 2022-09-24 21:51:40 +00:00
Outdated
Review
Copy link

Error wrapping looks nice. I started doing that.

In general it seems best to wrap errors which occur during runtime from the beginning of a http request handler, which then logs the error and returns an error page to the user. (I added an inline TODO about that)

Errors on startup however can just abort the whole program, like I implemented in main.go

Error wrapping looks nice. I started doing that. In general it seems best to wrap errors which occur during runtime from the beginning of a http request handler, which then logs the error and returns an error page to the user. (I added an inline TODO about that) Errors on startup however can just abort the whole program, like I implemented in `main.go`
log.Println(errMsg)
w.WriteHeader(http.StatusInternalServerError) w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(fmt.Sprintf("500: Template "+full_name+" konnte nicht geparst werden : %s", err.Error()))) w.Write([]byte(errMsg))
return return
} }
// TODO: cache templates in parsed state, but not executed // TODO: cache templates in parsed state, but not executed
err = template.Must(tmpl.Clone()).Execute(w, data) err = template.Must(tmpl.Clone()).Execute(w, data)
if err != nil { if err != nil {
// TODO: log.Printf errMsg := fmt.Sprintf("Template %s konnte nicht ausgeführt werden : %s", full_name, err.Error())
johannes marked this conversation as resolved Outdated
Gonne commented 2022-09-21 21:05:57 +00:00
Outdated
Review
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

Furthermore error wrapping is something cool I just learnt.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent. Furthermore [error wrapping](https://blog.boot.dev/golang/wrapping-errors-in-go-how-to-handle-nested-errors/) is something cool I just learnt.
log.Println(errMsg)
w.WriteHeader(http.StatusInternalServerError) w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(fmt.Sprintf("500: Template "+full_name+" konnte nicht ausgeführt werden : %s", err.Error()))) w.Write([]byte(errMsg))
return return
} }
} }
// standalone templates that should not be wrapped into the base.html
var RawTemplates = template.Must(template.New("").Funcs(funcs).ParseFiles( var RawTemplates = template.Must(template.New("").Funcs(funcs).ParseFiles(
"templates/confirmationMail", "templates/confirmationMail",
"templates/td.html", "templates/td.html",