Merge pull request #19 from antonlindstrom/refactor/cleanup

Refactor parts of the code and add more tests

.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

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
+.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

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)

cleanup_test.go

@@ -8,14 +8,14 @@ import (
 )
 
 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()
@@ -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
+	// 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.
 	var results []int64

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 != nil {
-	if err := securecookie.DecodeMulti(session.Name(), string(s.Data),
-		&session.Values, db.Codecs...); 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,
+	encoded, err := securecookie.EncodeMulti(session.Name(), session.Values, db.Codecs...)
-		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)
+		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
 }
 

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))
+	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()
@@ -125,14 +125,14 @@ func TestPGStore(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()