Fix for RedisSession.created #37

whiteroses · 2014-04-30T02:45:01Z

The three commits:

Change `RedisSession.created` to represent the time the session was first created, rather than the time this particular session instance was initialised.

The tuple approach follows the one in Pyramid (https://github.com/Pylons/pyramid/blob/master/pyramid/session.py#L368), and keeps the value out of the session dict. If you think that's okay, I think it would also make sense to follow the same approach with the timeout value?

Re-arrange imports as we discussed.

PEP 8 mostly, and in test-related code place less dependable imports in test methods wherever possible.

Remove support for Python 2.6 from setup.py as we discussed.

Do you also want to add support for Python 3.4? I think there's a bug with Nose that breaks something with Python 3.4 at the moment, but the tests pass with python -m unittest.

Please let me know if there's anything you need me to change. Thanks!

Change RedisSession.created to represent the time the session was first created, rather than the time this particular session *instance* was initialised.

whiteroses · 2014-04-30T12:47:54Z

Please hold on the merge for now (though feel free to take a look at it and see if you like the changes) -- I got to spend a bit more time with the code mmerickel showed me yesterday, and there are a couple of issues re: backward compatibility and exception handling that we would need to discuss. I'll link to his gist in the invalidate issue thread and explain more there in a bit. Thanks!

Update some docstrings that should have been updated following the change from persisting only the session dict to persisting a tuple.

…uple. Move timeout out of managed_dict, as it is metadata that is not part of the session. Previously any timeout set via .adjust_timeout_for_session() would be lost after .clear(), for example. Serialise session dict, created time, and timeout for storage in Redis as a dict instead of a tuple, as a dict is more extensible, more readable and less bug-prone.

This fix adds support for the use of a new session within the same request after an .invalidate() call. invalidate() now deletes the session_id key from Redis, and RedisSession now knows to start a new session with a new session_id when it is needed. (Previously invalidate() only cleared the session dict and set a header to delete the cookie, and did not take into account the use of the session after invalidate().) It also fixes the cookie setting after invalidate(). Previously invalidate() just added another Set-Cookie header to the response to delete the cookie, meaning there could be more than one Set-Cookie headers for the same cookie name, which should not happen according to RFC 6265. This has been fixed to set one and only one Set-Cookie header (if any is needed) in the response callback, and to handle the various combinations of whether the request started with a new session or existing session, whether a new session was used after invalidate(), more than one invalidate()s in the same request, and the cookie_on_exception setting.

The signed secret being bad is just one of the possible reasons why the value cannot be deserialized.

whiteroses · 2014-05-11T18:30:47Z

More commits, and finally, the invalidate fix (#33)!

With many thanks to mmerickel for the SessionState idea.

Some notes and questions:

If request.session starts with an existing session, we invalidate(), then
create a new session (existing session -> invalidate() -> new session), with
cookie_on_exception False and exception raised -- what should be the effect on
the cookie? I have gone with deleting the cookie, as I thought invalidate()
should mean a delete of the existing session no matter what cookie_on_exception
is. Please let me know if that's not right, although I don't see what the point
would be to not delete a session cookie whose session_id has been deleted from
Redis by the invalidate().

On the whole cookie_on_exception's purpose still seems really unclear to me. I
would totally get the purpose of a cookie_on_exception setting for a
client-side cookie-based session, because it would mean "Do we persist changes
to session within this request if an exception was raised during it?" But not
with pyramid_redis_sessions, because the cookie is only part of the session
persistence (the setting doesn't affect changes in Redis), if you see what I
mean.

Right now the cookie callback makes its decision based on the
RedisSession._invalidated property. This seems simpler than the cookie flag
approach (keep a flag value in the factory, pass in functions into RedisSession
that set the flag, and then the callback to make the decision based on the
flag), but what do you think?

The implementation of the _cookie_callback may not be very easy to understand
on first reading, but it makes more sense if you lay out a table of all the
possibilities. I had an alternative implementation whose logic might have been
a little easier to understand, but it was longer and the if-branching went
deeper, so I thought it was better to refactor it into the current one. Also
made various attempts at explaining things with docstring and comments, but
they only seemed to make things seem more complicated. The cases and expected
behaviour are clearly laid out in the tests though. How does it look to you?
Would be happy to add any comments you think would help make things clearer or
easier to read.

Is RedisSession.default_timeout intended to be part of the API? I don't think
it is, but it was previously set in __init__ (it was used as a backup default
for the timeout property, but it is not needed for that any more), and it seems
useful (for example, the user may want to reset timeout back to default) so I
kept it around. If you think it's worth keeping, the timeout parameter of
RedisSession should really be called default_timeout, as it is now only used to
set the .default_timeout attribute (the actual timeout is now inserted into
Redis and pulled out of Redis as part of the persisted dict). If not, I think the
timeout parameter should be removed completely? Either way, if you let me
know which you would like, I can make the changes and add the commit to this
pull request.

In test_session.py, I split makeOne() into set_up_session_in_redis() and
makeOne() because I needed to be able to specify values for things going into
Redis, and it didn't seem right to add them to the parameters of makeOne(),
when the "One" in makeOne is RedisSession. I hope that is okay.

I hope the rest makes sense, and please let me know if there's anything you'd
like me to change or clarify. Thanks!

ericrasmussen · 2014-05-13T03:45:47Z

Just a quick update: I started reviewing this and so far so good. Need to wrap up some coursework tonight but will address your questions by tomorrow night at the latest. Thanks!

whiteroses · 2014-05-14T00:31:37Z

Appreciate it! (And don't worry about taking a bit longer if you're busy, I already really appreciate how responsive you've been :) )

ericrasmussen · 2014-05-14T04:47:16Z

Answers --

I agree that the purpose of cookie_on_exception is unclear. When I first worked on this I wanted to accommodate all the current session use cases so I carried that flag over from beaker (and the never released zodb_sessions) even though it probably isn't needed here. I think your choice here of deleting on invalidate regardless is fine though. If someone has a use case where they don't want that to happen we can always consider adding a delete_on_invalidate flag, but I see no reason to bother at this point.
Looks clear to me. I'll probably add in a few comments but nothing stood out with the code.
The only intended APIs for timeout are a) setting timeout when you create the session (the timeout key), or b) changing it with adjust_timeout_for_session. The default_timeout was only used to choose between the timeout parameter and the adjusted timeout, so it doesn't need to stay. I don't really mind either way in terms of the implementation as long as people can still configure a timeout setting in their config (or by calling the session factory directly) and can adjust it with adjust_timeout_for_session.
I can't think of any issues with this.

Anyway, looks great! I'm going to take a couple days to review in more detail and test, but I don't foresee any major changes to what's there.

whiteroses · 2014-05-14T23:00:08Z

Thanks for reviewing! As you don't mind, I think we can remove the timeout parameter and the default_timeout attribute from RedisSession -- if we need the default value we can get it from request.registry.settings. I'll have the changes for you either tomorrow or Friday.

They are no longer needed now that the timeout is inserted into Redis by the factory at the beginning of a new session.

whiteroses · 2014-05-15T22:41:58Z

Added commit to remove RedisSession's timeout parameter and default_timeout attribute, and update tests. DummySession still has a timeout parameter, but that's okay, right?

ericrasmussen · 2014-05-18T05:41:20Z

Spent some time reviewing the changes today and the interactive tests seem fine (me poking at it in a sample pyramid app), but the tests are failing for me with pyramid==1.4.1 and WebOb==1.2.3, on both python 2.7 and python 3.3.

Here's a gist with the output: https://gist.github.com/ericrasmussen/481f036738c4b5f6c305

The issue seems to be pyramid.session.signed_serialize producing session IDs that end with something like 'VxAS4=' and webob adding a header with that same key as 'VxAS4\075'.

'=' is 075 in octal so I'm guessing something in the test case isn't handling a conversion that seems to work fine in an actual pyramid app.

Have you seen this error? If not, can you let me know what versions of pyramid and webob you're testing on?

Thanks!

whiteroses · 2014-05-18T09:09:55Z

The tests fail for me too on WebOb==1.2.3, but they pass fine on WebOb>=1.3. This looks to be the issue:

https://github.com/Pylons/webob/blob/master/docs/news.txt#L63

"- Cookies generated via webob.cookies quoted characters in cookie values that
did not need to be quoted per RFC 6265. The following characters are no
longer quoted in cookie values: ~/=<>()[]{}?@ . The full set of
non-letter-or-digit unquoted cookie value characters is now
!#$%&'*+-.^_|~/: =<>()[]{}?@`. See
http://tools.ietf.org/html/rfc6265#section-4.1.1 for more information."

With WebOb==1.2.3, the tests fail because we are comparing the cookie values with '=' quoted by response.set_cookie to values that have not been quoted by response.set_cookie. So if you upgrade to WebOb>=1.3, all the tests will pass.

I'm thinking about whether it's worth involving response.set_cookie in some way when generating the value we are checking against (instead of just self._serialize), but not seeing a simple way of doing it yet. Alternatively, we could just not assert the serialized value, and instead assert that "Max-Age" is not in the header value to indicate that it's a set cookie instead of a delete. Not ideal though. What do you think?

ericrasmussen · 2014-05-20T04:24:16Z

I'm actually starting to think we should just require WebOb >= 1.3 in
testing_requires in setup.py. We only support Pyramid >= 1.3, which
requires WebOb >= 1.2. If someone did depend on an earlier version for
testing their app, they could always create a new virtualenvironment for
pyramid_redis_sessions, and the WebOb dependency would only potentially be
upgraded if they install with testing enabled.

Can you think of anything I'm missing? If not, let's just add that to
testing_rquires at >= 1.3.

On Sun, May 18, 2014 at 2:09 AM, Ira [email protected] wrote:

The tests fail for me too on WebOb==1.2.3, but they pass fine on
WebOb>=1.3. This looks to be the issue:

https://github.com/Pylons/webob/blob/master/docs/news.txt#L63

"- Cookies generated via webob.cookies quoted characters in cookie values
that
did not need to be quoted per RFC 6265. The following characters are no
longer quoted in cookie values: /=<>()[]{}?@ . The full set of
non-letter-or-digit unquoted cookie value characters is now
!#$%&'*+-.^_`|/: =<>()[]{}?@. See
http://tools.ietf.org/html/rfc6265#section-4.1.1 for more information."

With WebOb==1.2.3, the tests fail because we are comparing the cookie
values with '=' quoted by response.set_cookie to values that have not
been quoted by response.set_cookie. So if you upgrade to WebOb>=1.3, all
the tests will pass.

I'm thinking about whether it's worth involving response.set_cookie in
some way when generating the value we are checking against (instead of just
self._serialize), but not seeing a simple way of doing it yet.
Alternatively, we could just not assert the serialized value, and
instead assert that "Max-Age" is not in the header value to indicate that
it's a set cookie instead of a delete. Not ideal though. What do you think?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-43434972
.

whiteroses · 2014-05-21T23:01:38Z

Update:

Found two bugs after we talked yesterday:

cookie_path parameter to factory is not actually used by the factory at all. Both set_cookie and delete_cookie should be setting the path to that value.
delete_cookie doesn't use the cookie_domain value, and it needs to, because the cookie only gets deleted if the path and domain in the header match the ones used when the cookie was set.
(These are from before my changes.)

Will fix those too. In light of these bugs I think we really ought to add tests to check that those cookie_ parameters are actually used by set_cookie and delete_cookie. Is that okay?

As for the assert problem, neither of the negative assertion options we looked at yesterday inspire confidence. Max-Age is case-insensitive, so we would need to .lower() the header value before we assert, and even then... just so fragile. session=; turns out not to be reliable either, as the value doesn't have to be an empty string for a delete cookie (it is an empty string with WebOb, but apparently it doesn't have to be).

So here's the best approach available to us that I can see: make another webob.Response() object in a helper method, call its .set_cookie() with the same values, and compare the resulting header with the one set by the factory. It works, is not fragile, wouldn't break from past or future changes to set_cookie() (not minor ones anyway), but is longer -- I'm going to see if I can keep most of that in a helper method so it doesn't make the tests too long. But it works.

If you're okay with those changes, let me know and I'll go ahead with them. May not have time tomorrow, but can have them for you by Friday (barring more issues coming up...)

``cookie_httponly``'s default is True, but docstring had it as False.

It was already accessible as an import.

To assert that a header is one that sets a cookie as opposed to one that deletes a cookie, we no longer assert the serialized session_id. This means the tests would not break if `response.set_cookie()` makes any changes to the serialized session_id before inserting it into the cookie, as WebOb<=1.3 did by quoting certain characters. The alternative approach chosen is to assert that 'Max-Age=0' is not in the header string. A Set-Cookie header is either one to set cookie (in which case 'Max-Age=0' would not be in the string), or one to delete cookie (in which case 'Max-Age=0' would be in the string.) It is a negative assertion, but all the alternatives would make the tests too complicated and/or difficult to make sense of. This is the least bad approach I can see for now.

Rearrange the ordering to be consistent with the factory's own parameters and docstring, so that any missing parameters would be easier to spot.

`cookie_path` parameter had no effect because it was not used by the factory at all. We now set and delete cookie with its value as intended.

The factory was not making use of the `cookie_domain` value when setting a header to delete cookie, meaning any cookie with a domain different from the default would not have been deleted (as a cookie only gets deleted if the path and domain match the ones used when the cookie was set).

whiteroses · 2014-05-25T21:08:46Z

Turned out even the approach of making a new webob.Response object and using its .set_cookie() and .delete_cookie() to come up with expected headers to assert against had its problem: the two headers may have slightly different Expires value due to one being generated after the other. Spent a lot of time trying to find a better approach for those asserts, but still couldn't avoid the negative assertion of assertNotIn('Max-Age=0'...) and the use of regex to get around the Expires problem -- all the other alternatives I tried were too complicated and/or made the tests too difficult to read. I'm not happy with the negative assertion or the regex (or how long a couple of the tests are), but they seem to be the least bad options available. If you see anything that can be improved or that you want me to change, please let me know.

With that first commit where I fixed the default value of cookie_httponly in the docstring (now I see I made a typo in the commit message, cookie_httpOnly should be cookie_httponly, could you please fix it if you think it matters? Sorry about that.): I assumed the current default value of True was what was intended, let me know if that's not right?

whiteroses · 2014-05-27T23:09:40Z

Just wanted to clarify, with the typo, I just wasn't sure if I could change it myself now that I've pushed, whether it would mess up this pull request. Can I do it myself? (rebase -i and push -f?)

ericrasmussen · 2014-05-28T04:41:35Z

I think you can still rebase. Whatever's on your fork and branch is what will be in the pull request. But the typo doesn't seem like an issue to me.

The test looks like it should be good too. Will try to wrap up a couple things this week so I can merge and then add to the docs and change log. Thanks!

whiteroses · 2014-05-28T23:29:09Z

If you don't mind the typo, I'll let it go. It's not a big deal, not worth the risk (it's probably fine to rebase as you said, but I'll test that another time!)

I can write up a list of the changes I've made, if that would help?

whiteroses · 2014-06-15T22:13:19Z

I updated the changelog as we talked about and tried to follow the existing format, but please feel free to edit/reword/reformat to your liking! You may want to take a look at this other issue (#38) before release -- as mmerickel mentioned that would be another change that would break all existing sessions, so you may want to do both in the same release. Thanks!

whiteroses · 2014-06-15T22:21:51Z

Also, is the changelog looking as wrong for you as it does for me on GitHub? Is that GitHub, or something in the markup that we can fix?

ericrasmussen · 2014-06-16T04:54:44Z

GitHub's rst renderer is a little weird so it's more or less expected. You can try building the change log with sphinx to double check but it's not a priority to spend time on that, as long as it's fairly easy to read. The updates look good.

I'm going to merge now and then work on the other release bits this week. Thanks so much for all the great work!

Fix for RedisSession.created

ericrasmussen · 2014-07-04T20:04:43Z

FYI -- all of your fixes and updates (plus a couple tiny misc things) are
in the latest release on pypi. If you catch any issues let me know, and
thanks again for everything!

On Sun, Jun 15, 2014 at 3:21 PM, Ira [email protected] wrote:

Also, is the changelog looking as wrong for you as it does for me on
GitHub? Is that GitHub, or something in the markup that we can fix?

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

whiteroses · 2014-07-05T22:52:34Z

Thanks very much for the release! (Hope that means you and your toddler have been getting better sleep! :) )

You may remember I mentioned back in #33 (comment) that we can replace the use of pipeline with the new parameters for .set(), introduced in Redis 2.6.12 and redis-py 2.7.4 (you just changed the required version of redis-py from >= 2.4.11 to <=2.9.1 -- I guess the lower bound is far enough back that we don't have to worry about it any more?) I think it would be a really worthwhile change, and should be fairly straightforward -- I can maybe work on a PR for you some time next week and you can merge as and when you're ready to drop support for Redis < 2.6.12 and redis-py < 2.7.4?

Also, while I was working on the previous PR, I noticed that in test_util.py, Test_get_unique_session_id._makeGenerator() could be replaced with itertools.count(start=1, step=1), and we wouldn't have to use global any more. This may be purely personal preference though, so feel free to ignore if it doesn't matter to you :)

