Skip to content

Commit e6c4b4c

Browse files
appleboyappleboy
committed
fix: improve error handling and message consistency across methods
- Change error message format to lowercase in `Serialize` method - Update session new check condition in `New` method - Add error handling for connection close in `Delete` method - Add error handling for connection close in `ping` method - Add error handling for connection close in `save` method - Add error handling for connection close in `load` method - Add error handling for connection close in `delete` method - Add error handling for store close in tests and example functions Signed-off-by: appleboy <appleboy.tw@gmail.com>
1 parent 3e044de commit e6c4b4c

2 files changed

Lines changed: 77 additions & 17 deletions

File tree

‎redistore.go‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ func (s JSONSerializer) Serialize(ss *sessions.Session) ([]byte, error) {
5454
for k, v := range ss.Values {
5555
ks, ok := k.(string)
5656
if !ok {
57-
err := fmt.Errorf("Non-string key value, cannot serialize session to JSON: %v", k)
57+
err := fmt.Errorf("non-string key value, cannot serialize session to JSON: %v", k)
5858
fmt.Printf("redistore.JSONSerializer.serialize() Error: %v", err)
5959
return nil, err
6060
}
@@ -365,7 +365,7 @@ func (s *RediStore) New(r *http.Request, name string) (*sessions.Session, error)
365365
err = securecookie.DecodeMulti(name, c.Value, &session.ID, s.Codecs...)
366366
if err == nil {
367367
ok, err = s.load(session)
368-
session.IsNew = !(err == nil && ok) // not new if no error and data available
368+
session.IsNew = err != nil || !ok // not new if no error and data available
369369
}
370370
}
371371
return session, err
@@ -402,7 +402,11 @@ func (s *RediStore) Save(r *http.Request, w http.ResponseWriter, session *sessio
402402
// Set session.Options.MaxAge = -1 and call Save instead. - July 18th, 2013
403403
func (s *RediStore) Delete(r *http.Request, w http.ResponseWriter, session *sessions.Session) error {
404404
conn := s.Pool.Get()
405-
defer conn.Close()
405+
defer func() {
406+
if err := conn.Close(); err != nil {
407+
fmt.Printf("Error closing connection: %v\n", err)
408+
}
409+
}()
406410
if _, err := conn.Do("DEL", s.keyPrefix+session.ID); err != nil {
407411
return err
408412
}
@@ -420,7 +424,11 @@ func (s *RediStore) Delete(r *http.Request, w http.ResponseWriter, session *sess
420424
// ping does an internal ping against a server to check if it is alive.
421425
func (s *RediStore) ping() (bool, error) {
422426
conn := s.Pool.Get()
423-
defer conn.Close()
427+
defer func() {
428+
if err := conn.Close(); err != nil {
429+
fmt.Printf("Error closing connection: %v\n", err)
430+
}
431+
}()
424432
data, err := conn.Do("PING")
425433
if err != nil || data == nil {
426434
return false, err
@@ -438,7 +446,11 @@ func (s *RediStore) save(session *sessions.Session) error {
438446
return errors.New("SessionStore: the value to store is too big")
439447
}
440448
conn := s.Pool.Get()
441-
defer conn.Close()
449+
defer func() {
450+
if err := conn.Close(); err != nil {
451+
fmt.Printf("Error closing connection: %v\n", err)
452+
}
453+
}()
442454
if err = conn.Err(); err != nil {
443455
return err
444456
}
@@ -454,7 +466,11 @@ func (s *RediStore) save(session *sessions.Session) error {
454466
// returns true if there is a sessoin data in DB
455467
func (s *RediStore) load(session *sessions.Session) (bool, error) {
456468
conn := s.Pool.Get()
457-
defer conn.Close()
469+
defer func() {
470+
if err := conn.Close(); err != nil {
471+
fmt.Printf("Error closing connection: %v\n", err)
472+
}
473+
}()
458474
if err := conn.Err(); err != nil {
459475
return false, err
460476
}
@@ -475,7 +491,11 @@ func (s *RediStore) load(session *sessions.Session) (bool, error) {
475491
// delete removes keys from redis if MaxAge<0
476492
func (s *RediStore) delete(session *sessions.Session) error {
477493
conn := s.Pool.Get()
478-
defer conn.Close()
494+
defer func() {
495+
if err := conn.Close(); err != nil {
496+
fmt.Printf("Error closing connection: %v\n", err)
497+
}
498+
}()
479499
if _, err := conn.Do("DEL", s.keyPrefix+session.ID); err != nil {
480500
return err
481501
}

‎redistore_test.go‎

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,11 @@ func TestRediStore(t *testing.T) {
102102
if err != nil {
103103
t.Fatal(err.Error())
104104
}
105-
defer store.Close()
105+
defer func() {
106+
if err := store.Close(); err != nil {
107+
fmt.Printf("Error closing store: %v\n", err)
108+
}
109+
}()
106110

107111
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
108112
rsp := NewRecorder()
@@ -133,7 +137,11 @@ func TestRediStore(t *testing.T) {
133137
if err != nil {
134138
t.Fatal(err.Error())
135139
}
136-
defer store.Close()
140+
defer func() {
141+
if err := store.Close(); err != nil {
142+
fmt.Printf("Error closing store: %v\n", err)
143+
}
144+
}()
137145

138146
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
139147
req.Header.Add("Cookie", cookies[0])
@@ -175,7 +183,11 @@ func TestRediStore(t *testing.T) {
175183
if err != nil {
176184
t.Fatal(err.Error())
177185
}
178-
defer store.Close()
186+
defer func() {
187+
if err := store.Close(); err != nil {
188+
fmt.Printf("Error closing store: %v\n", err)
189+
}
190+
}()
179191

180192
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
181193
rsp := NewRecorder()
@@ -204,7 +216,11 @@ func TestRediStore(t *testing.T) {
204216
if err != nil {
205217
t.Fatal(err.Error())
206218
}
207-
defer store.Close()
219+
defer func() {
220+
if err := store.Close(); err != nil {
221+
fmt.Printf("Error closing store: %v\n", err)
222+
}
223+
}()
208224

209225
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
210226
req.Header.Add("Cookie", cookies[0])
@@ -262,7 +278,11 @@ func TestRediStore(t *testing.T) {
262278
if err != nil {
263279
t.Fatal(err.Error())
264280
}
265-
defer store.Close()
281+
defer func() {
282+
if err := store.Close(); err != nil {
283+
fmt.Printf("Error closing store: %v\n", err)
284+
}
285+
}()
266286

267287
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
268288
rsp := NewRecorder()
@@ -305,7 +325,11 @@ func TestRediStore(t *testing.T) {
305325
if err != nil {
306326
t.Fatal(err.Error())
307327
}
308-
defer store.Close()
328+
defer func() {
329+
if err := store.Close(); err != nil {
330+
fmt.Printf("Error closing store: %v\n", err)
331+
}
332+
}()
309333

310334
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
311335
rsp := NewRecorder()
@@ -344,7 +368,11 @@ func TestRediStore(t *testing.T) {
344368

345369
func TestPingGoodPort(t *testing.T) {
346370
store, _ := NewRediStore(10, "tcp", ":6379", "", "", []byte("secret-key"))
347-
defer store.Close()
371+
defer func() {
372+
if err := store.Close(); err != nil {
373+
fmt.Printf("Error closing store: %v\n", err)
374+
}
375+
}()
348376
ok, err := store.ping()
349377
if err != nil {
350378
t.Error(err.Error())
@@ -356,7 +384,11 @@ func TestPingGoodPort(t *testing.T) {
356384

357385
func TestPingBadPort(t *testing.T) {
358386
store, _ := NewRediStore(10, "tcp", ":6378", "", "", []byte("secret-key"))
359-
defer store.Close()
387+
defer func() {
388+
if err := store.Close(); err != nil {
389+
fmt.Printf("Error closing store: %v\n", err)
390+
}
391+
}()
360392
_, err := store.ping()
361393
if err == nil {
362394
t.Error("Expected error")
@@ -369,7 +401,11 @@ func TestNewRediStoreWithURL(t *testing.T) {
369401
if err != nil {
370402
t.Fatalf("Expected no error, got %v", err)
371403
}
372-
defer store.Close()
404+
defer func() {
405+
if err := store.Close(); err != nil {
406+
fmt.Printf("Error closing store: %v\n", err)
407+
}
408+
}()
373409

374410
req, _ := http.NewRequest("GET", "http://localhost:8080/", nil)
375411
rsp := NewRecorder()
@@ -406,7 +442,11 @@ func ExampleRediStore() {
406442
if err != nil {
407443
panic(err)
408444
}
409-
defer store.Close()
445+
defer func() {
446+
if err := store.Close(); err != nil {
447+
fmt.Printf("Error closing store: %v\n", err)
448+
}
449+
}()
410450
}
411451

412452
func init() {

0 commit comments

Comments
 (0)