[Feature Request] Prioritize Primary when multiple Shops match domain

[pmn4]

Feature Request:

When multiple shops match the searched domain, prioritize the Primary Shop. This is important when searching for "shop settings" which are only stored on the Primary Shop (e.g., smtp settings).

This is unlikely to even be noticed because Mongo usually returns documents in the order in which they were stored (and the Primary Shop is stored first), but there are a few scenarios which could disrupt this.
I noticed this recently in development, and figured I would suggest the change now long before it is needed.

[pmn4]

if this were SQL, I would have done something like:

SELECT _id
FROM shops
WHERE domain = 'domain'
ORDER BY CASE shopType WHEN 'primary' THEN 0 ELSE 1 END # <--
LIMIT 1

but I couldn't find an equivalent construct in Mongo.
Instead, I sort on whether a Shop has Merchant Shops, assuming only the Primary Shop does.

[brent-hoover]

@pmn4 Since merchantShops is an array, I wonder if this is going to always give you the results you want? Seems like we would want to sort on merchantShop.length somehow?

[brent-hoover]

Wow, we call this function > 900 times on startup.

[pmn4]

we sure do! and about 15/request (I have a PR for that too!)

[pmn4]

as for the sort, it is definitely not ideal. It is basically a null/not-null sort.
seems like sorting on length would be an expensive query, but since you are not likely to have too many shops, I guess it would be ok.
I am definitely out of my depth when it comes to Mongo, so open to any suggestions

[brent-hoover]

Since orderBy is basically happening after the find, I wonder how much slower sorting it in memory would be? Although I wonder why this is doing a find and then a fetch[0]. Seems like we could be doing a findOne? If there's 1000 shops in a marketplace, that's a lot of data to pull across the network and then immediately throw away?

[pmn4]

there is a limit: 1 in the find, so it shouldn't be pulling much/any more data across the wire than findOne would. Doesn't seem like getting a cursor here helps at all (since we immediately get the first record), so happy to change it to findOne

[jshimko]

I feel like the bigger problem here is that you could possibly have more than one shop with the same domain. Should we maybe consider just fixing that instead? Then you would also know for sure that the query with limit: 1 or a findOne() would always be safe. And of course, you wouldn't have the issue of trying to access two shops with the same domain.

Just a thought. Not sure if there was originally some reason for allowing that. Or maybe I don't totally understand how domains are being used in a marketplace context. Opinion @spencern?

[pmn4]

@jshimko thanks for taking a look!

this is a tough one because all shops will include the Primary Shop's domain.
(This allows for the shop to be accessed via /shop/slug and for Primary Shop owners to toggle between the shops in Admin mode.)

This is a total edge case... Mongo sorts in such a way that this will likely never be an issue. The use-case for the change is if your site gets big enough that you start sharding your DB (maybe, I'm no expert there), or if for some reason, you delete and re-insert your primary shop.

I hit this while tinkering with the DB directly, so don't think it's a super important change, but one that could avoid a headache somewhere way down the line

[aaronjudd]

@reactioncommerce/community can we move this forward?

[brent-hoover]

@jshimko What's holding us back from moving forward with this?

[brent-hoover]

@pmn4 So looking at this again I am just not comfortable using what seems like a non-standard use of orderBy on an array. I think this is a legitimate problem but I feel like we need a more supported solution whose behavior will not change in a new version of Mongo.

[pmn4]

I completely understand the concern. If it helps you get more comfortable, this is a $type sort, and an argument can be made that this is a supported mongo feature. LMK what you think.

Another solution would be something like this, like you hinted at earlier:

getShopIdByDomain() {
  const domainShops = Shops.find({
    domains: domain
  }, {
    fields: {
      _id: 1,
      shopType: 1
    }
  }).fetch();

  const shop = domainShops.find((s) => s.shopType === "primary") || domainShops[0];
  return shop._id;
}

If it is safe to assume that we have already fetched the primary shop (which seems likely), we could do:

getShopIdByDomain() {
  const primaryShop = Reaction.getPrimaryShop();

  if (primaryShop.domains.includes(domain)) {
    return primaryShop._id;
  }

  // the rest of the method as it is currently defined
}

[spencern]

@zenweasel can you take a look at this?

[brent-hoover]

@pmn4 So I have been working with optimisations lately and I think this might have a performance implication that hasn't been noted in that it's impossible to index on that sort and in situations where a user may have a lot of shops it's going to cause this query to do a full-table scan.

Just thinking out loud but could we leverage the primaryShop flag somehow to order? If we can't sort on it maybe do queries for with and without and concatenate them?

Maybe I am missing something coming at this again after so long.

[brent-hoover]

@pmn4 Can we get conflicts resolved and some feedback on this so we can get it merged?

[pmn4]

thanks for following up on this @zenweasel.
I am a little out of my depth in mongoland, and given your concerns with indexing, I re-wrote it a bit. lmk what you think.

[pmn4]

I wanted to avoid making two queries for this method, but

1. if the condition on line 426 is satisfied, the second query will not execute
2. it is probably ok to assume that getPrimaryShop will be called at some other point and could be cached

[loanlaux]

A client's project needed this one and I had to manually integrate @pmn4's fix as a temporary bandaid. Would be great to see it merged, as I think I'm not the only one who had to do this.

[brent-hoover]

I'm ok with this approach if we can get the conflicts resolved

[pmn4]

@zenweasel rebased.