And reading over some of my own code comments and docstrings now, I may also submit a PR for minor rewordings, just to make the comments clearer in a couple places, if that's okay. Sorry about that -- probably over-worrying on my part, but I think they could be clearer, and I feel responsible :)

ericrasmussen · 2014-07-06T19:59:15Z

PRs for everything you mentioned are welcome!

About the version pinning, being more specific is probably better. I'm not
sure if you saw redis/redis-py#510 but with
redis-py > 2.9.1 we were seeing inconsistencies that have been fixed in
their latest master.

I'm not sure what the best solution is there. They're going to issue
deprecation warnings for two of the keyword arguments we use, so it might
be better to wait til their next release and pin at redis-py >= 2.10. It
would mean another breaking change, but we are still in alpha, and I don't
want to support multiple ways of doing things unless people really do need
earlier versions of redis-py.

On Sat, Jul 5, 2014 at 3:52 PM, Ira [email protected] wrote:

Thanks very much for the release! (Hope that means you and your toddler
have been getting better sleep! :) )

You may remember I mentioned back in #33 (comment)
#33 (comment)
that we can replace the use of pipeline with the new parameters for .set(),
introduced in Redis 2.6.12 and redis-py 2.7.4 (you just changed the
required version of redis-py from >= 2.4.11 to <=2.9.1 -- I guess the lower
bound is far enough back that we don't have to worry about it any more?) I
think it would be a really worthwhile change, and should be fairly
straightforward -- I can maybe work on a PR for you some time next week and
you can merge as and when you're ready to drop support for Redis < 2.6.12
and redis-py < 2.7.4?

Also, while I was working on the previous PR, I noticed that in
test_util.py, Test_get_unique_session_id._makeGenerator() could be
replaced with itertools.count(start=1, step=1), and we wouldn't have to
use global any more. This may be purely personal preference though, so
feel free to ignore if it doesn't matter to you :)

And reading over some of my own code comments and docstrings now, I may
also submit a PR for minor rewordings, just to make the comments clearer in
a couple places, if that's okay. Sorry about that -- probably over-worrying
on my part, but I think they could be clearer, and I feel responsible :)

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

