#80 add context_legacy.go and context.go for go 1.7 migration #95

shawnps · 2016-09-21T05:05:26Z

Tests on Go 1.7 currently failing with:

➜  sessions [go17-context] go test
--- FAIL: TestFlashes (0.00s)
    sessions_test.go:71: No cookies. Header: map[]
FAIL
exit status 1
FAIL    github.com/gorilla/sessions 0.014s

Trying to debug. cc @elithrar

elithrar · 2016-09-21T06:26:15Z

I suspect this is because the copied http.Request isn't being passed back to the caller, and thus the context isn't retained, but need to dig into where that's being triggered (late here, so will look tomorrow).

shawnps · 2016-09-21T15:49:55Z

@elithrar thanks! I appreciate you taking the time to look. Let me know what I can do, if you don't have time to debug I will continue to try. Figured I'd just ping you since you probably have better insight on it than me.

elithrar · 2016-09-22T02:14:50Z

Still looking at this. Trying to debug where we forget to return any copy of the http.Request after doing a contextSave.

shawnps · 2016-09-23T00:53:29Z

@elithrar after returning the request in the Get methods, the tests pass for me locally (will see about travis in a bit). This of course changes the contract for the Get method, so I'm not sure how you'd like to handle that.

elithrar · 2016-09-23T00:59:21Z

I think that was inevitable as we have to return any copied Request.

Let me review tonight - thanks for working on this!
On Thu, Sep 22, 2016 at 5:53 PM Shawn Smith [email protected]
wrote:

@elithrar https://github.com/elithrar after returning the request in
the Get methods, the tests pass for me locally (will see about travis in
a bit). This of course changes the contract for the Get method, so I'm
not sure how you'd like to handle that.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABIcAjDsNf0XfTBDZ3febrLy2uXwAxIks5qsyMKgaJpZM4KCZ2A
.

elithrar · 2016-09-23T03:59:12Z

Apparently I already came to these conclusions a while ago: #80

I'd like to tag a release prior to merging this, and then a release after, so anyone who is vendoring can vendor pre- / post- breaking change.

shawnps · 2016-09-23T04:02:38Z

Thanks, let me know if there's anything else I can do to help

elithrar · 2016-09-23T05:40:56Z

Tagged v1.1 before I break this. Will do a v1.2 on the weekend when I merge this.

elithrar · 2016-09-25T00:35:07Z

store.go

