Skip to content

Upgrade to Rack 3 #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
8826905
Changed rack version dependency so that it works with newer versions …
Apr 18, 2017
c3f2194
Fixed issue where session item is found with no data.
Apr 21, 2017
19e175c
Fixed issue where error occurs when deleting a session that does not …
Apr 24, 2017
4618999
Merge pull request #13: Add expire_at attribute you can configure TTL…
Apr 25, 2017
af9d8b3
Changed value for exipre_at to a hash
Apr 25, 2017
f0ecb0e
Changed value of n to a string
Apr 25, 2017
2dfa52a
Removed extra space
Apr 25, 2017
10cb346
Added missing brace.
Apr 25, 2017
38cbc9e
Merged pull request #11: Remove expected attributes from deleting ses…
Apr 25, 2017
8969995
Remove expected attributes from delete options.
Sep 8, 2017
aa8526e
Merge commit 'f64696a9fd91a51572982b31e8411de14002528f'
Oct 2, 2017
67f68e7
Updated expire_at method.
Oct 2, 2017
9fad25a
Update aws-sdk version
ryokdy May 11, 2018
8ebe9a2
Merge remote-tracking branch 'jdutil/hotfix/10-rebase'
ryokdy May 15, 2018
789cf9d
Merge remote-tracking branch 'FronteraConsulting/master'
ryokdy May 15, 2018
b807243
Change attribute type of created_at and updated_at to float
ryokdy Jun 26, 2018
5ecb1e0
Make sure DynamoDB record has data attribute
ryokdy Jun 26, 2018
1caddc3
Merge upstream main branch
ryokdy Dec 6, 2021
018e580
Remove .ruby-gemset and .ruby-version
ryokdy May 31, 2023
df4b777
Upgrade rack to 3.0
ryokdy Nov 7, 2023
9f6dafc
Inherit ActionDispatch::Session::AbstractSecureStore instead of Rack:…
ryokdy Nov 8, 2023
238476f
Sid may be nil when testing with RSpec
ryokdy Nov 10, 2023
02c4bb0
Resolve race condition issues when using one-time passwords
ryokdy Nov 16, 2023
555cdb8
Revert 02c4bb0e
ryokdy Nov 20, 2023
791ead7
Remove LockWaitTimeoutError
ryokdy Nov 24, 2023
fdc3806
Merge upstream main branch
ryokdy Nov 24, 2023
2ad11f4
Update Rack version to v2.0.8+
ryokdy Nov 27, 2023
899f68e
Remove debug gem
ryokdy Nov 27, 2023
b6eb684
Remove debug gem
ryokdy Nov 27, 2023
336c2d5
Add support for legacy format session ids
ryokdy Nov 29, 2023
51aa400
Organize alphabetically
ryokdy Nov 29, 2023
b712196
Restore threaded_session_spec
ryokdy Nov 29, 2023
3126844
Change required rack-session to v1.0.1
ryokdy Nov 29, 2023
8cbf185
Fix bug preventing saving of sessions with legacy format sid
ryokdy Nov 29, 2023
bfbe733
Merge branch 'pr33-review'
ryokdy Nov 29, 2023
af58c09
Avoid to create a new sid when writing sessin
ryokdy Nov 29, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 1 addition & 29 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Run the session store as a Rack middleware in the following way:
use Aws::SessionStore::DynamoDB::RackMiddleware.new(options)
run SomeRackApp

Note that `:secret_key` is a mandatory configuration option that must be set.
Note that `:secret_key` is a configuration option that is used for the older version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "older version" (may help users down the road to be more specific).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate it if someone else could revise the README and other texts because I'm not very good at English and I might make misunderstandings. Sorry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries~! I can do that for you when we're done.


## Detailed Usage

Expand Down Expand Up @@ -84,34 +84,6 @@ garbage collection similar to below:
The above example will clear sessions older than one day or that have been
stale for longer than an hour.

### Locking Strategy

