Detect controller concern clash #1560

andyw8 · 2022-10-31T15:09:53Z

What this PR does

Closes https://github.com/Shopify/first-party-library-planning/issues/486

RequireKnownShop and LoginProtection both have a method current_shopify_domain but with different implementations. If both these concerns are included, it can result in unintentional behaviour, since one method will overwrite the other.

Reviewer's guide to testing

Things to focus on

Checklist

Before submitting the PR, please consider if any of the following are needed:

Update CHANGELOG.md if the changes would impact users
Update README.md, if appropriate.
Update any relevant pages in /docs, if necessary
For security fixes, the Disclosure Policy must be followed.

andyw8 · 2022-11-01T15:35:49Z

test/dummy/app/controllers/application_controller.rb

@@ -1,5 +1,4 @@
 # frozen_string_literal: true

 class ApplicationController < ActionController::Base
-  include ShopifyApp::LoginProtection


(tests pass but need to do more digging here to see if this is valid)

I believe this needs be removed, because the UnauthenticatedHomeController template inherits from ApplicationController and so would have both ShopifyApp::LoginProtection and ShopifyApp::RequireKnownShop.

cc @nelsonwittwer @teddyhwang

This sounds sane to me - we shouldn't be including LoginProtection globally unless we have a separate "unauthenticated" app controller.

andyw8 · 2022-11-01T15:36:17Z

FYI @nelsonwittwer @klenotiw

andyw8 · 2022-11-08T22:00:56Z

test/dummy/app/controllers/home_controller.rb

@@ -1,6 +1,9 @@
 # frozen_string_literal: true

 class HomeController < ApplicationController
+  include ShopifyApp::EmbeddedApp


I've updated this to better match what the generator outputs:
https://github.com/Shopify/shopify_app/blob/main/lib/generators/shopify_app/home_controller/templates/unauthenticated_home_controller.rb

andyw8 · 2022-11-08T22:12:11Z

CHANGELOG.md

