Move Session Storage to `shopify_app` #1055

nelsonwittwer · 2022-11-07T18:48:38Z

Description

We would like to remove persistence responsibilities from this API library. Ideally, this API would just deal with API sessions and pass storing, loading, and deleting sessions responsibilities to Shopify App or some other middleware that has persistence.

This change makes config.session_storage optional and only makes store_session calls if the storage library is provided.

TODO

Add deprecation logging to SessionUtils
Add testing for new conditional store_session
Can we deprecate session storage

How has this been tested?

Created a new embedded app
Pointed to WIP shopify_app PR where we moved session persistence logic to that layer
Verified I was able to install new app, reload sessions, and make API calls.

Checklist:

My commit message follow the pattern described in here
I have performed a self-review of my own code.
I have added tests that prove my fix is effective or that my feature works.
I have updated the project documentation.
I have added a changelog line.

Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>

lib/shopify_api/context.rb

Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>

andyw8 · 2022-11-15T21:16:21Z

lib/shopify_api/utils/session_utils.rb

@@ -17,12 +19,14 @@ class << self
          ).returns(T.nilable(Auth::Session))
        end
        def load_current_session(auth_header: nil, cookies: nil, is_online: false)
+          ::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_current_session has been deprecated. The ShopifyAPI  will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")


Do we still need these now that we have the check in Context?

(also - extra space before 'will')

mkevinosullivan · 2022-11-15T21:13:40Z

lib/shopify_api/context.rb

@@ -69,6 +69,11 @@ def setup(
        @scope = Auth::AuthScopes.new(scope)
        @is_embedded = is_embedded
        @session_storage = session_storage
+        if @session_storage
+          # rubocop:disable Layout/LineLength
+          ::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")


Suggested change

::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")

::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

lib/shopify_api/utils/session_utils.rb

mkevinosullivan · 2022-11-15T21:15:34Z

lib/shopify_api/utils/session_utils.rb

@@ -33,10 +37,12 @@ def load_current_session(auth_header: nil, cookies: nil, is_online: false)
          ).returns(T::Boolean)
        end
        def delete_current_session(auth_header: nil, cookies: nil, is_online: false)
+          ::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI  will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")


Suggested change

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

mkevinosullivan · 2022-11-15T21:18:44Z

lib/shopify_api/utils/session_utils.rb

@@ -46,8 +52,10 @@ def delete_current_session(auth_header: nil, cookies: nil, is_online: false)
          ).returns(T.nilable(Auth::Session))
        end
        def load_offline_session(shop:, include_expired: false)
+          ::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI  will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")


Suggested change

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

mkevinosullivan · 2022-11-15T21:19:09Z

lib/shopify_api/utils/session_utils.rb

-          unless Context.private_shop
-            raise Errors::SessionNotFoundError, "Could not load private shop, Context.private_shop is nil."
-          end
+          ::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI  will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")


Suggested change

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")

::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

lib/shopify_api/context.rb

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

andyw8 · 2022-11-18T14:32:34Z

lib/shopify_api/auth/oauth.rb

-            raise Errors::SessionStorageError,
-              "Session could not be saved. Please check your session storage implementation."
-          end
+          Context.session_storage&.store_session(session)


It's probably an edge case, but should we be retaining the previous behaviour of raising if store_session returns nil?

The new normal is for Context.session_storage to be nil as it is deprecated. It's my hope that this will be nil :)

andyw8 · 2022-11-18T14:33:47Z

lib/shopify_api/context.rb

+        if @session_storage
+          ::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. " \
+            "The ShopifyAPI will no longer have responsibility for session persistence. " \
+            "Consider using the `shopify_app` gem which now implements this responsibility.")


Can we point to any particular docs here?

lib/shopify_api/context.rb

mkevinosullivan · 2022-11-18T15:43:49Z

lib/shopify_api/utils/session_utils.rb

+        def validate_session_storage_for_deprecated_utils
+          unless Context.session_storage
+            raise ShopifyAPI::Errors::SessionStorageError,
+              "session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."


Suggested change

"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."

"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."

If these session utility methods are deprecated, what's the recommended replacement? Presumably something in shopify_app? Should we point them towards that or does that create too much of a link between shopify-api-ruby and shopify_app?

this particular error will be super hard to reach. This error was put in place mainly because the sorbet one wasn't pretty. I think the upgrade logs we have in the Context.setup call should help give actionable steps on upgrade path.

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

