patterngoMinor
Slack App OAuth flow in golang
Viewed 0 times
flowappoauthgolangslack
Problem
I'm working on a Slack app that uses Slash Commands. I've been looking for an excuse to learn Go, but the online tour was a little slow paced so I tried jumping in with this project.
I'm looking for advice on everything I can do to make this code as idiomatic as possible (that includes using libraries that might make some of what I'm doing simpler).
I'm also having some trouble figuring out how to split this into separate source files "the go way" since the project is going to get a bit larger than this.
There are 3 endpoints exposed:
"/": Returns "Hello World"
"/setColor": Called by Slack when a user uses the command
"/setColor", currently just returns "Hello World"
"/authenticateSlackTeam": Callback given to Slack to kickoff the OAuth flow, generates token and stores in database
}
func sayHelloWorld(writer http.ResponseWriter, request *http.Request) {
log.Print("Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}
func setColor(writer http.ResponseWriter, request *http.Request) {
log.Print("Set Color Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}
func generateOAuthRequest(code string) (request *http.Request, err error) {
params := slackOauthR
I'm looking for advice on everything I can do to make this code as idiomatic as possible (that includes using libraries that might make some of what I'm doing simpler).
I'm also having some trouble figuring out how to split this into separate source files "the go way" since the project is going to get a bit larger than this.
There are 3 endpoints exposed:
"/": Returns "Hello World"
"/setColor": Called by Slack when a user uses the command
"/setColor", currently just returns "Hello World"
"/authenticateSlackTeam": Callback given to Slack to kickoff the OAuth flow, generates token and stores in database
package main
import (
"encoding/json"
"fmt"
"log"
http "net/http"
"os"
"time"
"github.com/dghubble/sling"
"github.com/jinzhu/gorm"
_ "github.com/jinzhu/gorm/dialects/postgres"
)
//Team exported for Gorm?
type team struct {
ID string gorm:"primary_key"
Name string
Scope string json:"scope"
Token string
}
type slackOauthRequestParams struct {
ClientID string url:"client_id,omitempty"
ClientSecret string url:"client_secret,omitempty"
Code string url:"code,omitempty"
}
type slackOauthRequestResponse struct {
AccessToken string json:"access_token"
Scope string json:"scope"
TeamName string json:"team_name"
TeamID string json:"team_id"`}
func sayHelloWorld(writer http.ResponseWriter, request *http.Request) {
log.Print("Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}
func setColor(writer http.ResponseWriter, request *http.Request) {
log.Print("Set Color Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}
func generateOAuthRequest(code string) (request *http.Request, err error) {
params := slackOauthR
Solution
Well, when you decide to something as a learning exercise it's normal to tackle just one problem at a time. In your program you have databases, object-relational mapping, JSON encoding, OAuth protocols, closures, REST, and more. It almost makes me a bit sad not to see slices, channels, and go-routines in there. What happened to the good-old FizzBuzz?
Your ambition here is admirable, and I understand the motivation to accomplish something meaningful (a cool slack-bot is awesome), but I worry that you'll maybe miss some details in your haste.
General
In general, your code looks like it has been through the wringer a few times, and it has wrinkles, and a few worn spots. While it looks like you are already in the habit of running your code through the
-
there's no need to rename the import
-
the comments on
-
you have mis-uses of
The
There are some places where error checking is incomplete:
The
with:
In that same area, you have the function declaration:
but there's no need to name the
Despite the above issues, in general, the code is good. The variable names are descriptive, the function names too. The code does flow quite well, and I don't feel like the functions are doing too much.
http errors
It's common in http-handling code to have a helper function to allow child methods to return an error, and then you catch that error and return a consistent, reused, error message system.
Consider a function like:
Now you can have your own set of handlers that have an error return value, and you can simplify the error-handling code that you have scattered over the place. The setup routine changes from:
to
Your ambition here is admirable, and I understand the motivation to accomplish something meaningful (a cool slack-bot is awesome), but I worry that you'll maybe miss some details in your haste.
General
In general, your code looks like it has been through the wringer a few times, and it has wrinkles, and a few worn spots. While it looks like you are already in the habit of running your code through the
go fmt routines, you should also add go vet and golint to your tools. You have a few issues I can see off-hand:-
there's no need to rename the import
net/http to http... that's what it's called already.-
the comments on
team are out of date. It's no longer exported (no longer capital-T Team.-
you have mis-uses of
log.Fatal(...). That call really is fatal, it calls os.Exit(1) when done. No code can run after the log.Fatal() so the following lines are redundant:errorMessage := fmt.Sprintf("Failed to create OAuth Token request, parameters: %v", request)
log.Fatal(errorMessage)
http.Error(writer, errorMessage, 501)
returnThe
httpError(...) is never sent.There are some places where error checking is incomplete:
database, err := gorm.Open("postgres", os.Getenv("DATABASE_URL"))
defer database.Close()The
authorizeSlackTeam method's handler function captures the body of the request, but does not use it. If you are simply trying to force the close of the body at the function end (and not the beginning), you can replace:body := request.Body
defer body.Close()with:
defer request.Body.Close()In that same area, you have the function declaration:
func authorizeSlackTeam(db *gorm.DB) (handler func(writer http.ResponseWriter, request *http.Request)) {but there's no need to name the
writer and request. It can be just:
func authorizeSlackTeam(db *gorm.DB) (handler func(http.ResponseWriter, *http.Request)) {
but even better, you can reuse the http.HandlerFunc` type to simplify it to:func authorizeSlackTeam(db *gorm.DB) (handler http.HandlerFunc) {Despite the above issues, in general, the code is good. The variable names are descriptive, the function names too. The code does flow quite well, and I don't feel like the functions are doing too much.
http errors
It's common in http-handling code to have a helper function to allow child methods to return an error, and then you catch that error and return a consistent, reused, error message system.
Consider a function like:
type UncheckedHandler func(http.ResponseWriter, *http.Request) error
func checkedHandler(handler UncheckedHandler) http.HandlerFunc {
return func(writer http.ResponseWriter, request *http.Request) {
if err := handler(writer, request) err != nil {
log.Printf(err)
http.Error(writer, err.Error(), 501)
}
}
}Now you can have your own set of handlers that have an error return value, and you can simplify the error-handling code that you have scattered over the place. The setup routine changes from:
http.HandleFunc("/", sayHelloWorld)
http.HandleFunc("/setColor", setColor)
http.HandleFunc("/authorizeSlackTeam", authorizeSlackTeam(database))to
http.HandleFunc("/", checkedHandler(sayHelloWorld))
http.HandleFunc("/setColor", checkedHandler(setColor))
http.HandleFunc("/authorizeSlackTeam", checkedHandler(authorizeSlackTeam(database)))Code Snippets
errorMessage := fmt.Sprintf("Failed to create OAuth Token request, parameters: %v", request)
log.Fatal(errorMessage)
http.Error(writer, errorMessage, 501)
returndatabase, err := gorm.Open("postgres", os.Getenv("DATABASE_URL"))
defer database.Close()body := request.Body
defer body.Close()defer request.Body.Close()func authorizeSlackTeam(db *gorm.DB) (handler func(writer http.ResponseWriter, request *http.Request)) {Context
StackExchange Code Review Q#154759, answer score: 2
Revisions (0)
No revisions yet.