You may want the Session Store to implement the provided pessimistic locking
strategy if you are concerned about concurrency issues with session accesses.
By default, locking is not implemented for the session store. You must trigger
the locking strategy through the configuration of the session store. Pessimistic
locking, in this case, means that only one read can be made on a session at
once. While the session is being read by the process with the lock, other
processes may try to obtain a lock on the same session but will be blocked.

Locking is expensive and will drive up costs depending on how it is used.
Without locking, one read and one write are performed per request for session
data manipulation. If a locking strategy is implemented, as many as the total
maximum wait time divided by the lock retry delay writes to the database.
Keep these considerations in mind if you plan to enable locking.

#### Configuration for Locking

The following configuration options will allow you to configure the pessimistic
locking strategy according to your needs:

options = {
:enable_locking => true,
:lock_expiry_time => 500,
:lock_retry_delay => 500,
:lock_max_wait_time => 1
}

### Error Handling

You can pass in your own error handler for raised exceptions or you can allow
Expand Down
4 changes: 3 additions & 1 deletion aws-sessionstore-dynamodb.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ Gem::Specification.new do |spec|
spec.require_paths = ["lib"]

# Require 1.85.0 for user_agent_frameworks config
spec.add_dependency('actionpack', '>= 6.1')
spec.add_dependency 'aws-sdk-dynamodb', '~> 1', '>= 1.85.0'
spec.add_dependency 'rack', '~> 2'
spec.add_dependency 'rack', '>= 2.0.8', '< 4'
spec.add_dependency 'rack-session', '>= 1.0.1'
end
2 changes: 0 additions & 2 deletions lib/aws-sessionstore-dynamodb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ module DynamoDB; end
require 'aws/session_store/dynamo_db/configuration'
require 'aws/session_store/dynamo_db/invalid_id_error'
require 'aws/session_store/dynamo_db/missing_secret_key_error'
require 'aws/session_store/dynamo_db/lock_wait_timeout_error'
require 'aws/session_store/dynamo_db/errors/base_handler'
require 'aws/session_store/dynamo_db/errors/default_handler'
require 'aws/session_store/dynamo_db/garbage_collection'
require 'aws/session_store/dynamo_db/locking/base'
require 'aws/session_store/dynamo_db/locking/null'
require 'aws/session_store/dynamo_db/locking/pessimistic'
require 'aws/session_store/dynamo_db/rack_middleware'
require 'aws/session_store/dynamo_db/table'
require 'aws/session_store/dynamo_db/version'
43 changes: 1 addition & 42 deletions lib/aws/session_store/dynamo_db/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,6 @@ module Aws::SessionStore::DynamoDB
# :error_handler as a cofniguration object. You must implement the BaseErrorHandler class.
# @see BaseHandler Interface for Error Handling for DynamoDB Session Store.
#
# == Locking Strategy
# By default, locking is not implemented for the session store. You must trigger the
# locking strategy through the configuration of the session store. Pessimistic locking,
# in this case, means that only one read can be made on a session at once. While the session
# is being read by the process with the lock, other processes may try to obtain a lock on
# the same session but will be blocked. See the accessors with lock in their name for
# how to configure the pessimistic locking strategy to your needs.
#
# == DynamoDB Specific Options
# You may configure the table name and table hash key value of your session table with
# the :table_name and :table_key options. You may also configure performance options for
Expand All @@ -49,10 +41,6 @@ class Configuration
:raise_errors => false,
# :max_age => 7*3600*24,
# :max_stale => 3600*5,
:enable_locking => false,
:lock_expiry_time => 500,
:lock_retry_delay => 500,
:lock_max_wait_time => 1,
:secret_key => nil
}

Expand Down Expand Up @@ -94,24 +82,7 @@ class Configuration
# before the current time that the session was last accessed.
attr_reader :max_stale

