Update routers.go

[ethomas2]

No description provided.

[ethomas2]

Minor, but you guys didn't run gofmt on the most recent commit of your code. I don't really care about gofmt, in fact I disagree with a lot of the time, but I know some folks that think it's nice for code to be consistent across the go community.
find . -iname \*go -exec gofmt -w {} \;

or for extra points add a -s
find . -iname \*go -exec gofmt -s -w {} \;

[ethomas2]

I'm not a big fan of SetRoutes() setting global state. In main.go you call http.ListenAndServe(port, nil), and it's not easy for me to figure out where the handler is coming from. I'd prefer something like

// In routers.go
func GetRoutes() http.Handler {
  mux := http.NewServeMux()
  mux.HandleFunc(paths.LoginOrSignupPath, handlers.HandleLoginOrSignupRequest)
  mux.HandleFunc(paths.LoginOrSignupPath, handlers.HandleLoginOrSignupRequest)
  mux.HandleFunc("/user", handlers.HandleUserRequest)
  mux.HandleFunc("/session", handlers.HandleSessionRequest)
  mux.HandleFunc(paths.HomePath, handlersAuthenticateOrRedirectToLogin(handlers.HandleHomeRequest))
  ...
  return mux
}

// In main.go
func main() {
  handler := routers.GetRoutes()
  http.ListenAndServe(port, handler)
}

GetRoutes might be a terrible name, but you get the idea

[ethomas2]

responseWriter is a io.Writer, which means you can do
fmt.Fprint(responseWriter, "password email combo was correct")

there's also a fmt.Fprintln and fmt.Fprintf in case you ever need that

[ethomas2]

This line is reading all the bytes from the io.Reader into memory. When possible, avoid reading all the bytes from an io.Reader into memory, instead give the io.Reader directly to whatever wants to consume it. That way the consumer can decide if it needs to read the whole byte stream, and even if it does need to read the whole byte stream it can decide if it needs all the bytes in memory at the same time or if it can stream it.

It looks like the bytes from getRequestBody are only being consumed by json.Unmarshal. I'd get rid of getRequestBody() and do the so instead you could do

func HandleUserRequest(...) {
   defer func() {
       io.CopyN(ioutil.Discard, request.Body, 512) 
       request.Body.Close()
   }()
   
   ...

   signupForm := &SignupForm{}  // alternatively signupForm := new(SignupForm)
   if err := json.NewDecoder(request.Body).Decode(signupForm); err != nil {
       ...
   }
}

[ethomas2]

Edit: Ignore everything below. Apparently as of go1.7 you don't need to drain the request.Body.
See google/go-github#847
and golang/go@18072ad

As a side note, i'm not 100% sure why you want to drain the reader before closing it. Someone told me to do it once. I've also seen this in go-github, which is a github client written in go that we use at work. I really like their code, and I copy a lot of their patterns.

I chose the number 512 because that's what go-github does. I'm not sure why they do that and tbh 512 seems pretty low. I think it probably makes more sense to do io.Copy(ioutil.Discard, request.Body), idk why go-github doesn't do that.

I think the reason you want to drain the request.Body is because the drain will wait for the guy on the other end of the tcp connection to finish sending all of his data before terminating the tcp connection. If you Close before draining you risk closing the connection before the other guy is done.

Actually, now that I write this out, that's totally why you want to drain before closing.

It's worth noting that in this example, the drain is technically unnecessary since you're always calling json.NewDecoder.Decode on request.Body which will drain it for you ... but that's an implementation detail of the json package that you shouldn't depend on.

[atmiguel]

@ethomas2 could you explain your code snippet above? I don't understand what's happening in the deferred function

[ethomas2]

It takes up to 512 bytes from the request body and sends it to the equivilent of /dev/null.

I used 512 instead of just discarding all the bytes because that's what go-github does. It probably makes more sense to discard all the bytes with io.Copy