whiteroses · 2014-07-09T00:22:40Z

I saw the submitted issue and PR over here, so I understood why you pinned to <= 2.9.1 -- was just wondering whether you meant to drop the >=2.4.11 part?

I hadn't seen the response you got over on the redis-py side though. So if I understand everything correctly: the breaking changes are here to stay. And from what I can see, pyramid_redis_sessions is just passing those values on. It seems to me that it's not pyramid_redis_sessions' responsibility at all, and if anything we could just not and maybe should not specify socket_timeout, connection_pool, charset/encoding, errors/encoding_errors and unix_socket_path in the factory's parameters at all or be giving them default values. Instead, just extend the redis_options dict with all left over factory **kwargs? (But document in the docstring that those are optional parameters whose values will be passed on to redis-py, and let redis-py handle any errors or deprecation warnings.) I feel instinctively a bit iffy about using the **kwargs that way because it seems like it might get in the way of using the kwargs for other things in future, but we could always pop those other values we need out of the kwargs, then pass the rest on to redis-py, if it comes to that.

Once redis-py 2.10.2 comes out, we can stop pinning to <= 2.9.1 and users using charset and errors will get deprecation warnings from redis-py. But encoding and encoding_errors won't work, and we'll need to make some kind of change for that. What do you think about the **kwargs solution above? I haven't had as much time to look over this as I'd like, but wanted to give you a response -- have I missed anything?