# @return [true] Pessimistic locking strategy will be implemented for
# all session accesses.
# @return [false] No locking strategy will be implemented for
# all session accesses.
attr_reader :enable_locking

# @return [Integer] Time in milleseconds after which lock will expire.
attr_reader :lock_expiry_time

# @return [Integer] Time in milleseconds to wait before retrying to obtain
# lock once an attempt to obtain lock has been made and has failed.
attr_reader :lock_retry_delay

# @return [Integer] Maximum time in seconds to wait to acquire lock
# before giving up.
attr_reader :lock_max_wait_time

# @return [String] The secret key for HMAC encryption.
# @return [String] The secret key for HMAC encryption of legacy sid.
attr_reader :secret_key

# @return [String,Pathname]
Expand Down Expand Up @@ -159,18 +130,6 @@ class Configuration
# from the current time that a session was created.
# @option options [Integer] :max_stale (nil) Maximum number of seconds
# before current time that session was last accessed.
# @option options [String] :secret_key (nil) Secret key for HMAC encription.
# @option options [Integer] :enable_locking (false) If true, a pessimistic
# locking strategy will be implemented for all session accesses.
# If false, no locking strategy will be implemented for all session
# accesses.
# @option options [Integer] :lock_expiry_time (500) Time in milliseconds
# after which lock expires on session.
# @option options [Integer] :lock_retry_delay (500) Time in milleseconds to
# wait before retrying to obtain lock once an attempt to obtain lock
# has been made and has failed.
# @option options [Integer] :lock_max_wait_time (500) Maximum time
# in seconds to wait to acquire lock before giving up.
# @option options [String] :secret_key (SecureRandom.hex(64))
# Secret key for HMAC encription.
def initialize(options = {})
Expand Down
3 changes: 1 addition & 2 deletions lib/aws/session_store/dynamo_db/errors/default_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ class DefaultHandler < Aws::SessionStore::DynamoDB::Errors::BaseHandler
HARD_ERRORS = [
Aws::DynamoDB::Errors::ResourceNotFoundException,
Aws::DynamoDB::Errors::ConditionalCheckFailedException,
Aws::SessionStore::DynamoDB::MissingSecretKeyError,
Aws::SessionStore::DynamoDB::LockWaitTimeoutError
Aws::SessionStore::DynamoDB::MissingSecretKeyError
]

