Solidus: Document or deprecate use of HTTP_SPREE_STORE header

[brchristian]

I have noticed three(!) SQL calls related to Spree::Store on every single pageview and wondered what was going on...it seems excessive.

The culprit is this line from current_store.rb:

def store_key
  @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME']
end

It seems that (since this commit from two years ago), the _default_ method of checking the current store is by looking at the HTTP_SPREE_STORE response header.

However, looking far and wide I could find a total of zero documentation both in the Solidus codebase, and on the entire internet (!), about how to set or use the HTTP_SPREE_STORE header.

My feeling here is that we need to either (a) deprecate, or (b) document the use of this.

[brchristian]

e.g., perhaps this can be extracted into https://github.com/solidusio/solidus_multi_domain?

[kennyadsl]

@brchristian Hi there, which solidus version are you using? I think this is fixed with this commit.

When @request.headers['HTTP_SPREE_STORE'] or @request.env['SERVER_NAME'] are not set only the default store query (last one) should be done.

[brchristian]

@kennyadsl I’m using the latest gem release, which is 2.2.1.

I note that out of the box, the default 2.2.1 store makes _six_ Spree::Store find calls per pageview! (Three for the main layout, and three for the cart partial.)

My hunch is that only a small fraction of the Solidus user base have more than one Store. My personal opinion is that six SQL calls per pageview is a lot of DB chatter to support what is (IMO) a somewhat niche use-case.

What do you think about trying to move this functionality out into the solidus_multi_domain gem and support a single store by default in the vanilla Solidus gem?

[brchristian]

@kennyadsl Also, the commit you referenced was reverted by https://github.com/solidusio/solidus/commit/0abb4d043408e31aa0da8dc4e514016ae7c190c6!

[kennyadsl]

@brchristian I think the last commit you posted is not reverting the one I linked but it is just removing the deprecation previously set.

As far as I can see this is the code you are using:

      def store
        if store_key
          Spree::Store.current(store_key)
        else
          Spree::Store.default
        end
      end

      private

      def store_key
        @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME']
      end

So if store_key is nil (no @request.headers['HTTP_SPREE_STORE'] or @request.env['SERVER_NAME'] set) it should just make the default store query.

Anyway I'm going to do some more testing on a clean solidus app and see if I find those queries and they are coming from other places in the code. Will keep you posted!

[brchristian]

@kennyadsl Oh, I think you are right about the deprecation. Sorry and thanks for the correction!

When I create a brand new blank Solidus app, @request.env['SERVER_NAME'] is "localhost" by default, and so the conditional for store_key is always true.

What’s more, I think that there is some redundancy because store_key can be either the HTTP_SPREE_STORE field _or_ the SERVER_NAME. This forces us in https://github.com/solidusio/solidus/blob/master/core/app/models/spree/store.rb#L25 to check by code _and_ by url, generating two queries.

Seems like we can clean this up, for sure!

[kennyadsl]

@brchristian I think that moving that code out of core (in solidus_multi_domain as you suggested) is the best way to go. I've just opened a PR (#1993) with initial steps for having it done. Let's see what others think about it and I'll proceed with the rest of the needed PRs.

[kennyadsl]

Steps to fix this issue:

• [x] make it more standard/easier to use another class to determine which store to use #1993
• [ ] move out of core the logic that checks requests info to determine if another store has to be used. This will be moved to solidus_multi_domain. (solidusio/solidus_multi_domain#67)
• [ ] just use the Spree::Store.default on a solidus store without solidus_multi_domain avoiding multiple queries for each request (#2022).

[jhawthorn]

I also would like to remove support for HTTP_SPREE_STORE gone. Since it requires extra configuration to use properly, it doesn't make sense to support it on all stores by default.

I've opened #2041 which is related to this. It supports selecting a store by domain name and falling back to default with a single query :zap:.

[aitbw]

I think this one can be closed as per #2041, right @kennyadsl?

[kennyadsl]

Yes, thanks @aitbw !