(The other PRs should be coming later in the week! :) )

ericrasmussen · 2014-07-12T19:29:30Z

Dropping >= 2.4.11 is just an oversight. If you can add that back in with
your PR that'd be great.

If I understand the kwargs solution you're proposing, it would work, and
the best argument for it is not having to deal with this in the future
(we'd just refer people to the redis-py docs for whatever version they
have). The tradeoff though is giving up a lot of the control we have now,
and since it's only a few settings I'm inclined to keep it as-is and
provide docs and code that people can use without needing to learn anything
about redis-py.

I see two other solutions:

wait for the 2.10.2 release and make it the new lower bound, updating
our settings to match theirs. Upside is one way of doing things that will
match their way of doing things, downside is yet another backwards
incompatible change, and not everyone might be able to upgrade redis-py if
they're relying on older versions for use with other libraries
(dogpile.cache, various redis queues, etc).
Allow both the old and new setting names and choose which ones to pass
to redis-py based on its version.

The second option isn't that fun from a code or maintenance standpoint, but
I think ultimately much better for the users, because they don't need to
worry about breaking changes.

I don't think I'll be able to get to any of it this weekend, but let me
know what you think. Thanks!

On Tue, Jul 8, 2014 at 5:22 PM, Ira [email protected] wrote:

I saw the submitted issue and PR over here, so I understood why you pinned
to <= 2.9.1 -- was just wondering whether you meant to drop the >=2.4.11
part?