# Determines behavior of DefaultErrorHandler
Expand Down
4 changes: 2 additions & 2 deletions lib/aws/session_store/dynamo_db/garbage_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def process!(config, sub_batch)
opts = {}
opts[:request_items] = {config.table_name => sub_batch}
begin
response = config.dynamo_db_client.batch_write_item(opts)
response = config.dynamo_db_client.batch_write_item(**opts)
opts[:request_items] = response[:unprocessed_items]
end until opts[:request_items].empty?
end
Expand All @@ -101,7 +101,7 @@ def table_opts(config)
# @api private
def oldest_date(sec)
hash = {}
hash[:attribute_value_list] = [:n => "#{((Time.now - sec).to_f)}"]
hash[:attribute_value_list] = [(Time.now - sec).to_f]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not work, I think it depends on if the SDK is configured with "simple_attributes" - I believe it defaults to true in the SDK.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version of the gem already does not work without the simple_attributes plugin. I have aligned coding style.
This is a testing result for the current version when I turned of the plugin.

     Failure/Error: @config.dynamo_db_client.update_item(save_opts)

     ArgumentError:
       parameter validator found 5 errors:
         - expected params[:key]["session_id"] to be a hash, got class String instead.
         - expected params[:attribute_updates]["updated_at"][:value] to be a hash, got class String instead.
         - expected params[:attribute_updates]["data"][:value] to be a hash, got class String instead.
         - expected params[:attribute_updates]["created_at"][:value] to be a hash, got class String instead.
         - expected params[:attribute_updates]["expire_at"][:value] to be a hash, got class Integer instead.
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/param_validator.rb:35:in `validate!'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/param_validator.rb:15:in `validate!'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/param_validator.rb:25:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/seahorse/client/plugins/raise_response_errors.rb:16:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/checksum_algorithm.rb:111:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/jsonvalue_converter.rb:16:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/idempotency_token.rb:19:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/param_converter.rb:26:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/seahorse/client/plugins/request_callback.rb:89:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/aws-sdk-core/plugins/response_paging.rb:12:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/seahorse/client/plugins/response_target.rb:24:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-core-3.186.0/lib/seahorse/client/request.rb:72:in `send_request'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/aws-sdk-dynamodb-1.96.0/lib/aws-sdk-dynamodb/client.rb:7248:in `update_item'
     # ./lib/aws/session_store/dynamo_db/locking/base.rb:17:in `block in set_session_data'
     # ./lib/aws/session_store/dynamo_db/locking/base.rb:44:in `handle_error'
     # ./lib/aws/session_store/dynamo_db/locking/base.rb:15:in `set_session_data'
     # ./lib/aws/session_store/dynamo_db/rack_middleware.rb:75:in `write_session'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-2.2.7/lib/rack/session/abstract/id.rb:388:in `commit_session'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-2.2.7/lib/rack/session/abstract/id.rb:268:in `context'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-2.2.7/lib/rack/session/abstract/id.rb:260:in `call'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-test-2.1.0/lib/rack/test.rb:360:in `process_request'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-test-2.1.0/lib/rack/test.rb:163:in `custom_request'
     # /Users/ryo/.rvm/gems/ruby-3.1.1/gems/rack-test-2.1.0/lib/rack/test.rb:112:in `get'
     # ./spec/aws/session_store/dynamo_db/rack_middleware_database_spec.rb:83:in `block (3 levels) in <module:DynamoDB>'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a check for invalid client configuration during initialization?