…y-api-ruby into response_in_error_object * 'response_in_error_object' of github.com:kaarelss/shopify-api-ruby: Added assertion that response is present in MaxHttpRetriesExceededError Added sorbet assertion Removed extra empty line ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError because code can be obtained from response as well as headers for custom retry logic Remove mentions of private apps (Shopify#1062) Fix ActiveSupport inflector dependency (Shopify#1063) Move Session Storage to `shopify_app` (Shopify#1055) Remove session argument from REST examples (Shopify#1064) Constrain Zeitwerk to 2.6.5 (Shopify#1059) Clarify statement about packages (Shopify#1057)

Configuring the API with session_storage is now deprecated. Shopify#1055

make session storage optional

aaa813b

nelsonwittwer requested a review from a team as a code owner November 7, 2022 18:48

nelsonwittwer added 3 commits November 15, 2022 13:37

deprecate session storage

924bd3e

logging

e4e1754

cleaner deprecation strategy

1d989aa

nelsonwittwer changed the title ~~WIP - Deprecate Session Storage~~ Move Session Storage to shopify_app Nov 15, 2022

andyw8 and others added 3 commits November 15, 2022 15:39

Clarify statement about packages (#1057)

4658620

Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>

Constrain Zeitwerk to 2.6.5 (#1059)

40c4eb1

Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>

Merge branch 'main' into nelsonwittwer/deprecate_session_utils

3391c5f

andyw8 reviewed Nov 15, 2022

View reviewed changes

lib/shopify_api/context.rb Outdated Show resolved Hide resolved

Update lib/shopify_api/context.rb

b27123c

Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>

andyw8 reviewed Nov 15, 2022

View reviewed changes

mkevinosullivan reviewed Nov 15, 2022

View reviewed changes

nelsonwittwer and others added 6 commits November 16, 2022 09:37

Update lib/shopify_api/utils/session_utils.rb

99290fa

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

clean up deprecation logging

6b3018c

raise error if trying to use deprecrated utils without session_storage

269f322

throw error if misconfigured session_storage

3b6ff74

Merge branch 'main' into nelsonwittwer/deprecate_session_utils

82b3a15

Update lib/shopify_api/context.rb

3b33a32

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

andyw8 reviewed Nov 18, 2022

View reviewed changes

nelsonwittwer added 2 commits November 18, 2022 09:55

change to more helpful upgrade instructions

1450927

rubocop line length

3956654

andyw8 approved these changes Nov 18, 2022

View reviewed changes

mkevinosullivan approved these changes Nov 18, 2022

View reviewed changes

nelsonwittwer and others added 3 commits November 18, 2022 14:00

Update lib/shopify_api/context.rb

8b4eb07

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

Update lib/shopify_api/context.rb

d13ace5

Co-authored-by: Kevin O'Sullivan <56687600+mkevinosullivan@users.noreply.github.com>

rubocop

b6816eb

nelsonwittwer merged commit 5b5e0b4 into main Nov 18, 2022

nelsonwittwer deleted the nelsonwittwer/deprecate_session_utils branch November 18, 2022 19:08

mikelieser mentioned this pull request Jan 30, 2023

Warning about Sessions?? #1084

Closed

nelsonwittwer mentioned this pull request May 15, 2023

eject session storage #1155

Merged

5 tasks

takp added a commit to takp/shopify-api-ruby that referenced this pull request May 27, 2023

Fix documents for deprecated session_storage

deb82c0

Configuring the API with session_storage is now deprecated. Shopify#1055

takp mentioned this pull request May 27, 2023

Fix documents for deprecated session_storage #1165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Session Storage to `shopify_app` #1055

Move Session Storage to `shopify_app` #1055

nelsonwittwer commented Nov 7, 2022 •

edited

Loading

andyw8 Nov 15, 2022

andyw8 Nov 15, 2022

mkevinosullivan Nov 15, 2022

mkevinosullivan Nov 15, 2022

mkevinosullivan Nov 15, 2022

mkevinosullivan Nov 15, 2022

andyw8 Nov 18, 2022

nelsonwittwer Nov 18, 2022

andyw8 Nov 18, 2022

mkevinosullivan Nov 18, 2022

nelsonwittwer Nov 18, 2022

	::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
	::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

	::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
	::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

	"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."
	"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."

Move Session Storage to shopify_app #1055

Move Session Storage to shopify_app #1055

Conversation

nelsonwittwer commented Nov 7, 2022 • edited Loading

Description

TODO

How has this been tested?

Checklist:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Move Session Storage to `shopify_app` #1055

Move Session Storage to `shopify_app` #1055

nelsonwittwer commented Nov 7, 2022 •

edited

Loading