I hadn't seen the response you got over on the redis-py side though. So if
I understand everything correctly: the breaking changes are here to stay.
And from what I can see, pyramid_redis_sessions is just passing those
values on. It seems to me that it's not pyramid_redis_sessions'
responsibility at all, and if anything we could just not and maybe should
not specify socket_timeout, connection_pool, charset/encoding, errors/
encoding_errors and unix_socket_path in the factory's parameters at all
or be giving them default values. Instead, just extend the redis_options
dict with all left over factory *_kwargs? (But document in the docstring
that those are optional parameters whose values will be passed on to
redis-py, and let redis-py handle any errors or deprecation warnings.) I
feel instinctively a bit iffy about using the *_kwargs that way because it
seems like it mi ght get in the way of using the kwargs for other things in
future, but we could always pop those other values we need out of the
kwargs, then pass the rest on to redis-py, if it comes to that.

Once redis-py 2.10.2 comes out, we can stop pinning to <= 2.9.1 and users
using charset and errors will get deprecation warnings from redis-py. But
encoding and encoding_errors won't work, and we'll need to make some kind
of change for that. What do you think about the **kwargs solution above? I
haven't had as much time to look over this as I'd like, but wanted to give
you a response -- have I missed anything?

(The other PRs should be coming later in the week! :) )

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