(@mullermp - should we also update the metrics/user agent info?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a check for invalid client configuration during initialization?

It's worth considering. It might be better to make it a separate issue because it is not caused by this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think assuming that it defaults to true is fine.

hash[:comparison_operator] = 'LT'
hash
end
Expand Down
7 changes: 0 additions & 7 deletions lib/aws/session_store/dynamo_db/lock_wait_timeout_error.rb

This file was deleted.

133 changes: 56 additions & 77 deletions lib/aws/session_store/dynamo_db/locking/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,14 @@ def set_session_data(env, sid, session, options = {})
return false if session.empty?
packed_session = pack_data(session)
handle_error(env) do
save_opts = update_opts(env, sid, packed_session, options)
@config.dynamo_db_client.update_item(save_opts)
if env['dynamo_db.new_session']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're removing pessimistic locking, does base/null locking as classes even make sense? We can possibly refactor away anything related to locking.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I need that feature, but others might. I cannot make the decision, but I don't have much time to implement and test for the feature. Please rename files to a suitable names if we could remove the feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to remove pessimistic locking. I am saying that locking/base and locking/null are no longer relevant (because locking is being removed entirely) and we can probably delete these classes and put the logic into rack_middleware or some other new class.

save_options = save_new_opts(env, sid, packed_session)
@config.dynamo_db_client.put_item(save_options)
env.delete('dynamo_db.new_session')
else
save_options = save_exists_opts(env, sid, packed_session, options)
@config.dynamo_db_client.update_item(save_options)
end
sid
end
end
Expand Down Expand Up @@ -51,109 +57,82 @@ def handle_error(env = nil, &block)

# @return [Hash] Options for deleting session.
def delete_opts(sid)
table_opts(sid)
end

# @return [Hash] Options for updating item in Session table.
def update_opts(env, sid, session, options = {})
if env['dynamo_db.new_session']
updt_options = save_new_opts(env, sid, session)
else
updt_options = save_exists_opts(env, sid, session, options)
end
updt_options
{
table_name: @config.table_name,
key: {
@config.table_key => sid
}
}
end

# @return [Hash] Options for saving a new session in database.
def save_new_opts(env, sid, session)
attribute_opts = attr_updts(env, session, created_attr)
merge_all(table_opts(sid), attribute_opts)
{
table_name: @config.table_name,
item: {
@config.table_key => sid,
data: session.to_s,
created_at: created_at,
updated_at: updated_at,
expire_at: expire_at
},
condition_expression: "attribute_not_exists(#{@config.table_key})"
}
end

# @return [Hash] Options for saving an existing sesison in the database.
def save_exists_opts(env, sid, session, options = {})
add_attr = options[:add_attrs] || {}
expected = options[:expect_attr] || {}
attribute_opts = merge_all(attr_updts(env, session, add_attr), expected)
merge_all(table_opts(sid), attribute_opts)
data = if data_unchanged?(env, session)
{}
else
{
data: {
value: session.to_s,
action: 'PUT'
}
}
end
{
table_name: @config.table_name,
key: {
@config.table_key => sid
},
attribute_updates: {
updated_at: {
value: updated_at,
action: 'PUT'
},
expire_at: {
value: expire_at,
action: 'PUT'
}
}.merge(data),
return_values: 'UPDATED_NEW'
}
end

# Unmarshal the data.
def unpack_data(packed_data)
Marshal.load(packed_data.unpack("m*").first)
end

# Table options for client.
def table_opts(sid)
{
:table_name => @config.table_name,
:key => { @config.table_key => sid }
}
end

# Attributes to update via client.
def attr_updts(env, session, add_attrs = {})
data = data_unchanged?(env, session) ? {} : data_attr(session)
{
attribute_updates: merge_all(updated_attr, data, add_attrs, expire_attr),
return_values: 'UPDATED_NEW'
}
end

# Update client with current time attribute.
def updated_at
{ :value => "#{(Time.now).to_f}", :action => "PUT" }
Time.now.to_f
end

# Attribute for creation of session.
def created_attr
{ "created_at" => updated_at }
def created_at
updated_at
end

# Update client with current time + max_stale.
def expire_at
max_stale = @config.max_stale || 0
{ value: (Time.now + max_stale).to_i, action: 'PUT' }
end

# Attribute for TTL expiration of session.
def expire_attr
{ 'expire_at' => expire_at }
end

# Attribute for updating session.
def updated_attr
{
"updated_at" => updated_at
}
end

def data_attr(session)
{ "data" => {:value => session, :action => "PUT"} }
(Time.now + max_stale).to_i
end

# Determine if data has been manipulated
def data_unchanged?(env, session)
return false unless env['rack.initial_data']
env['rack.initial_data'] == session
end

# Expected attributes
def expected_attributes(sid)
{ :expected => { @config.table_key => {:value => sid, :exists => true} } }
end

# Attributes to be retrieved via client
def attr_opts
{:attributes_to_get => ["data"],
:consistent_read => @config.consistent_read}
end

# @return [Hash] merged hash of all hashes passed in.
def merge_all(*hashes)
new_hash = {}
hashes.each{|hash| new_hash.merge!(hash)}
new_hash
end
end
end
17 changes: 14 additions & 3 deletions lib/aws/session_store/dynamo_db/locking/null.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,24 @@ def get_session_data(env, sid)

# @return [Hash] Options for getting session.
def get_session_opts(sid)
merge_all(table_opts(sid), attr_opts)
{
table_name: @config.table_name,
key: {
@config.table_key => sid
},
attributes_to_get: [ "data" ],
consistent_read: @config.consistent_read
}
end

# @return [String] Session data.
def extract_data(env, result = nil)
env['rack.initial_data'] = result[:item]["data"] if result[:item]
unpack_data(result[:item]["data"]) if result[:item]
if result[:item] && result[:item].has_key?("data")
env['rack.initial_data'] = result[:item]["data"]
unpack_data(result[:item]["data"])
else
nil
end
end

end
Expand Down
Loading