All of that is irrelevant though. Apparently you don't have to drain the request body after go1.7

[ethomas2]

These panics are pretty bad

1. panic doesn't just kill the current goroutine, it crashes the whole program. So one user's malformed request could crash your whole server instead of just 400ing to that user. Be very wary of panics. panic should only be used in cases where the only reasonable thing to do is crash the whole service. I've only ever used panic very early in the startup of a service (e.g. failure to load the config file), or in places where something is logically impossible (e.g. in the default of a switch statement that should never occur).

2. You could defer a recover at the top of this function. That would work, but it would be non-idomatic. The idiomatic thing to do is to immediately handle the error and return, or bubble the error up to the caller (in this case there is no caller). Obvs if the error handling gets complicated or duplicated a bunch, then factor the error handling out into its own function

[ethomas2]

sql injection vulnerability here. You're probably aware of this already and just don't care (I wouldn't care if I were you, it's not like this is a production service). But in the interest of leaving a good code review, I feel like I should note this.

[ethomas2]

More sql injections later on in the file

[zingales]

I looked into this and i think sql.Query and sql.QueryRow are injection safe.

[atmiguel]

we def have a task to clean all user input

[ethomas2]

Usually when a function returns two values (foo, error), if error is non-nil, foo is the zero value. The zero value for numeric types is 0, not -1

[ethomas2]

Same for a bunch of methods in tokenutil

[zingales]

Is there a zero value function? so we could just do return int.ZeroValue, err. Cause that would be more readable.

[ethomas2]

don't think so, I think you just have to memorize them. But they're all what you'd expect. Just imagine that type in memory with every byte being zero.

[ethomas2]

ah I see, you're concerned about readability, not memorability. Still, the answer is no

[zingales]

@atmiguel how do you feel about -1 vs 0. Cause i think we specifically changed it from 0 -> -1

[ethomas2]

Probably makes more sense for this to be a time.Duration instead of an int.

const oneWeek = time.Hour * 24 * 7

You can get the number of minutes that this duration is by doing duration.Minutes().
Using time.Duration gives you the standard advantages that a time library gives you. Lets you do time arithmetic, lets you see if one time comes before or after another, better formatting when printing times ... stuff like that

[ethomas2]

When you end up using this intokenutil, you do something like

jwt.StandardClaims{
   ExpiresAt: time.Now().Add(oneWeek).Unix()
}

[ethomas2]

In general I completely agree with you guys that single letter variable names are a cancer, but I do use the signature (w http.ResponseWriter, r *http.Request) a lot. This function signature is super common. This is the rare exception where I think spelling out a descriptive variable name is less readable than using the single letter. Just because i'm so used to seeing (w http.ResponseWriter, r *http.Request), when I see that signature I immediately pattern match and think "oh, it's one of those types of functions".

[atmiguel]

Totally understand that, but i had a really hard time reading all the code i found written like that. Like damn one of the variables is r and i was always like wait is this the request or the response. I understand what you mean about getting used to things, but I often will index over i in for loops, as well as event over e in js event handlers. Just makes more sense to me

[ethomas2]

This is similar to panic in that it's gonna crash the whole program instead of returning an error status for this one request

mbp-IT002597:go ethomas$ go doc log.Fatal
func Fatal(v ...interface{})
    Fatal is equivalent to Print() followed by a call to os.Exit(1).

[ethomas2]

omg I love this function so much. A function that takes a function and returns another function. This smells so much of haskell

[ethomas2]

I think I might adopt this pattern for authenticated endpoints at work

[ethomas2]

Minor: The return value of this function is
func(http.ResponseWriter, *http.Request)
you could change it to
http.HandlerFunc, which is the same thing, but is more concise and possibly more readable (there's an argument to be made that it's no more readable. So ... whatevs)

[zingales]

error: handlers/handlers.go:202:3: http.HandleFunc is not a type

[ethomas2]

HandlerFunc, not HandleFunc