whiteroses · 2014-07-13T18:50:43Z

Yeah, the kwargs solution gives up the control, and I understand your reservations about that. Of course if you don't mind supporting both versions of the settings and having to watch redis-py closely for changes (for example, if redis-py changes the default value for a parameter, you'd have to notice and change yours too), then that would be best for your users :) Option 1 on the other hand seems like it would mean people wouldn't be able to upgrade if they're stuck on anything but the most recent versions of redis-py, so that doesn't seem a great option.

If you don't have time to do option 2 yet, the kwargs option would as far as I can see be good enough as a stopgap to get everything working for everyone until you do.

I submitted the other changes as separate PRs, as last time the PR really got quite huge and unwieldy. Hope that's okay!

ericrasmussen · 2014-07-14T01:51:49Z

Thanks! Started merging them but will probably take me a few days to get
through the rest. So far so good though. I really appreciate it.

On Sun, Jul 13, 2014 at 11:50 AM, Ira [email protected] wrote:

Yeah, the kwargs solution gives up the control, and I understand your
reservations about that. Of course if you don't mind supporting both
versions of the settings and having to watch redis-py closely for changes
(for example, if redis-py changes the default value for a parameter, you'd
have to notice and change yours too), then that would be best for your
users :) Option 1 on the other hand seems like it would mean people
wouldn't be able to upgrade if they're stuck on anything but the most
recent versions of redis-py, so that doesn't seem a great option.

If you don't have time to do option 2 yet, the kwargs option would as far
as I can see be good enough as a stopgap to get everything working for
everyone until you do.

I submitted the other changes as separate PRs, as last time the PR really
got quite huge and unwieldy. Hope that's okay!

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

whiteroses added 3 commits April 30, 2014 03:00

Fix RedisSession.created.

6471e9d

Change RedisSession.created to represent the time the session was first created, rather than the time this particular session *instance* was initialised.

Re-arrange imports.

9334f57

Remove support for Python 2.6.

ccf5d5b

whiteroses added 5 commits May 1, 2014 17:24

Update some docstrings.

a207c3e

Update some docstrings that should have been updated following the change from persisting only the session dict to persisting a tuple.

