Default store headaches on 1.3

[jhawthorn]

Something that seems to be popping up early on in the Solidus 1.3.0.rc1 upgrades is issues with the new requirement that all orders have an attached store.

This was introduced in #935, which IMO is a great change, it will simplify a lot being able to rely on the store being set. However we should do whatever possible to make the upgrade smooth.

Previously, Store.default would return Store.new if there was no existing default (to simplify writing specs, presumably). This is no longer a valid "default" in most cases, since an id will be required to make the store valid. This will make operations like current_order(create_order_if_neccesary: true) break if no store is in the database.

Things we must do

• Document this change more obviously
• Document what changes are recommended for specs
• Fixes @jordan-brough has proposed in Task and task spec fixes #1224

Some options we could consider:

• @jrochkind suggests that Store.default could create a default store if none exists. This would make current_order(create_order_if_neccesary: true) work in specs (largely) as it used to.
• Have Store.default raise an error if there is no default Store. This won't make anything pass which was broken by this, but would give better indication of the issue.
• Drop the validation for the 1.3 release, and instead introduce a warning that we intend to validate this in the future.
• Drop the validation for the 1.3 release, and introduce a Order#store_or_default to help any pieces of code which want to reliably get a store.

[jordan-brough]

This was introduced in #935, which IMO is a great change

Agreed

Store.default could create a default store if none exists

For some reason that feels a bit scary to me.

Have Store.default raise an error if there is no default Store

I like that idea. I think it'll prevent more errors than it'll cause.

Or perhaps we could instead (or in addition) remove the || new from Store#default and add a boot-time check to ensure that a default Store exists? That might require updating the solidus installer to generate a Store by default for new stores but that's probably a good idea anyway. (Does the installer already do that?)

[jrochkind]

The current Store.default implementation is screwy, creating a non-persisted store with new.

So if you call Store.default with no default store in the db, you get an unpersisted one returned, which maybe something will try to save at a later point (say, if you assign it to an association and save the target), or maybe won't. This is a mess.

So while I agree lazily creating a store in default doesn't feel right, it's for a long time already done this, although in an even harder to predict way -- the one it lazily creates doesn't even have the default attribute set!

So my initial thought was to just change Store.default to where(default: true).first_or_create -- if it's going to lazily create a store (which it has been doing for a long time), at least set it's default attribute to true, and persist it.

But this doesn't work, because a Store can't be saved without a name, url, and mail_from_address too.

So maybe Store.default shoudln't be lazily creating anything.

The problem is this will make a whole bunch of tests in my local app fail, that have always passed before. I'm probably not alone. Because I use DatabaseCleaner to wipe the database in between every test (I guess solidus suite does not do this?), so there's always 0 Spree::Store's at the beginning of a test.

But after this change, any test that involves current_order(create_order_if_neccesary: true) (and probably more) will end up raising, unless it creates a default store in advance. When they all used to work. I guess the answer could just be find all my failing tests and add a before { create(:store, default: true) } to them. I guess maybe that works. I think people are going to find it painful.

I have run into related problems like this with test environments and 'seed' data, and I have no good general purpose solution, I've always ended up having to write code I didn't like.

Incidentally, in investigating it, I discovered that the [current specs for Spree::Store.default](current_order%28create_order_if_neccesary: true%29) are pretty useless. They say they are testing to make sure .default should ensure there is a default if one doesn't exist yet and .default should ensure there is only one default, but they do not test/verify this, the current specs don't in fact test anything other than their own setup, unless I'm missing something.

[jordan-brough]

The || new part of where(default: true).first || new in Store.default seems like an odd pattern to me for Solidus, and it's confused me more than once.

Some other places where we have "default" methods:

• Country.default
• TaxCategory.default
• User#default_address
• User#default_user_address
• User#default_credit_card
• Store.default_created_by
• Tracker.current
• Zone.default_tax

All of those return nil if there is no default set.

The only one I could find that behaves similar to this was Address.default, and that method is deprecated in favor of Address.build_default, which is a lot clearer.

[jordan-brough]

the one it lazily creates doesn't even have the default attribute set!

@jrochkind I believe the before_save :ensure_default_exists_and_is_unique here means that it will have the default attribute set when saved. But it's true that it won't have that attribute set until it's actually saved.

maybe Store.default shoudln't be lazily creating anything

I feel like intuitively that's what I'd expect. If we could get there w/o breaking things for people too much I think it'd be great.

[jordan-brough]

FYI another issue I've had while upgrading is that Order.before_validation :associate_store isn't aggressive enough to rely on -- it only happens after valid? is called on the order, which is too late in some cases. So it makes some things work but others not work (because some code tries to read order.store before order.valid? has been called.)

[jordan-brough]

Drop the validation for the 1.3 release

Would that require a lot of change? A new issue that came up for me in rc1 was from this change on Order#tax_address -- it expects a store to be present on the order or it raises an NoMethodError error since the store is nil.

[jrochkind]

@jordan-brough

@jrochkind I believe the before_save :ensure_default_exists_and_is_unique here means

Aha, right.

However, I think in fact, if anything does try to save the thing lazily created from Store.default -- it'll just fail validation anyway, since it has no name, email, or mail_from_address. (Not sure how recent those presence validations are). So that lazily created Store.new returned from Store.default at present is just a disaster waiting to happen. You can easily accidentally trigger a save (without even knowing what you got from Store.default wasn't yet persisted in the first place), by assigning it to an association on some other record (which is probably what you want Store.default for after all).

[jordan-brough]

it'll just fail validation anyway, since it has no name, email, or mail_from_address
So that lazily created Store.new returned from Store.default at present is just a disaster waiting to happen

Great points.

[jrochkind]

Does anyone know how Solidus' own specs take care of making sure a default store is there?

I know a default store starts out in the generated dummy app, which I guess works if you're using transactional test destroy method, but that doesn't work for capybara feature tests, they presumably wipe out the whole db... frontend at least is using DatabaseCleaner truncation for tests tagged js. I can't figure out how these specs are passing at all, without creating a default Store first.

I guess I can try to debug it and figure it out.

[mamhoff]

Drop the validation for the 1.3 release
Would that require a lot of change? A new issue that came up for me in rc1 was from this change on Order#tax_address -- it expects a store to be present on the order or it raises an NoMethodError error since the store is nil.

That's originally why I introduced the validation in the first place. If I don't have it, code down the line has to provide paths for the store not being there, and that's not very good.

Additional food for thought: I think the vast majority of users has only one store, and those that don't will have a store_id associated. Otherwise that would be just reckless, and will produce more pain the later they address that issue.

[mtomov]

In my view

• tests should not have to create a store for every order test
• default store should not be DB looked up on every request (valid for both one-store & multi-store)

So, I think a default store should be automatically created and class memorized. I just recently did that for a single-store site to avoid all those db lookups on every request.

    def current(domain = nil)
      @@current_store ||= Spree::Store.default
    end

That has the disadvantage that an app restart will be required if someone changes the default store, but I think the benefits are worth the minor inconvenience.

[jrochkind]

@mtomov I agree with your bullet points as desirable, but I've found it awfully hard to implement.

It's pretty dangerous to keep an AR in a class attribute, as it'll end up shared between threads in many scenarios (including worker threads, multi-threaded dispatch with puma, and others), and AR models are not thread-safe to be shared between threads. If you marked it readonly (an AR feature), it might be safer, but I wouldn't want to count on it, this stuff gets complicated.

I think sharing an AR model between threads, as will happen eventually if it's cached at the class level, is probably a not good idea.

So I'm not sure there's any good way to accomplish those points, if every order needs a store, which has already been committed to I think?

It is a pickle.