Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Serialize the `last_request_at` entry as an Integer #2954

Merged
merged 1 commit into from

3 participants

@lucasmazza
Owner

Possible solution for #2930.

My first though was of adding support for timedout? to receive Strings, but this logic really belongs to the callback. Thinking out loud on Campfire I realized that storing the object as an integer would make more sense than bending Strings around, but I don't know if this change might break existing session data on apps out there.

Regarding to the testing bits, the existing integration tests felt a bit pointless due the semantics of numbers vs dates - between requests not just the values are the same, but the objects in memory too.

I wanted to cover this better with a unit test for the callback code, but the current state of just passing the blocks to Warden makes it impossible. What do you guys think of extracting these blocks to classes like Devise::Hooks::Timeoutable instead? I can do this on another Pull Request.

@lucasmazza lucasmazza Serialize the `last_request_at` entry as an Integer
Pushing the `Time` object inside the session has inconsistencies
across different serializers and we should use a more primitive type
so we don't need any specific parsing logic for the JSON serializer.
da0c273
@rafaelfranca rafaelfranca commented on the diff
lib/devise/hooks/timeoutable.rb
@@ -9,6 +9,11 @@
if record && record.respond_to?(:timedout?) && warden.authenticated?(scope) && options[:store] != false
last_request_at = warden.session(scope)['last_request_at']
+
+ if last_request_at.is_a? Integer
@rafaelfranca Collaborator

Will YAML and JSON read this values as integer? I thought JSON read as string.

@lucasmazza Owner

JSON support numbers too. If we want to store this as a String we would have to cast it back to Integer before casting it to a Time :runner:

@rafaelfranca Collaborator

Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim josevalim commented on the diff
lib/devise/hooks/timeoutable.rb
@@ -9,6 +9,11 @@
if record && record.respond_to?(:timedout?) && warden.authenticated?(scope) && options[:store] != false
last_request_at = warden.session(scope)['last_request_at']
+
+ if last_request_at.is_a? Integer
+ last_request_at = Time.at(last_request_at).utc
@josevalim Owner

I think Time.at always returns the value in UTC. :)

@lucasmazza Owner

nope :/

require 'time'
now = Time.now.utc
# 2014-03-29 13:52:18 UTC
Time.at(now.to_i)
# 2014-03-29 10:52:18 -0300
Time.at(now.to_i).utc
# 2014-03-29 13:52:18 UTC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim
Owner

:shipit:

@lucasmazza lucasmazza merged commit 6027787 into master

1 check passed

Details default The Travis CI build passed
@lucasmazza lucasmazza deleted the lm-last-request-at-json-serialization branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 29, 2014
  1. @lucasmazza

    Serialize the `last_request_at` entry as an Integer

    lucasmazza authored
    Pushing the `Time` object inside the session has inconsistencies
    across different serializers and we should use a more primitive type
    so we don't need any specific parsing logic for the JSON serializer.
This page is out of date. Refresh to see the latest.
View
7 lib/devise/hooks/timeoutable.rb
@@ -9,6 +9,11 @@
if record && record.respond_to?(:timedout?) && warden.authenticated?(scope) && options[:store] != false
last_request_at = warden.session(scope)['last_request_at']
+
+ if last_request_at.is_a? Integer
@rafaelfranca Collaborator

Will YAML and JSON read this values as integer? I thought JSON read as string.

@lucasmazza Owner

JSON support numbers too. If we want to store this as a String we would have to cast it back to Integer before casting it to a Time :runner:

@rafaelfranca Collaborator

Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ last_request_at = Time.at(last_request_at).utc
@josevalim Owner

I think Time.at always returns the value in UTC. :)

@lucasmazza Owner

nope :/

require 'time'
now = Time.now.utc
# 2014-03-29 13:52:18 UTC
Time.at(now.to_i)
# 2014-03-29 10:52:18 -0300
Time.at(now.to_i).utc
# 2014-03-29 13:52:18 UTC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
proxy = Devise::Hooks::Proxy.new(warden)
if record.timedout?(last_request_at) && !env['devise.skip_timeout']
@@ -22,7 +27,7 @@
end
unless env['devise.skip_trackable']
- warden.session(scope)['last_request_at'] = Time.now.utc
+ warden.session(scope)['last_request_at'] = Time.now.utc.to_i
end
end
end
View
3  test/integration/timeoutable_test.rb
@@ -8,12 +8,11 @@ def last_request_at
test 'set last request at in user session after each request' do
sign_in_as_user
- old_last_request = last_request_at
assert_not_nil last_request_at
+ @controller.user_session.delete('last_request_at')
get users_path
assert_not_nil last_request_at
- assert_not_equal old_last_request, last_request_at
end
test 'set last request at in user session after each request is skipped if tracking is disabled' do
Something went wrong with that request. Please try again.