Code organization

I think the organization of the internal/fe26 package could be improved. The package does three things. First, it inspect the command line and the environment variables to collect the necessary configuration. Second, it creates a router based on that configuration. Third, it starts an HTTP server using that router. I would split these responsibilities in three separate packages.

Decouple the definition of the router from the HTTP server

Coupling the definition of the router and the launch of an HTTP server based on that handler prevents you from reusing that hander for different purposes or instantiate it for testing purposes. Fixing this is quite easy. Just move the call to http.ListenAndServe from fe26Router to Fe26. Of course, you need to change fe26Router in order to return a http.Handler.

func fe26Router() http.Handler {
    r := mux.NewRouter()
    // Configure the handlers...
    return r
}

func Fe26() {
    preStartChecks()

    r := fe26Router()

    http.Handle("/", r)

    if err := http.ListenAndServe(Config.ip+":"+strconv.Itoa(Config.port), nil); err != nil {
        log.Fatal(err)
    }
}

Decouple the configuration of the routers

Instead of creating a main router on the fly in fe26Router by reading the configuration directly from a global variable, a better approach would be to pass the router configuration when the router is created. That's relatively easy for fe26Router:

func fe26Router(cfg *Settings) http.Handler {
    r := mux.NewRouter()

    // Base path: i.e GET fe26 & fe26.json
    r.HandleFunc("/"+cfg.FeBase, ListFilesHTML).Methods("GET")
    r.HandleFunc("/"+cfg.FeBase+".json", ListFilesJson).Methods("GET")

    // Handle static files like css and js files
    staticFiles := packr.New("static", "../../web/static")
    r.PathPrefix("/static.file").Handler(http.StripPrefix("/static.file", http.FileServer(staticFiles))).Methods("GET")

    // if GET to / redirect to fe26
    r.HandleFunc("/", redirectToView).Methods("GET")

    // Handle static files that exist
    r.Handle("/{filePath:.*}", http.FileServer(Fe26Dir(cfg.docRoot))).Methods("GET")

    // Handle Post requests
    //r.HandleFunc("/"+config.FeBase, handlePost).Methods("POST")
    r.HandleFunc("/"+cfg.FeBase+".json", handlePost).Methods("POST")

    return r
}

Unluckily, this change should propagate to the rest of the handlers. One handler and some of the code used by the handlers still use Config directly. This should be decoupled little by little. A very simple example is the redirect handler. You could implement it by leveraging function closures:

func newRedirectHandler(newPath string) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        log.Info("REDIRECT: from: " + r.URL.Path + "to" + newPath)
        if q := r.URL.RawQuery; q != "" {
            newPath += "?" + q
        }
        w.Header().Set("Location", newPath)
        w.WriteHeader(StatusMovedPermanently)
    })
}

You would register this new handler like this:

r.Handle("/", newRedirectHandler(cfg.FeBase)).Methods("GET")

Make steps to test the /fe62 and /fe26.json routers

These two routers do very similar things, but they change the way they present data. There are some commonalities that can be exploited. First of all, they both have the same initial logic. Maybe you can encapsulate this into its own wrapper http.Handler. For example:

func newFileDataHandler(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        d := r.URL.Query().Get("d")
        data, err := getFilesData(d)

        if err != nil {
            log.Warn("Wrong query parameter, redirecting to: " + data.RequestUrl)
            http.Redirect(w, r, Config.FeBase+"?d="+data.RequestUrl, 302)
            return
        }

        ctx := context.WithValue(r.Context(), "data", data)
        next.ServeHTTP(w, r.WithContext(ctx))
    })
}

With such a "filter" in place, you can focus about just the presentation logic in ListFilesHTML and ListFilesJson. I won't repeat everything, but ListFilesHTML should look something like that:

func ListFilesHTML(w http.ResponseWriter, r *http.Request) {
    data, ok := r.Context().Value("data").(listFileData)
    if !ok {
        panic("file data expected in the request context")
    }

    log.Info("LIST: " + data.RequestUrl)
}

When you register the handler, you need to wrap your handlers like this:

r.HandleFunc("/"+Config.FeBase, newFileDataHandler(http.HandlerFunc(ListFilesHTML))).Methods("GET")

I find that this solution has two benefits. First, it prevents you from repeating the same code in two different handlers. Second, it improves the testability of your code. With this improvement, you don't need a real filesystem to test ListFilesHTML. Instead, you can create an instance of listFileData, put it in the request context, and use the package httptest to test your handlers completely in memory. You can also go one step farther and isolate the new file data handler from file-system operations.

type fileDataReader func(string) (listFileData, error)

func newFileDataHandler(fdr fileDataReader, next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        data, err := fdr(r.URL.Query().Get("d"))

        if err != nil {
            log.Warn("Wrong query parameter, redirecting to: " + data.RequestUrl)
            http.Redirect(w, r, Config.FeBase+"?d="+data.RequestUrl, 302)
            return
        }

        ctx := context.WithValue(r.Context(), contextKeyFileData, data)
        next.ServeHTTP(w, r.WithContext(ctx))
    })
}

The registration for this handler ties all the pieces together.

r.HandleFunc("/"+Config.FeBase, newFileDataHandler(getFilesData, http.HandlerFunc(ListFilesHTML))).Methods("GET")

You can also unit test the new file data handler in isolation from the file system by creating a test implementaiton of fileDataReader to test every possible corner case.

Don't swallow errors

If a component can't handle errors, it should just bubble them up to the caller, optionally adding some additionally context. Logging an error must be a conscious choice to handle that error. I have to admit that I don't know all the internals of this project, but the choice to log errors in FileManager instead of passing them to the caller looks a bit suspicious.

For example, FileManager.ReadDir should return an error if it's not possible to read the directory, and NewFileManager should probably return that error up the chain. Eventually, an HTTP handler will have to deal with it, and I think that a handler the right place to either log it or show an error message to the user (or both).

The FileManager doesn't manage much

I think that the FileManager API could be simplified a bit. In order to use a FileManager, I have to create one by invoking NewFileManager passing both the URL and the document root. Once I have a FileManager, the only thing I can do is invoke ReadDir, which will just list the files in the directory identified by the URL provided at construction time. Moreover, the result of reading the content of a directory is stored in the fields of the FileManager itself.

In this case, I would prefer to either have a function, like this:

type DirContent struct {
    Files []File
    Dirs  []File
    Path  string
}

func ReadDir(root, url string) (*DirContent, error) {
    panic("not implemented")
}

or a type that encapsulates the context common to every call (e.g. the document root), like this:

type DirContent struct {
    Files []File
    Dirs  []File
    Path  string
}

type FileManager struct {
    root string
}

func NewFileManager(root string) *FileManager {
    return &FileManager{root}
}

func (m *FileManager) ReadDir(url string) (*DirContent, error) {
    panic("not implemented")
}

Since the document root comes from the configuration and is not supposed to change for the lifetime of the application, I would prefer the second option. With the second solution, you could create just an instance of a FileManager and reuse it in all the handlers that need it. In addition, it's easy to mock a FileManager for testing purposes and decouple functionality if you opt for the second solution.

andresbott / Fe26

Code review #2