@@ -2,6 +2,7 @@ Unreleased
 ----------
 * Fixes a bug with `EnsureAuthenticatedLinks` causing deep links to not work [#1549](https://github.com/Shopify/shopify_app/pull/1549)
 * Ensure online token is properly used when using `current_shopify_session` [#1566](https://github.com/Shopify/shopify_app/pull/1566)
+* Detect the use of incompatible controller concerns [#1560](https://github.com/Shopify/shopify_app/pull/1560). Depending on how your app is structured, this may result in an error on startup even though the app was working correctly. However, it is helping exposing an existing flaw that may have caused issues in future.


Does this seeem clear?

It makes sense to me, but I wonder if we should just log a warning rather than blow up, so this doesn't become a breaking change. Could we switch it over to an error on a later major version?

This does sound like it's a breaking change so we should follow our deprecation strategy

Makes sense, I'll try switch it over to a deprecation, e.g.:

We detected the use of incompatible concerns (RequireKnownShop and LoginProtection) in a controller, which may lead to unpredictable behavior. In a future release of this library this will raise an error.

Can we easily log the controller name to make debugging it easier?

The logs should already indicate the location, but I've added it just in case. Here's how it looks now:

DEPRECATION WARNING: We detected the use of incompatible concerns (RequireKnownShop and LoginProtection) in ProductsController, which may lead to unpredictable behavior. In a future release of this library this will raise an error. (called from include at /Users/andyw8/src/github.com/Shopify/shopify_app/my_shopify_app_clash/app/controllers/products_controller.rb:5)

andyw8 · 2022-11-09T18:29:13Z

I tested on a newly generated app with a intentional conflict in the ProductsControllers:

class ProductsController < AuthenticatedController
  include ShopifyApp::RequireKnownShop
  # ...

~~With config.eager_load = true, as it would be in production, the app refuses to boot, as would be expected.~~

~~With the Rails default of config.eager_load = false, the browsers stalls on "Loading..." for the Products request, and the error is reported in the console.~~
(now that we are outputting a deprecation warning the above is no longer true).

* Detect controller concern clash * Deprecation warning instead of raising error Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>

andyw8 force-pushed the andyw8/detect-controller-concern-clash branch 6 times, most recently from 5bbb403 to 422346c Compare November 1, 2022 15:11

andyw8 commented Nov 1, 2022

View reviewed changes

andyw8 mentioned this pull request Nov 8, 2022

Update README #1575

Merged

andyw8 force-pushed the andyw8/detect-controller-concern-clash branch 2 times, most recently from 87febfe to 480bf4b Compare November 8, 2022 21:59

andyw8 commented Nov 8, 2022

View reviewed changes

andyw8 force-pushed the andyw8/detect-controller-concern-clash branch 2 times, most recently from 557fee1 to 10b1f2b Compare November 8, 2022 22:11

andyw8 commented Nov 8, 2022

View reviewed changes

Detect controller concern clash

9d7e60a

andyw8 force-pushed the andyw8/detect-controller-concern-clash branch from 10b1f2b to 9d7e60a Compare November 9, 2022 15:55

andyw8 marked this pull request as ready for review November 9, 2022 15:55

andyw8 requested review from paulomarg, teddyhwang and nelsonwittwer and removed request for teddyhwang November 9, 2022 15:55

andyw8 mentioned this pull request Nov 9, 2022

Adding Debug Logs #1545

Merged

4 tasks

nelsonwittwer approved these changes Nov 15, 2022

View reviewed changes

Deprecation warning instead of raising error

bd1f046

andyw8 force-pushed the andyw8/detect-controller-concern-clash branch from bcc75be to bd1f046 Compare November 15, 2022 14:50

andyw8 merged commit e297ef1 into main Nov 15, 2022

andyw8 deleted the andyw8/detect-controller-concern-clash branch November 15, 2022 14:50

fabriazza pushed a commit to fabriazza/shopify_app that referenced this pull request Feb 1, 2023

Detect controller concern clash (Shopify#1560)

44aa93f

* Detect controller concern clash * Deprecation warning instead of raising error Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect controller concern clash #1560

Detect controller concern clash #1560

andyw8 commented Oct 31, 2022 •

edited

Loading

andyw8 Nov 1, 2022 •

edited

Loading

andyw8 Nov 7, 2022

paulomarg Nov 9, 2022

andyw8 commented Nov 1, 2022

andyw8 Nov 8, 2022 •

edited

Loading

andyw8 Nov 8, 2022

paulomarg Nov 9, 2022 •

edited

Loading

teddyhwang Nov 9, 2022

andyw8 Nov 9, 2022

paulomarg Nov 9, 2022

andyw8 Nov 9, 2022

andyw8 commented Nov 9, 2022 •

edited

Loading

Detect controller concern clash #1560

Detect controller concern clash #1560

Conversation

andyw8 commented Oct 31, 2022 • edited Loading

What this PR does

Reviewer's guide to testing

Things to focus on

Checklist

andyw8 Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

andyw8 Nov 7, 2022

Choose a reason for hiding this comment

paulomarg Nov 9, 2022

Choose a reason for hiding this comment

andyw8 commented Nov 1, 2022

andyw8 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

andyw8 Nov 8, 2022

Choose a reason for hiding this comment

paulomarg Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

teddyhwang Nov 9, 2022

Choose a reason for hiding this comment

andyw8 Nov 9, 2022

Choose a reason for hiding this comment

paulomarg Nov 9, 2022

Choose a reason for hiding this comment

andyw8 Nov 9, 2022

Choose a reason for hiding this comment

andyw8 commented Nov 9, 2022 • edited Loading

andyw8 commented Oct 31, 2022 •

edited

Loading

andyw8 Nov 1, 2022 •

edited

Loading

andyw8 Nov 8, 2022 •

edited

Loading

paulomarg Nov 9, 2022 •

edited

Loading

andyw8 commented Nov 9, 2022 •

edited

Loading