Upgrade to Rack 3 #33

ryokdy · 2023-11-24T02:27:37Z

Upgrade to Rack 3 and Rack::Session v2.0 Upgrade to Rack 3 #28
Inherit ActionDispatch::Session::AbstractSecureStore instead of Rack::Session::Abstract::Persisted
Remove the pessimistic locking strategy due to performance issue. It is complicated and consistency-read option is enough to solve the concurrency issues. Please implement if someone needs it.
Use DynamoDB local for integration test.

Replaced the generate_sid function with the default ActionDispatch::Session#generate_sid instead of the original function. Existing sessions associated with older versions of the sid will be available after upgrading the gem.
Newly created sids are hashed and stored in the database.

…of rack.

…exist.

…TTL on for garbage collection.

…session.

# Conflicts: # aws-sessionstore-dynamodb.gemspec # lib/aws/session_store/dynamo_db/locking/base.rb # lib/aws/session_store/dynamo_db/locking/null.rb

…:Session::Abstract::Persisted

mullermp · 2023-11-28T18:34:45Z

Thanks for opening a PR. I will be able to review this more thoroughly soon.

Using standard generate_sid function of ActionDispatch::Session instead of the original generate_sid, existing sessions will no longer be available after upgrading the gem. The SID is hashed and stored in the database.

Do you have a recommendation for customers on how to handle this? We should also document this behavior in the changelog.

mullermp

This is an interesting PR. Thanks for opening a pull request. I can talk this over with the team.

aws-sessionstore-dynamodb.gemspec

mullermp · 2023-11-28T18:39:12Z

lib/aws/session_store/dynamo_db/garbage_collection.rb

@@ -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]


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.

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>'

Worth adding a check for invalid client configuration during initialization?

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

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.

I think assuming that it defaults to true is fine.

mullermp · 2023-11-28T18:42:28Z

lib/aws/session_store/dynamo_db/locking/base.rb

@@ -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']


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

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.

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.

spec/aws/session_store/dynamo_db/locking/threaded_sessions_spec.rb

ryokdy · 2023-11-29T05:21:28Z

Do you have a recommendation for customers on how to handle this? We should also document this behavior in the changelog.

I added support for the legacy format sid. Sessions related to older versions of the sid will be available after upgrading.
336c2d5
8cbf185

alextwoods

Thanks for submitting this - looks good overall and I think Its reasonable to remove the pessimistic locking strategy in an MV bump.

alextwoods · 2023-11-29T15:53:47Z

README.md

@@ -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.


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

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.

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

alextwoods · 2023-11-29T16:01:33Z

spec/aws/session_store/dynamo_db/table_spec.rb

@@ -23,7 +23,13 @@ module DynamoDB
      describe Table do
        context 'Mock Table Methods Tests', integration: true do
          let(:table_name) { "sessionstore-integration-test-#{Time.now.to_i}" }