@@ -76,8 +76,12 @@ type CookieStore struct {
 //
 // It returns a new session and an error if the session exists but could
 // not be decoded.
-func (s *CookieStore) Get(r *http.Request, name string) (*Session, error) {
-	return GetRegistry(r).Get(s, name)
+func (s *CookieStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {


We shouldn't need to return the store here, if we're already replacing the *http.Request passed in.

elithrar

GetRegistry API changes are OK
Store changes are not. I don't believe they are required.

elithrar · 2016-09-25T00:35:16Z

store.go

@@ -179,8 +183,12 @@ func (s *FilesystemStore) MaxLength(l int) {
 // Get returns a session for the given name after adding it to the registry.
 //
 // See CookieStore.Get().
-func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, error) {
-	return GetRegistry(r).Get(s, name)
+func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {


We shouldn't need to return the store here, if we're already replacing the *http.Request passed in.

elithrar · 2016-09-25T00:35:57Z

store.go

@@ -21,7 +21,7 @@ import (
 // See CookieStore and FilesystemStore for examples.
 type Store interface {
 	// Get should return a cached session.
-	Get(r *http.Request, name string) (*Session, error)
+	Get(r *http.Request, name string) (*Session, *http.Request, error)


Ditto. Breaking the Store interface is a major break - it requires third-party stores to update, which may be out of the control of the package user.

elithrar · 2016-09-25T00:37:02Z

Further: we should put a note in the README that indicates why this is breaking, and the steps to update your code.

shawnps · 2016-09-25T03:11:40Z

Store changes are not. I don't believe they are required.

@elithrar the tests fail without the changes in the second commit, unless I'm misunderstanding something:

➜  sessions [go17-context] git rev-parse HEAD
e1ef5d16938bbb62d2404dd30061606c3725ef7e
➜  sessions [go17-context] go test
PASS
ok      github.com/gorilla/sessions 0.014s




➜  sessions [go17-context] git checkout ecfa348961f780c90ace75df52f8c588f3762aef
➜  sessions  go test
--- FAIL: TestFlashes (0.00s)
    sessions_test.go:71: No cookies. Header: map[]
FAIL
exit status 1
FAIL    github.com/gorilla/sessions 0.014s

elithrar · 2016-09-25T03:20:45Z

The internal changes are required, but you shouldn't need to change the method signature. You already have a request, so no need to return one.

shawnps · 2016-09-25T03:25:50Z

Sorry, I'm not sure what to do from here. The code didn't work until I made the Get methods return the request, so If I revert the signature of Store.Get then I have to revert all of the implementations of it, which makes the test fail.

shawnps · 2016-09-25T03:28:06Z

@elithrar example:

diff --git a/sessions_test.go b/sessions_test.go
index f00813e..c166b05 100644
--- a/sessions_test.go
+++ b/sessions_test.go
@@ -48,7 +48,7 @@ func TestFlashes(t *testing.T) {
    req, _ = http.NewRequest("GET", "http://localhost:8080/", nil)
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Get a flash.
@@ -71,7 +71,7 @@ func TestFlashes(t *testing.T) {
        t.Fatal("No cookies. Header:", hdr)
    }

-   if _, req, err = store.Get(req, "session:key"); err.Error() != "sessions: invalid character in cookie name: session:key" {
+   if _, err = store.Get(req, "session:key"); err.Error() != "sessions: invalid character in cookie name: session:key" {
        t.Fatalf("Expected error due to invalid cookie name")
    }

@@ -81,7 +81,7 @@ func TestFlashes(t *testing.T) {
    req.Header.Add("Cookie", cookies[0])
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Check all saved values.
@@ -114,7 +114,7 @@ func TestFlashes(t *testing.T) {
    req, _ = http.NewRequest("GET", "http://localhost:8080/", nil)
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Get a flash.
@@ -141,7 +141,7 @@ func TestFlashes(t *testing.T) {
    req.Header.Add("Cookie", cookies[0])
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Check all saved values.
diff --git a/store.go b/store.go
index f041a6b..6c901d6 100644
--- a/store.go
+++ b/store.go
@@ -21,7 +21,7 @@ import (
 // See CookieStore and FilesystemStore for examples.
 type Store interface {
    // Get should return a cached session.
-   Get(r *http.Request, name string) (*Session, *http.Request, error)
+   Get(r *http.Request, name string) (*Session, error)

    // New should create and return a new session.
    //
@@ -76,12 +76,12 @@ type CookieStore struct {
 //
 // It returns a new session and an error if the session exists but could
 // not be decoded.
-func (s *CookieStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {
+func (s *CookieStore) Get(r *http.Request, name string) (*Session, error) {
    var reg *Registry
    reg, r = GetRegistry(r)
    ses, err := reg.Get(s, name)

-   return ses, r, err
+   return ses, err
 }

 // New returns a session for the given name without adding it to the registry.
@@ -183,12 +183,12 @@ func (s *FilesystemStore) MaxLength(l int) {
 // Get returns a session for the given name after adding it to the registry.
 //
 // See CookieStore.Get().
-func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {
+func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, error) {
    var reg *Registry
    reg, r = GetRegistry(r)
    ses, err := reg.Get(s, name)

-   return ses, r, err
+   return ses, err
 }

 // New returns a session for the given name without adding it to the registry.

➜  sessions [go17-context] go test
--- FAIL: TestFlashes (0.00s)
    sessions_test.go:71: No cookies. Header: map[]
FAIL
exit status 1
FAIL    github.com/gorilla/sessions 0.014s

shawnps · 2016-09-25T03:30:21Z

Btw, I also tried setting reg.request = r in the Get method but that did not work.

elithrar · 2016-09-25T04:37:56Z

https://github.com/gorilla/sessions/tree/pr95 @shawnps

diff --git a/sessions_test.go b/sessions_test.go
index f00813e..c166b05 100644
--- a/sessions_test.go
+++ b/sessions_test.go
@@ -48,7 +48,7 @@ func TestFlashes(t *testing.T) {
    req, _ = http.NewRequest("GET", "http://localhost:8080/", nil)
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Get a flash.
@@ -71,7 +71,7 @@ func TestFlashes(t *testing.T) {
        t.Fatal("No cookies. Header:", hdr)
    }

-   if _, req, err = store.Get(req, "session:key"); err.Error() != "sessions: invalid character in cookie name: session:key" {
+   if _, err = store.Get(req, "session:key"); err.Error() != "sessions: invalid character in cookie name: session:key" {
        t.Fatalf("Expected error due to invalid cookie name")
    }

@@ -81,7 +81,7 @@ func TestFlashes(t *testing.T) {
    req.Header.Add("Cookie", cookies[0])
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Check all saved values.
@@ -114,7 +114,7 @@ func TestFlashes(t *testing.T) {
    req, _ = http.NewRequest("GET", "http://localhost:8080/", nil)
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Get a flash.
@@ -141,7 +141,7 @@ func TestFlashes(t *testing.T) {
    req.Header.Add("Cookie", cookies[0])
    rsp = NewRecorder()
    // Get a session.
-   if session, req, err = store.Get(req, "session-key"); err != nil {
+   if session, err = store.Get(req, "session-key"); err != nil {
        t.Fatalf("Error getting session: %v", err)
    }
    // Check all saved values.
diff --git a/store.go b/store.go
index f041a6b..0aab0d4 100644
--- a/store.go
+++ b/store.go
@@ -21,7 +21,7 @@ import (
 // See CookieStore and FilesystemStore for examples.
 type Store interface {
    // Get should return a cached session.
-   Get(r *http.Request, name string) (*Session, *http.Request, error)
+   Get(r *http.Request, name string) (*Session, error)

    // New should create and return a new session.
    //
@@ -76,12 +76,12 @@ type CookieStore struct {
 //
 // It returns a new session and an error if the session exists but could
 // not be decoded.
-func (s *CookieStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {
-   var reg *Registry
-   reg, r = GetRegistry(r)
+func (s *CookieStore) Get(r *http.Request, name string) (*Session, error) {
+   reg, req := GetRegistry(r)
    ses, err := reg.Get(s, name)
+   *r = *req

-   return ses, r, err
+   return ses, err
 }

 // New returns a session for the given name without adding it to the registry.
@@ -183,12 +183,12 @@ func (s *FilesystemStore) MaxLength(l int) {
 // Get returns a session for the given name after adding it to the registry.
 //
 // See CookieStore.Get().
-func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, *http.Request, error) {
-   var reg *Registry
-   reg, r = GetRegistry(r)
+func (s *FilesystemStore) Get(r *http.Request, name string) (*Session, error) {
+   reg, req := GetRegistry(r)
    ses, err := reg.Get(s, name)
+   *r = *req

-   return ses, r, err
+   return ses, err
 }

 // New returns a session for the given name without adding it to the registry.

shawnps · 2016-09-25T05:04:02Z

@elithrar got it, I'll push an update

shawnps · 2016-10-03T06:32:14Z

@elithrar does the new stuff look okay? Anything I can do?

elithrar · 2016-10-03T06:56:04Z

Thinking it through. I'd like to automate some PR creation for a few people
this would break and/or add some notes to the README to explain the
breakage more.
On Mon, Oct 3, 2016 at 7:32 AM Shawn Smith [email protected] wrote:

@elithrar https://github.com/elithrar does the new stuff look okay?
Anything I can do?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABIcPIJ8w7QYGwhBs-aFZyF5ys77Dmbks5qwKFugaJpZM4KCZ2A
.

shawnps · 2016-10-03T06:59:07Z

@elithrar ah you did mention the README. I'll add something to this PR updating the README, so please take a look when you have time!

As for the PR creation for people this will break, I can investigate that as well if you'd like. I have a script that automatically makes PRs after running gofmt -s -w on a repo, so I could use a modified version of that which takes a list of repos and automatically makes a PR updating GetRegistry calls. I guess we could use GitHub's code search for people calling GetRegistry to get the list of repos.

shawnps · 2016-10-03T07:01:14Z

Might be able to use something like this to automate, or just use sed:

https://github.com/golang/tools/blob/master/refactor/rename/rename.go#L38

shawnps · 2016-10-03T08:19:29Z

@elithrar I just added a note to the README, see the rendered Markdown here:

https://github.com/shawnps/sessions/blob/dc17d8cc4b7165432d37bd224ae43309f97950a9/README.md#notes

shawnps · 2016-10-03T08:19:51Z

(comments/edit suggestions welcome of course)

nitingupta910 · 2016-10-31T23:46:09Z

Following this long conversation, it appears changes are now ready to be pulled?

elithrar · 2016-11-01T00:30:25Z

We need to:

a) tag the current HEAD before we break
b) break it
c) tag the new HEAD post break

We also need to document it and reach out to those using the library
without vendoring (the majority of mux users).
On Mon, Oct 31, 2016 at 4:46 PM Nitin Gupta [email protected]
wrote:

Following this long conversation, it appears changes are now ready to be
pulled?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#95 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABIcAJINHs41WaQFfEA9PUTs7SxmFUdks5q5n3CgaJpZM4KCZ2A
.

omeid · 2017-01-20T05:07:43Z

Just a friendly ping. Is there anything needs to be done to merge this in?

elithrar · 2017-01-22T16:46:22Z

@omeid I'd like to do more work to automatically create PRs on those who import us, don't vendor, and thus will have their applications break.

gorilla/sessions pre-dates most vendoring tools and I suspect the intersection of those who use it and don't vendor is therefore very high.

elithrar · 2017-01-29T00:37:38Z

Take note of #105 - we may just fix it there.

seh · 2017-07-22T02:19:09Z

sessions.go

@@ -186,7 +184,9 @@ func init() {

 // Save saves all sessions used during the current request.
 func Save(r *http.Request, w http.ResponseWriter) error {
-	return GetRegistry(r).Save(w)
+	var reg *Registry
+	reg, r = GetRegistry(r)


If there was no registry saved in the request's context, we shouldn't create a new, empty one here, only to throw it away after telling it to save nothing. Instead, separate GetRegistry into its two cases—one that returns an existing registry if it already exists, and one that wraps it and installs a fresh registry if there wasn't one before. This function should use the former and bail early if there was no existing registry.

stale · 2019-06-30T02:15:04Z

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

elithrar self-assigned this Sep 21, 2016

elithrar added enhancement question and removed question labels Sep 21, 2016

elithrar added the breaking change label Sep 23, 2016

elithrar reviewed Sep 25, 2016

View reviewed changes

elithrar suggested changes Sep 25, 2016

View reviewed changes

#80 add context_legacy.go and context.go for go 1.7 migration

ac11544

#80 add note in README about GetRegistry change in Go 1.7

dc17d8c

seh reviewed Jul 22, 2017

View reviewed changes

elithrar added the proposal label Jun 8, 2018

stale bot added the stale label Jun 30, 2019

shawnps closed this Jun 30, 2019

shawnps deleted the go17-context branch June 30, 2019 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#80 add context_legacy.go and context.go for go 1.7 migration #95

#80 add context_legacy.go and context.go for go 1.7 migration #95

shawnps commented Sep 21, 2016 •

edited

Loading

elithrar commented Sep 21, 2016

shawnps commented Sep 21, 2016

elithrar commented Sep 22, 2016

shawnps commented Sep 23, 2016

elithrar commented Sep 23, 2016

elithrar commented Sep 23, 2016

shawnps commented Sep 23, 2016

elithrar commented Sep 23, 2016

elithrar Sep 25, 2016

elithrar left a comment

elithrar Sep 25, 2016

elithrar Sep 25, 2016 •

edited

Loading

elithrar commented Sep 25, 2016

shawnps commented Sep 25, 2016 •

edited

Loading

elithrar commented Sep 25, 2016

shawnps commented Sep 25, 2016

shawnps commented Sep 25, 2016

shawnps commented Sep 25, 2016

elithrar commented Sep 25, 2016 •

edited

Loading

shawnps commented Sep 25, 2016

shawnps commented Oct 3, 2016

elithrar commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

nitingupta910 commented Oct 31, 2016

elithrar commented Nov 1, 2016

omeid commented Jan 20, 2017

elithrar commented Jan 22, 2017

elithrar commented Jan 29, 2017

seh Jul 22, 2017

stale bot commented Jun 30, 2019

#80 add context_legacy.go and context.go for go 1.7 migration #95

#80 add context_legacy.go and context.go for go 1.7 migration #95

Conversation

shawnps commented Sep 21, 2016 • edited Loading

elithrar commented Sep 21, 2016

shawnps commented Sep 21, 2016

elithrar commented Sep 22, 2016

shawnps commented Sep 23, 2016

elithrar commented Sep 23, 2016

elithrar commented Sep 23, 2016

shawnps commented Sep 23, 2016

elithrar commented Sep 23, 2016

elithrar Sep 25, 2016

Choose a reason for hiding this comment

elithrar left a comment

Choose a reason for hiding this comment

elithrar Sep 25, 2016

Choose a reason for hiding this comment

elithrar Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

elithrar commented Sep 25, 2016

shawnps commented Sep 25, 2016 • edited Loading

elithrar commented Sep 25, 2016

shawnps commented Sep 25, 2016

shawnps commented Sep 25, 2016

shawnps commented Sep 25, 2016

elithrar commented Sep 25, 2016 • edited Loading

shawnps commented Sep 25, 2016

shawnps commented Oct 3, 2016

elithrar commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

shawnps commented Oct 3, 2016

nitingupta910 commented Oct 31, 2016

elithrar commented Nov 1, 2016

omeid commented Jan 20, 2017

elithrar commented Jan 22, 2017

elithrar commented Jan 29, 2017

seh Jul 22, 2017

Choose a reason for hiding this comment

stale bot commented Jun 30, 2019

shawnps commented Sep 21, 2016 •

edited

Loading

elithrar Sep 25, 2016 •

edited

Loading

shawnps commented Sep 25, 2016 •

edited

Loading

elithrar commented Sep 25, 2016 •

edited

Loading