Make minor adjustments to imports.

70f4042

Small correction to docstring.

e377ab4

The signed secret being bad is just one of the possible reasons why the value cannot be deserialized.

Remove RedisSession's timeout parameter and default_timeout attribute.

6271916

They are no longer needed now that the timeout is inserted into Redis by the factory at the beginning of a new session.

whiteroses added 7 commits May 25, 2014 19:45

Correct default value of cookie_httpOnly in docstring.

fc00c40

``cookie_httponly``'s default is True, but docstring had it as False.

Remove unnecessary signed_serialize parameter.

1455752

It was already accessible as an import.

Add tests for cookie-related factory parameters.

a2944f0

Rearrange ordering of cookie_ parameters.

20576e1

Rearrange the ordering to be consistent with the factory's own parameters and docstring, so that any missing parameters would be easier to spot.

Fix bug where cookie_path parameter had no effect.

75b324e

`cookie_path` parameter had no effect because it was not used by the factory at all. We now set and delete cookie with its value as intended.

Update changelog.

59f65a4

ericrasmussen added a commit that referenced this pull request Jun 16, 2014

Merge pull request #37 from whiteroses/fix.session_created

8e7948e

Fix for RedisSession.created

ericrasmussen merged commit 8e7948e into ericrasmussen:master Jun 16, 2014

ericrasmussen mentioned this pull request Jul 3, 2014

support clear() and new cookie signing #38

Open

whiteroses deleted the fix.session_created branch July 5, 2014 21:22

This was referenced Jul 13, 2014

Restore redis-py install_requires lower bound. #45

Merged

Replace generator with itertools.count(). #47

Merged

Make use of StrictRedis.set() parameters ex and nx. #48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for RedisSession.created #37

Fix for RedisSession.created #37

whiteroses commented Apr 30, 2014

whiteroses commented Apr 30, 2014

whiteroses commented May 11, 2014

ericrasmussen commented May 13, 2014

whiteroses commented May 14, 2014

ericrasmussen commented May 14, 2014

whiteroses commented May 14, 2014

whiteroses commented May 15, 2014

ericrasmussen commented May 18, 2014

whiteroses commented May 18, 2014

ericrasmussen commented May 20, 2014

whiteroses commented May 21, 2014

whiteroses commented May 25, 2014

whiteroses commented May 27, 2014

ericrasmussen commented May 28, 2014

whiteroses commented May 28, 2014

whiteroses commented Jun 15, 2014

whiteroses commented Jun 15, 2014

ericrasmussen commented Jun 16, 2014

ericrasmussen commented Jul 4, 2014

whiteroses commented Jul 5, 2014

ericrasmussen commented Jul 6, 2014

whiteroses commented Jul 9, 2014

ericrasmussen commented Jul 12, 2014

whiteroses commented Jul 13, 2014

ericrasmussen commented Jul 14, 2014

Fix for RedisSession.created #37

Fix for RedisSession.created #37

Conversation

whiteroses commented Apr 30, 2014

Change RedisSession.created to represent the time the session was first created, rather than the time this particular session instance was initialised.

Re-arrange imports as we discussed.

Remove support for Python 2.6 from setup.py as we discussed.

whiteroses commented Apr 30, 2014

whiteroses commented May 11, 2014

ericrasmussen commented May 13, 2014

whiteroses commented May 14, 2014

ericrasmussen commented May 14, 2014

whiteroses commented May 14, 2014

whiteroses commented May 15, 2014

ericrasmussen commented May 18, 2014

whiteroses commented May 18, 2014

ericrasmussen commented May 20, 2014

whiteroses commented May 21, 2014

whiteroses commented May 25, 2014

whiteroses commented May 27, 2014

ericrasmussen commented May 28, 2014

whiteroses commented May 28, 2014

whiteroses commented Jun 15, 2014

whiteroses commented Jun 15, 2014

ericrasmussen commented Jun 16, 2014

ericrasmussen commented Jul 4, 2014

whiteroses commented Jul 5, 2014

ericrasmussen commented Jul 6, 2014

whiteroses commented Jul 9, 2014

ericrasmussen commented Jul 12, 2014

whiteroses commented Jul 13, 2014

ericrasmussen commented Jul 14, 2014

Change `RedisSession.created` to represent the time the session was first created, rather than the time this particular session instance was initialised.