Refactor parts of the code and add more tests

This is a refactor of parts of the code, some minor things such as adding
punctuation in the end of sentences and making the code a bit easier to read.

More tests has been added in the form of gometalinter which checks the code
health, style and spelling errors. The flaky cleanup test should also be fixed
with this commit.
This commit is contained in:
Anton Lindstrom 2016-07-21 19:48:29 +00:00
parent 3d07ee7804
commit d42994de5c
6 changed files with 83 additions and 56 deletions

View File

@ -1,10 +1,14 @@
language: go language: go
go: go:
- 1.2 - 1.5
- 1.6
- tip - tip
script: make test script:
- make get-deps
- make metalint
- make check
env: env:
- PGSTORE_TEST_CONN="postgres://postgres@127.0.0.1/test?sslmode=disable" - PGSTORE_TEST_CONN="postgres://postgres@127.0.0.1/test?sslmode=disable"
@ -13,6 +17,6 @@ before_script:
- psql -c 'create database test;' -U postgres - psql -c 'create database test;' -U postgres
addons: addons:
postgresql: "9.3" postgresql: "9.4"
sudo: false sudo: false

View File

@ -1,13 +1,39 @@
all: get-deps build all: get-deps build
.PHONY: build
build: build:
@go build pgstore.go go build ./...
.PHONY: get-deps
get-deps: get-deps:
@go get -d -v ./... go get -v ./...
test: get-deps .PHONY: test
@go test -v ./... test: get-deps metalint check
format: .PHONY: check
@go fmt ./... check:
go test -v -race -cover ./...
.PHONY: metalint
metalint:
which gometalinter > /dev/null || (go get github.com/alecthomas/gometalinter && gometalinter --install --update)
gometalinter --cyclo-over=20 -e "struct field Id should be ID" --enable="gofmt -s" --enable=misspell --fast ./...
.PHONY: fmt
fmt:
@go fmt ./... | awk '{ print "Please run go fmt"; exit 1 }'
.PHONY: docker-test
docker-test:
docker run -d -p 5432:5432 --name=pgstore_test_1 postgres:9.4
sleep 5
docker run --rm --link pgstore_test_1:postgres postgres:9.4 psql -c 'create database test;' -U postgres -h postgres
PGSTORE_TEST_CONN="postgres://postgres@127.0.0.1:5432/test?sslmode=disable" make check
docker kill pgstore_test_1
docker rm pgstore_test_1
.PHONY: docker-clean
docker-clean:
-docker kill pgstore_test_1
-docker rm pgstore_test_1

View File

