From d42994de5c7d7839981c542787eab8c261a33b31 Mon Sep 17 00:00:00 2001 From: Anton Lindstrom Date: Thu, 21 Jul 2016 19:48:29 +0000 Subject: [PATCH] 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. --- .travis.yml | 10 +++++++--- Makefile | 38 ++++++++++++++++++++++++++++++++------ cleanup.go | 4 ++-- cleanup_test.go | 18 +++++++++--------- pgstore.go | 45 +++++++++++++++++++++------------------------ pgstore_test.go | 24 ++++++++++++------------ 6 files changed, 83 insertions(+), 56 deletions(-) diff --git a/.travis.yml b/.travis.yml index 64ab288..32e28a8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,14 @@ language: go go: - - 1.2 + - 1.5 + - 1.6 - tip -script: make test +script: + - make get-deps + - make metalint + - make check env: - 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 addons: - postgresql: "9.3" + postgresql: "9.4" sudo: false diff --git a/Makefile b/Makefile index 735beee..25325b0 100644 --- a/Makefile +++ b/Makefile @@ -1,13 +1,39 @@ all: get-deps build +.PHONY: build build: - @go build pgstore.go + go build ./... +.PHONY: get-deps get-deps: - @go get -d -v ./... + go get -v ./... -test: get-deps - @go test -v ./... +.PHONY: test +test: get-deps metalint check -format: - @go fmt ./... +.PHONY: check +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 diff --git a/cleanup.go b/cleanup.go index 5a5caed..51aee3f 100644 --- a/cleanup.go +++ b/cleanup.go @@ -38,11 +38,11 @@ func (db *PGStore) cleanup(interval time.Duration, quit <-chan struct{}, done ch for { select { case <-quit: - // Handle the quit signal + // Handle the quit signal. done <- struct{}{} return case <-ticker.C: - // Delete expired sessions on each tick + // Delete expired sessions on each tick. err := db.deleteExpired() if err != nil { log.Printf("pgstore: unable to delete expired sessions: %v", err) diff --git a/cleanup_test.go b/cleanup_test.go index f63ccbe..00ffa58 100644 --- a/cleanup_test.go +++ b/cleanup_test.go @@ -8,14 +8,14 @@ import ( ) func TestCleanup(t *testing.T) { - ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) - - if err != nil { - t.Fatal("Failed to get store", err) + dsn := os.Getenv("PGSTORE_TEST_CONN") + if dsn == "" { + t.Skip("This test requires a real database.") } - if ss == nil { - t.Skip("This test requires a real database") + ss, err := NewPGStore(dsn, []byte(secret)) + if err != nil { + t.Fatal("Failed to get store", err) } defer ss.Close() @@ -32,7 +32,7 @@ func TestCleanup(t *testing.T) { t.Fatal("Failed to create session", err) } - // Expire the session + // Expire the session. session.Options.MaxAge = 1 m := make(http.Header) @@ -40,8 +40,8 @@ func TestCleanup(t *testing.T) { t.Fatal("failed to save session:", err.Error()) } - // Give the ticker a moment to run - time.Sleep(time.Second * 1) + // Give the ticker a moment to run. + time.Sleep(time.Millisecond * 1500) // SELECT expired sessions. We should get a zero-length result slice back. var results []int64 diff --git a/pgstore.go b/pgstore.go index 3fe7165..4c3549c 100644 --- a/pgstore.go +++ b/pgstore.go @@ -10,10 +10,12 @@ import ( "github.com/coopernurse/gorp" "github.com/gorilla/securecookie" "github.com/gorilla/sessions" + + // Include the pq postgres driver. _ "github.com/lib/pq" ) -// PGStore represents the currently configured session store +// PGStore represents the currently configured session store. type PGStore struct { Codecs []securecookie.Codec Options *sessions.Options @@ -36,7 +38,7 @@ type Session struct { func NewPGStore(dbURL string, keyPairs ...[]byte) (*PGStore, error) { db, err := sql.Open("postgres", dbURL) if err != nil { - // Ignore and return nil + // Ignore PGStore value and return nil. return nil, err } return NewPGStoreFromPool(db, keyPairs...) @@ -62,14 +64,14 @@ func NewPGStoreFromPool(db *sql.DB, keyPairs ...[]byte) (*PGStore, error) { err := dbmap.CreateTablesIfNotExists() if err != nil { - // Ignore and return nil + // Ignore PGStore value and return nil. return nil, err } return dbStore, nil } -// Close closes the database connection +// Close closes the database connection. func (db *PGStore) 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) } -// 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) { session := sessions.NewSession(db, name) if session == nil { - return session, nil + return nil, nil } + opts := *db.Options session.Options = &(opts) 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 session.ID = strings.TrimRight( base32.StdEncoding.EncodeToString( - securecookie.GenerateRandomKey(32)), "=") + securecookie.GenerateRandomKey(32), + ), "=") } 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 -// into session.Values +// into session.Values. func (db *PGStore) load(session *sessions.Session) error { var s Session err := db.DbMap.SelectOne(&s, "SELECT * FROM http_sessions WHERE key = $1", session.ID) - - if err := securecookie.DecodeMulti(session.Name(), string(s.Data), - &session.Values, db.Codecs...); err != nil { + if err != nil { 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. // writes to http_sessions table by default. func (db *PGStore) save(session *sessions.Session) error { - encoded, err := securecookie.EncodeMulti(session.Name(), session.Values, - db.Codecs...) - + encoded, err := securecookie.EncodeMulti(session.Name(), session.Values, db.Codecs...) if err != nil { return err } - var createdOn time.Time - var expiresOn time.Time - crOn := session.Values["created_on"] exOn := session.Values["expires_on"] - if crOn == nil { + var expiresOn time.Time + + createdOn, ok := crOn.(time.Time) + if !ok { createdOn = time.Now() - } else { - createdOn = crOn.(time.Time) } if exOn == nil { @@ -218,11 +216,10 @@ func (db *PGStore) save(session *sessions.Session) error { } if session.IsNew { - err = 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) + return db.DbMap.Insert(&s) } + _, 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 } diff --git a/pgstore_test.go b/pgstore_test.go index 33144be..cfc0a92 100644 --- a/pgstore_test.go +++ b/pgstore_test.go @@ -27,14 +27,14 @@ func (ho headerOnlyResponseWriter) WriteHeader(int) { var secret = "EyaC2BPcJtNqU3tjEHy+c+Wmqc1yihYIbUWEl/jk0Ga73kWBclmuSFd9HuJKwJw/Wdsh1XnjY2Bw1HBVph6WOw==" func TestPGStore(t *testing.T) { - ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) - - if err != nil { - t.Fatal("failed to get store", err.Error()) + dsn := os.Getenv("PGSTORE_TEST_CONN") + if dsn == "" { + t.Skip("This test requires a real database.") } - if ss == nil { - t.Skip("This test requires a real database") + ss, err := NewPGStore(dsn, []byte(secret)) + if err != nil { + t.Fatal("Failed to get store", err) } defer ss.Close() @@ -125,14 +125,14 @@ func TestPGStore(t *testing.T) { } func TestSessionOptionsAreUniquePerSession(t *testing.T) { - ss, err := NewPGStore(os.Getenv("PGSTORE_TEST_CONN"), []byte(secret)) - - if err != nil { - t.Fatal("Failed to get store", err) + dsn := os.Getenv("PGSTORE_TEST_CONN") + if dsn == "" { + t.Skip("This test requires a real database.") } - if ss == nil { - t.Skip("This test requires a real database") + ss, err := NewPGStore(dsn, []byte(secret)) + if err != nil { + t.Fatal("Failed to get store", err) } defer ss.Close()