-          let(:options) { { table_name: table_name } }
+          let(:dynamo_db_client) {
+            options = {


Nit: probably don't need a hash for this and can just initialize client

You mean like this?

Aws::DynamoDB::Client.new(endpoint: 'http://localhost:8000')

alextwoods · 2023-11-29T16:01:56Z

spec/aws/session_store/dynamo_db/table_spec.rb

-          let(:options) { { table_name: table_name } }
+          let(:dynamo_db_client) {
+            options = {
+              endpoint: 'http://localhost:8000'


We should add a section in the README mentioning the use of dynamodb local for integ testing.

alextwoods · 2023-11-29T16:31:19Z

lib/aws/session_store/dynamo_db/garbage_collection.rb

@@ -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]


Worth adding a check for invalid client configuration during initialization?

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

mullermp · 2023-12-04T13:32:34Z

Added comments, sorry for the late reply! I will try my best to keep up with this, but I will be traveling this week.

mullermp · 2024-01-23T20:29:58Z

I extracted out the minimum rack 3 upgrade here: #34. I think most of these other changes make sense in a major version bump - but for now, I think we need rack 3 available in a minor version.

mullermp · 2024-01-25T18:43:52Z

I've merged and released #34 to upgrade to Rack 3. Other discussions about using locking and using ActionDispatch for a session store are needed, and major rework of this PR is needed.

From this package's POV, I think we should NOT depend on ActionDispatch (specific to rails) - this gem is intended for all rack applications, not just rails applications. I do think we can remove locking in a MV bump.

mullermp · 2024-10-20T01:57:44Z

I've got a PR at #37 that does this work and a bunch of other changes.

mullermp · 2024-10-27T23:38:25Z

Rack 3 will be the minimum dependency of aws-sessionstore-dynamodb ~> 3 and I will release the gem this week. I am uncertain if I will force that version in the aws-sdk-rails dependency which is currently pinned to ~> 2. I have a PR which does some refactoring in aws-sdk-rails and will eventually check for this gem to be loaded and not be a dependency (aws/aws-sdk-rails#147)

David Massad and others added 26 commits April 18, 2017 18:43

Changed rack version dependency so that it works with newer versions …

8826905

…of rack.

Fixed issue where session item is found with no data.

c3f2194

Fixed issue where error occurs when deleting a session that does not …

19e175c

…exist.

Merge pull request aws#13: Add expire_at attribute you can configure …

4618999

…TTL on for garbage collection.

Changed value for exipre_at to a hash

af9d8b3

Changed value of n to a string

f0ecb0e

Removed extra space

2dfa52a

Added missing brace.

10cb346

Merged pull request aws#11: Remove expected attributes from deleting …

38cbc9e

…session.

Remove expected attributes from delete options.

8969995

Merge commit 'f64696a9fd91a51572982b31e8411de14002528f'

aa8526e

# Conflicts: # aws-sessionstore-dynamodb.gemspec # lib/aws/session_store/dynamo_db/locking/base.rb # lib/aws/session_store/dynamo_db/locking/null.rb

Updated expire_at method.

67f68e7

Update aws-sdk version

9fad25a

Merge remote-tracking branch 'jdutil/hotfix/10-rebase'

8ebe9a2

Merge remote-tracking branch 'FronteraConsulting/master'

789cf9d

Change attribute type of created_at and updated_at to float

b807243

Make sure DynamoDB record has data attribute

5ecb1e0

Merge upstream main branch

1caddc3

Remove .ruby-gemset and .ruby-version

018e580

Upgrade rack to 3.0

df4b777

Inherit ActionDispatch::Session::AbstractSecureStore instead of Rack:…

9f6dafc

…:Session::Abstract::Persisted

Sid may be nil when testing with RSpec

238476f

Resolve race condition issues when using one-time passwords

02c4bb0

Revert 02c4bb0

555cdb8

Remove LockWaitTimeoutError

791ead7

Merge upstream main branch

fdc3806

mullermp added the pr/needs-review This PR needs a review from a Member. label Nov 24, 2023

ryokdy added 3 commits November 27, 2023 11:51

Update Rack version to v2.0.8+

2ad11f4

Remove debug gem

899f68e

Remove debug gem

b6eb684

mullermp reviewed Nov 28, 2023

View reviewed changes

ryokdy added 6 commits November 29, 2023 12:06

Add support for legacy format session ids

336c2d5

Organize alphabetically

51aa400

Restore threaded_session_spec

b712196

Change required rack-session to v1.0.1

3126844

Fix bug preventing saving of sessions with legacy format sid

8cbf185

Merge branch 'pr33-review'

bfbe733

Avoid to create a new sid when writing sessin

af58c09

alextwoods reviewed Nov 29, 2023

View reviewed changes

mullermp added pr/do-not-merge This PR should not be merged at this time. and removed pr/needs-review This PR needs a review from a Member. labels Jan 25, 2024

mullermp mentioned this pull request Oct 20, 2024

New version refresh #37

Merged

mullermp closed this in #37 Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Rack 3 #33

Upgrade to Rack 3 #33

ryokdy commented Nov 24, 2023 •

edited

Loading

mullermp commented Nov 28, 2023

mullermp left a comment

mullermp Nov 28, 2023

ryokdy Nov 29, 2023

alextwoods Nov 29, 2023

ryokdy Nov 30, 2023

mullermp Dec 4, 2023

mullermp Nov 28, 2023

ryokdy Nov 29, 2023

mullermp Dec 4, 2023

ryokdy commented Nov 29, 2023

alextwoods left a comment

alextwoods Nov 29, 2023

ryokdy Nov 30, 2023

mullermp Dec 4, 2023

alextwoods Nov 29, 2023

ryokdy Nov 30, 2023

alextwoods Nov 29, 2023

alextwoods Nov 29, 2023

mullermp commented Dec 4, 2023

mullermp commented Jan 23, 2024

mullermp commented Jan 25, 2024

mullermp commented Oct 20, 2024

mullermp commented Oct 27, 2024

Upgrade to Rack 3 #33

Upgrade to Rack 3 #33

Conversation

ryokdy commented Nov 24, 2023 • edited Loading

mullermp commented Nov 28, 2023

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryokdy commented Nov 29, 2023

alextwoods left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp commented Dec 4, 2023

mullermp commented Jan 23, 2024

mullermp commented Jan 25, 2024

mullermp commented Oct 20, 2024

mullermp commented Oct 27, 2024

ryokdy commented Nov 24, 2023 •

edited

Loading