@ -38,11 +38,11 @@ func (db *PGStore) cleanup(interval time.Duration, quit <-chan struct{}, done ch
for { for {
select { select {
case <-quit: case <-quit:
// Handle the quit signal // Handle the quit signal.
done <- struct{}{} done <- struct{}{}
return return
case <-ticker.C: case <-ticker.C:
// Delete expired sessions on each tick // Delete expired sessions on each tick.
err := db.deleteExpired() err := db.deleteExpired()
if err != nil { if err != nil {
log.Printf("pgstore: unable to delete expired sessions: %v", err) log.Printf("pgstore: unable to delete expired sessions: %v", err)

View File

@ -8,14 +8,14 @@ import (
) )
func TestCleanup(t *testing.T) { func TestCleanup(t *testing.T) {
ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) dsn := os.Getenv("PGSTORE_TEST_CONN")
if dsn == "" {
if err != nil { t.Skip("This test requires a real database.")
t.Fatal("Failed to get store", err)
} }
if ss == nil { ss, err := NewPGStore(dsn, []byte(secret))
t.Skip("This test requires a real database") if err != nil {
t.Fatal("Failed to get store", err)
} }
defer ss.Close() defer ss.Close()
@ -32,7 +32,7 @@ func TestCleanup(t *testing.T) {
t.Fatal("Failed to create session", err) t.Fatal("Failed to create session", err)
} }
// Expire the session // Expire the session.
session.Options.MaxAge = 1 session.Options.MaxAge = 1
m := make(http.Header) m := make(http.Header)
@ -40,8 +40,8 @@ func TestCleanup(t *testing.T) {
t.Fatal("failed to save session:", err.Error()) t.Fatal("failed to save session:", err.Error())
} }
// Give the ticker a moment to run // Give the ticker a moment to run.
time.Sleep(time.Second * 1) time.Sleep(time.Millisecond * 1500)
// SELECT expired sessions. We should get a zero-length result slice back. // SELECT expired sessions. We should get a zero-length result slice back.
var results []int64 var results []int64

View File

@ -10,10 +10,12 @@ import (
"github.com/coopernurse/gorp" "github.com/coopernurse/gorp"
"github.com/gorilla/securecookie" "github.com/gorilla/securecookie"
"github.com/gorilla/sessions" "github.com/gorilla/sessions"
// Include the pq postgres driver.
_ "github.com/lib/pq" _ "github.com/lib/pq"
) )
// PGStore represents the currently configured session store // PGStore represents the currently configured session store.
type PGStore struct { type PGStore struct {
Codecs []securecookie.Codec Codecs []securecookie.Codec
Options *sessions.Options Options *sessions.Options
@ -36,7 +38,7 @@ type Session struct {
func NewPGStore(dbURL string, keyPairs ...[]byte) (*PGStore, error) { func NewPGStore(dbURL string, keyPairs ...[]byte) (*PGStore, error) {
db, err := sql.Open("postgres", dbURL) db, err := sql.Open("postgres", dbURL)
if err != nil { if err != nil {
// Ignore and return nil // Ignore PGStore value and return nil.
return nil, err return nil, err
} }
return NewPGStoreFromPool(db, keyPairs...) return NewPGStoreFromPool(db, keyPairs...)
@ -62,14 +64,14 @@ func NewPGStoreFromPool(db *sql.DB, keyPairs ...[]byte) (*PGStore, error) {
err := dbmap.CreateTablesIfNotExists() err := dbmap.CreateTablesIfNotExists()
if err != nil { if err != nil {
// Ignore and return nil // Ignore PGStore value and return nil.
return nil, err return nil, err
} }
return dbStore, nil return dbStore, nil
} }
// Close closes the database connection // Close closes the database connection.
func (db *PGStore) Close() { func (db *PGStore) Close() {
db.DbMap.Db.Close() db.DbMap.Db.Close()
} }
@ -80,12 +82,13 @@ func (db *PGStore) Get(r *http.Request, name string) (*sessions.Session, error)
return sessions.GetRegistry(r).Get(db, name) return sessions.GetRegistry(r).Get(db, name)
} }
// New returns a new session for the given name w/o adding it to the registry. // New returns a new session for the given name without adding it to the registry.
func (db *PGStore) New(r *http.Request, name string) (*sessions.Session, error) { func (db *PGStore) New(r *http.Request, name string) (*sessions.Session, error) {
session := sessions.NewSession(db, name) session := sessions.NewSession(db, name)
if session == nil { if session == nil {
return session, nil return nil, nil
} }
opts := *db.Options opts := *db.Options
session.Options = &(opts) session.Options = &(opts)
session.IsNew = true session.IsNew = true
@ -121,7 +124,8 @@ func (db *PGStore) Save(r *http.Request, w http.ResponseWriter, session *session
// Generate a random session ID key suitable for storage in the DB // Generate a random session ID key suitable for storage in the DB
session.ID = strings.TrimRight( session.ID = strings.TrimRight(
base32.StdEncoding.EncodeToString( base32.StdEncoding.EncodeToString(
securecookie.GenerateRandomKey(32)), "=") securecookie.GenerateRandomKey(32),
), "=")
} }
if err := db.save(session); err != nil { if err := db.save(session); err != nil {
@ -165,39 +169,33 @@ func (db *PGStore) MaxAge(age int) {
} }
// load fetches a session by ID from the database and decodes its content // load fetches a session by ID from the database and decodes its content
// into session.Values // into session.Values.
func (db *PGStore) load(session *sessions.Session) error { func (db *PGStore) load(session *sessions.Session) error {
var s Session var s Session
err := db.DbMap.SelectOne(&s, "SELECT * FROM http_sessions WHERE key = $1", session.ID) err := db.DbMap.SelectOne(&s, "SELECT * FROM http_sessions WHERE key = $1", session.ID)
if err != nil {
if err := securecookie.DecodeMulti(session.Name(), string(s.Data),
&session.Values, db.Codecs...); err != nil {
return err return err
} }
return err return securecookie.DecodeMulti(session.Name(), string(s.Data), &session.Values, db.Codecs...)
} }
// save writes encoded session.Values to a database record. // save writes encoded session.Values to a database record.
// writes to http_sessions table by default. // writes to http_sessions table by default.
func (db *PGStore) save(session *sessions.Session) error { func (db *PGStore) save(session *sessions.Session) error {
encoded, err := securecookie.EncodeMulti(session.Name(), session.Values, encoded, err := securecookie.EncodeMulti(session.Name(), session.Values, db.Codecs...)
db.Codecs...)
if err != nil { if err != nil {
return err return err
} }
var createdOn time.Time
var expiresOn time.Time
crOn := session.Values["created_on"] crOn := session.Values["created_on"]
exOn := session.Values["expires_on"] exOn := session.Values["expires_on"]
if crOn == nil { var expiresOn time.Time
createdOn, ok := crOn.(time.Time)
if !ok {
createdOn = time.Now() createdOn = time.Now()
} else {
createdOn = crOn.(time.Time)
} }
if exOn == nil { if exOn == nil {
@ -218,11 +216,10 @@ func (db *PGStore) save(session *sessions.Session) error {
} }
if session.IsNew { if session.IsNew {
err = db.DbMap.Insert(&s) return db.DbMap.Insert(&s)
} else {
_, err = db.DbMap.Exec("update http_sessions set data=$1, modified_on=$2, expires_on=$3 where key=$4", s.Data, s.ModifiedOn, s.ExpiresOn, s.Key)
} }
_, err = db.DbMap.Exec("update http_sessions set data=$1, modified_on=$2, expires_on=$3 where key=$4", s.Data, s.ModifiedOn, s.ExpiresOn, s.Key)
return err return err
} }

View File

@ -27,14 +27,14 @@ func (ho headerOnlyResponseWriter) WriteHeader(int) {
var secret = "EyaC2BPcJtNqU3tjEHy+c+Wmqc1yihYIbUWEl/jk0Ga73kWBclmuSFd9HuJKwJw/Wdsh1XnjY2Bw1HBVph6WOw==" var secret = "EyaC2BPcJtNqU3tjEHy+c+Wmqc1yihYIbUWEl/jk0Ga73kWBclmuSFd9HuJKwJw/Wdsh1XnjY2Bw1HBVph6WOw=="
func TestPGStore(t *testing.T) { func TestPGStore(t *testing.T) {
ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) dsn := os.Getenv("PGSTORE_TEST_CONN")
if dsn == "" {
if err != nil { t.Skip("This test requires a real database.")
t.Fatal("failed to get store", err.Error())
} }
if ss == nil { ss, err := NewPGStore(dsn, []byte(secret))
t.Skip("This test requires a real database") if err != nil {
t.Fatal("Failed to get store", err)
} }
defer ss.Close() defer ss.Close()
@ -125,14 +125,14 @@ func TestPGStore(t *testing.T) {
} }
func TestSessionOptionsAreUniquePerSession(t *testing.T) { func TestSessionOptionsAreUniquePerSession(t *testing.T) {
ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) dsn := os.Getenv("PGSTORE_TEST_CONN")
if dsn == "" {
if err != nil { t.Skip("This test requires a real database.")
t.Fatal("Failed to get store", err)
} }
if ss == nil { ss, err := NewPGStore(dsn, []byte(secret))
t.Skip("This test requires a real database") if err != nil {
t.Fatal("Failed to get store", err)
} }
defer ss.Close() defer ss.Close()