Skip to content
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
dd6d315
Add in-memory fs example
bryphe-coder Feb 5, 2022
33897e5
Integrate test server too
bryphe-coder Feb 5, 2022
b8d092c
Some initial test cases for files at root
bryphe-coder Feb 5, 2022
9949261
Serve contents from filesystem
bryphe-coder Feb 5, 2022
fadc0b6
Factor router out
bryphe-coder Feb 5, 2022
771f39d
Add recursive router building
bryphe-coder Feb 5, 2022
78406b1
Hook up recursive case
bryphe-coder Feb 5, 2022
50c72ba
Refactor to handle different file-serving cases
bryphe-coder Feb 5, 2022
7a98a48
Handle case w/o html extension
bryphe-coder Feb 5, 2022
1eb2402
Handle redirecting to index.html at root
bryphe-coder Feb 5, 2022
b0fd9a8
Add test case for nested path
bryphe-coder Feb 5, 2022
a506148
Remove now-unnecessary smoke test
bryphe-coder Feb 5, 2022
b59e2a8
Some clean up
bryphe-coder Feb 5, 2022
ff75695
Refactor to use http.ServeContent
bryphe-coder Feb 5, 2022
a410396
Simplify router construction
bryphe-coder Feb 5, 2022
15c38b9
rtr -> router
bryphe-coder Feb 5, 2022
c813b5e
Handle serving non-html files
bryphe-coder Feb 5, 2022
317c040
Handle trailing-slash case
bryphe-coder Feb 5, 2022
133e452
Implement dynamic routing
bryphe-coder Feb 5, 2022
6d8f412
Add test case to verify static paths are preferred over dynamic paths
bryphe-coder Feb 5, 2022
a3ecd7f
Handle dynamic routing for folders
bryphe-coder Feb 5, 2022
700c9ef
Handle catch-all routes
bryphe-coder Feb 5, 2022
05a369e
Start plumbing in a way to inject template parameters
bryphe-coder Feb 5, 2022
03a20c1
Start adding plumbing for templates
bryphe-coder Feb 5, 2022
199d468
Add template functionality
bryphe-coder Feb 5, 2022
ca515e4
Set up 404 handling
bryphe-coder Feb 5, 2022
6072bad
Use nextrouter package in site, which simplifies it quite a bit
bryphe-coder Feb 5, 2022
826eaf5
Update interface to take logger
bryphe-coder Feb 5, 2022
dac34f5
Additional comments
bryphe-coder Feb 5, 2022
92d4eb0
Fix embed_test
bryphe-coder Feb 5, 2022
5262b7f
Remove now-unused noop template func
bryphe-coder Feb 5, 2022
33342e5
Merge main
bryphe-coder Feb 8, 2022
750bec9
Switch route addition from info -> debug log level
bryphe-coder Feb 8, 2022
c15b046
Invert logic to remove layer of indentation
bryphe-coder Feb 8, 2022
c17fc15
Rename buildRoutes -> registerRoutes
bryphe-coder Feb 8, 2022
34e4f46
TemplateDataFunc -> HTMLTemplateHandler
bryphe-coder Feb 8, 2022
0743503
Add return to reduce indentation in serveFile
bryphe-coder Feb 8, 2022
4585c13
Remove call to remove file extension
bryphe-coder Feb 8, 2022
8f2b47d
Switch to register 404, return error and warn at toplevel
bryphe-coder Feb 8, 2022
19fcfac
Move ./nextrouter -> ./site/nextrouter
bryphe-coder Feb 8, 2022
8343923
Return error from handler
bryphe-coder Feb 8, 2022
4a5c353
Change dynamic route logging from Info -> Debug
bryphe-coder Feb 8, 2022
e4d031f
Merge main
bryphe-coder Feb 9, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func New(options *Options) http.Handler {
r.Get("/serve", api.provisionerDaemonsServe)
})
})
r.NotFound(site.Handler().ServeHTTP)
r.NotFound(site.Handler(options.Logger).ServeHTTP)
return r
}

Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ require (
github.com/pion/udp v0.1.1 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef // indirect
github.com/sirupsen/logrus v1.8.1 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,8 @@ github.com/prometheus/procfs v0.1.3/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4O
github.com/prometheus/procfs v0.2.0/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4OA4YeYWdaU=
github.com/prometheus/procfs v0.6.0/go.mod h1:cz+aTbrPOrUb4q7XlbU9ygM+/jj0fzG6c1xBZuNvfVA=
github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef h1:NKxTG6GVGbfMXc2mIk+KphcH6hagbVXhcFkbTgYleTI=
github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef/go.mod h1:tcaRap0jS3eifrEEllL6ZMd9dg8IlDpi2S1oARrQ+NI=
github.com/remyoudompheng/bigfft v0.0.0-20190728182440-6a916e37a237/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=
github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=
github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
Expand Down
225 changes: 225 additions & 0 deletions nextrouter/nextrouter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
package nextrouter

import (
"bytes"
"context"
"html/template"
"io/fs"
"net/http"
"path/filepath"
"strings"
"time"

"github.com/go-chi/chi/v5"

"cdr.dev/slog"
)

// Options for configuring a nextrouter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this to be confused with our primary app routing logic.

For that reason, what do you think about this being in site like it was before? It could even be at the root if you feel that's clean enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'd prefer to keep it in its own package so it's clear that it is stand-alone - how about I move it to site/nextrouter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in 19fcfac

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

type Options struct {
Logger slog.Logger
TemplateDataFunc TemplateDataFunc
}

// TemplateDataFunc is a function that lets the consumer of `nextrouter`
// inject arbitrary template parameters, based on the request. This is useful
// if the Request object is carrying CSRF tokens, session tokens, etc -
// they can be emitted in the page.
type TemplateDataFunc func(*http.Request) interface{}

// Handler returns an HTTP handler for serving a next-based static site
// This handler respects NextJS-based routing rules:
// https://nextjs.org/docs/routing/dynamic-routes
//
// 1) If a file is of the form `[org]`, it's a dynamic route for a single-parameter
// 2) If a file is of the form `[[...any]]`, it's a dynamic route for any parameters
func Handler(fileSystem fs.FS, options *Options) http.Handler {
if options == nil {
options = &Options{
Logger: slog.Logger{},
TemplateDataFunc: nil,
}
}
router := chi.NewRouter()

// Build up a router that matches NextJS routing rules, for HTML files
buildRouter(router, fileSystem, *options)

// Fallback to static file server for non-HTML files
// Non-HTML files don't have special routing rules, so we can just leverage
// the built-in http.FileServer for it.
fileHandler := http.FileServer(http.FS(fileSystem))
router.NotFound(fileHandler.ServeHTTP)

// Finally, if there is a 404.html available, serve that
serve404IfAvailable(fileSystem, router, *options)

return router
}

// buildRouter recursively traverses the file-system, building routes
// as appropriate for respecting NextJS dynamic rules.
func buildRouter(rtr chi.Router, fileSystem fs.FS, options Options) {
files, err := fs.ReadDir(fileSystem, ".")
if err != nil {
options.Logger.Warn(context.Background(), "Provided filesystem is empty; unable to build routes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a warn? I'm not sure empty directories would be of much concern. Maybe change to debug here.

return
}

// Loop through everything in the current directory...
for _, file := range files {
name := file.Name()

// ...if it's a directory, create a sub-route by
// recursively calling `buildRouter`
if file.IsDir() {
sub, err := fs.Sub(fileSystem, name)
if err != nil {
options.Logger.Error(context.Background(), "Unable to call fs.Sub on directory", slog.F("directory_name", name))
continue
}

// In the special case where the folder is dynamic,
// like `[org]`, we can convert to a chi-style dynamic route
// (which uses `{` instead of `[`)
routeName := name
if isDynamicRoute(name) {
routeName = "{dynamic}"
}

options.Logger.Info(context.Background(), "Adding route", slog.F("name", name), slog.F("routeName", routeName))
rtr.Route("/"+routeName, func(r chi.Router) {
buildRouter(r, sub, options)
})
} else {
// ...otherwise, if it's a file - serve it up!
serveFile(rtr, fileSystem, name, options)
}
}
}

// serveFile is responsible for serving up HTML files in our next router
// It handles various special cases, like trailing-slashes or handling routes w/o the .html suffix.
func serveFile(router chi.Router, fileSystem fs.FS, fileName string, options Options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this to serveHTMLFile, or something of the sort.

It might be helpful to move the .html check to the router-building function.

// We only handle .html files for now
ext := filepath.Ext(fileName)
if ext != ".html" {
return
}

options.Logger.Debug(context.Background(), "Reading file", slog.F("fileName", fileName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a linting rule for this, but all system errors we've used are generally lowercase.


data, err := fs.ReadFile(fileSystem, fileName)
if err != nil {
options.Logger.Error(context.Background(), "Unable to read file", slog.F("fileName", fileName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a fatal error too!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AKA maybe this function should return errors too.

return
}

// Create a template from the data - we can inject custom parameters like CSRF here
tpls, err := template.New(fileName).Parse(string(data))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is precompiled in memory 👍

if err != nil {
options.Logger.Error(context.Background(), "Unable to create template for file", slog.F("fileName", fileName))
return
}

handler := func(writer http.ResponseWriter, request *http.Request) {
var buf bytes.Buffer

// See if there are any template parameters we need to inject!
// Things like CSRF tokens, etc...
//templateData := struct{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we structure this data for the user? I'm not sure we need to allow arbitrary injection right now.

Having a supported set of exposed injectable values seems reasonable to me!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can still be dynamic per-request, but let's have something structured instead of interface{}.

var templateData interface{}
templateData = nil
if options.TemplateDataFunc != nil {
templateData = options.TemplateDataFunc(request)
}

options.Logger.Debug(context.Background(), "Applying template parameters", slog.F("fileName", fileName), slog.F("templateData", templateData))
err := tpls.ExecuteTemplate(&buf, fileName, templateData)

if err != nil {
options.Logger.Error(request.Context(), "Error executing template", slog.F("template_parameters", templateData))
http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}

http.ServeContent(writer, request, fileName, time.Time{}, bytes.NewReader(buf.Bytes()))
}

fileNameWithoutExtension := removeFileExtension(fileName)

// Handle the `[[...any]]` catch-all case
if isCatchAllRoute(fileNameWithoutExtension) {
options.Logger.Info(context.Background(), "Registering catch-all route", slog.F("fileName", fileName))
router.NotFound(handler)
return
}

// Handle the `[org]` dynamic route case
if isDynamicRoute(fileNameWithoutExtension) {
options.Logger.Info(context.Background(), "Registering dynamic route", slog.F("fileName", fileName))
router.Get("/{dynamic}", handler)
return
}

// Handle the basic file cases
// Directly accessing a file, ie `/providers.html`
router.Get("/"+fileName, handler)
// Accessing a file without an extension, ie `/providers`
router.Get("/"+fileNameWithoutExtension, handler)

// Special case: '/' should serve index.html
if fileName == "index.html" {
router.Get("/", handler)
} else {
// Otherwise, handling the trailing slash case -
// for examples, `providers.html` should serve `/providers/`
router.Get("/"+fileNameWithoutExtension+"/", handler)
}
}

func serve404IfAvailable(fileSystem fs.FS, router chi.Router, options Options) {
// Get the file contents
fileBytes, err := fs.ReadFile(fileSystem, "404.html")
if err != nil {
// An error is expected if the file doesn't exist
return
}

router.NotFound(func(writer http.ResponseWriter, request *http.Request) {
writer.WriteHeader(http.StatusNotFound)
_, err = writer.Write(fileBytes)
if err != nil {
options.Logger.Error(request.Context(), "Unable to write bytes for 404")
return
}
})
}

// isDynamicRoute returns true if the file is a NextJS dynamic route, like `[orgs]`
// Returns false if the file is not a dynamic route, or if it is a catch-all route (`[[...any]]`)
func isDynamicRoute(fileName string) bool {
fileWithoutExtension := removeFileExtension(fileName)

// Assuming ASCII encoding - `len` in go works on bytes
byteLen := len(fileWithoutExtension)
if byteLen < 2 {
return false
}

return fileWithoutExtension[0] == '[' && fileWithoutExtension[1] != '[' && fileWithoutExtension[byteLen-1] == ']'
}

// isCatchAllRoute returns true if the file is a catch-all route, like `[[...any]]`
// Return false otherwise
func isCatchAllRoute(fileName string) bool {
fileWithoutExtension := removeFileExtension(fileName)
ret := strings.HasPrefix(fileWithoutExtension, "[[.")
return ret
}

// removeFileExtension removes the extension from a file
// For example, removeFileExtension("index.html") would return "index"
func removeFileExtension(fileName string) string {
return strings.TrimSuffix(fileName, filepath.Ext(fileName))
}
Loading