reactioncommerce / reaction

Mailchimp Open Commerce is an API-first, headless commerce platform built using Node.js, React, GraphQL. Deployed via Docker and Kubernetes.
https://mailchimp.com/developer/open-commerce/
GNU General Public License v3.0
12.33k stars 2.17k forks source link

Roles field on admin user is too large to index when we have many packages #4261

Closed zrisher closed 4 years ago

zrisher commented 6 years ago

Prerequisites

Issue Description

Here is our highest rated query on our mLab Slow Queries tab:

image

This query is produced by the userIsInRole() function from Meteor Roles.

As you can probably tell, the query itself is not particularly troublesome - the issue here is that we're missing an index on the roles field.

I'm sure there's something in the app that ensures an index on this field (though I couldn't find it in either the Reaction or Meteor Roles codebases), but the problem for us is that we can't create an index on Users.roles because the value for the admin user is too large.

Here's the admin user record created in a fresh database, it's similar in production:

image

As you can see, under roles we have one entry per store (keyed by the store's id) as well as under __global_roles__, under each of which we have the full set of permissions generated from the package's registry (including one per route). This is all done by assignOwnerRoles().

For those familiar with Meteor Roles, this pattern makes sense - it allows us to define roles for users under particular groups (in this case shops), as well as globally for all groups. When a permission is checked (via userIsInRole()), it returns true if that permission is stored in that user's roles under that particular group or in __global_roles__.

But unfortunately this leaves us with an object so large in the admin user's roles field that it cannot be indexed when we have many shops and routes/permissions.

Steps to Reproduce

  1. Start a new application
  2. Add a large number of shops and routes/permissions to the registry.
  3. Run the application

Possible Solution

Clearly one solution would be to use less Shops. We are definitely not currently using it as it was originally intended, and we do plan on moving to an implementation with less shops. However, this seems to be a structural issue with the code base if it provides an upper limit on the number of shops the application can use and still run well.

assignOwnerRoles() adds permissions at both the Shop level and Global level:

  // we don't use accounts/addUserPermissions here because we may not yet have permissions
  Roles.addUsersToRoles(owners, defaultOwnerRoles, shopId);

  // the reaction owner has permissions to all sites by default
  Roles.addUsersToRoles(owners, globalRoles, Roles.GLOBAL_GROUP);

This allows us to remove the admin's permissions to one shop but not others as we see fit - we would remove the global permission and the permission under that particular shop. It seems to me this feature would rarely be used.

However, if we were OK with losing that feature, we could only add permissions to the admin user under the GLOBAL_GROUP (i.e. comment out line 98), which would fix this problem without removing any other functionality.

Versions

Much earlier version, but relevant code is the same in master.

Node: 4.8.4
NPM: 2.15.11
Meteor Node: 4.8.4
Meteor NPM: 4.6.1
Reaction CLI: 0.29.0
Reaction: 1.5.3
zrisher commented 6 years ago

Unfortunately, it turns out that even after removing the permissions for all entries besides __global_roles__, the field is still too large to index on the Admin user:

db.getCollection('users').createIndex({"roles": 1}) produces

{
    "ok" : 0.0,
    "errmsg" : "WiredTigerIndex::insert: key too large to index, failing  4864 { : { __global_roles__: [ \"owner\", \"admin\", \"guest\", \"account/profile\", \"product\", \"tag\", \"index\", \"cart/checkout\", \"cart/completed\", \"createProduct\", \"reaction-shipping\", \"shipping\", \"settings/shipping\", \"reaction-accounts\", \"accounts\", \"account/verify\", \"reaction-accounts/accountsSettings\", \"dashboard/accounts\", \"reaction-catalog\", \"catalog/settings\", \"reaction-checkout\", \"reaction-connectors\", \"connectors\", \"settings/connectors\", \"reaction-dashboard\", \"dashboardPackages\", \"dashboard\", \"shopSettings\", \"reaction-discounts\", \"reaction-email\", \"email/settings\", \"reaction-i18n\", \"reaction-i18n/i18nSettings\", \"reaction-layout\", \"reaction-logging\", \"reaction-orders\", \"orders\", \"dashboard/orders\", \"dashboard/pdf/orders\", \"reaction-payments\", \"payments\", \"payment/settings\", \"reaction-revisions\", \"catalog/settings/revisions/general\", \"reaction-router\", \"reaction-notification\", \"notifications\", \"reaction-taxes\", \"taxes\", \"taxes/settings\", \"taxes/settings/rates\", \"reaction-taxes/flatRateCheckoutTaxes\", \"reaction-templates\", \"Templates\", \"templates/settings\", \"templates/settings/email\", \"reaction-ui\", \"reaction-ui/uiDashboard\", \"dashboard/uiThemeDetails\", \"reaction-ui-navbar\", \"reaction-ui-tagnav\", \"reaction-migrations\", \"reaction-analytics\", \"reaction-analytics/reactionAnalytics\", \"dashboard/analytics\", \"reaction-analytics/reactionAnalyticsSettings\", \"reaction-connectors-shopify\", \"settings/connectors/shopify\", \"reaction-default-theme\", \"discount-codes\", \"discount-codes/customDiscountCodes\", \"discount-codes/discountCodesCheckout\", \"reaction-inventory\", \"dashboard/inventory\", \"reaction-jobcontrol\", \"reaction-marketplace\", \"reaction-marketplace/marketplaceShopSettings\", \"marketplaceShops\", \"sellerShopSettings\", \"reaction-marketplace/marketplaceMerchantSettings\", \"taxes-avalara\", \"taxes/settings/avalara\", \"taxes/addressValidation/avalara\", \"taxes/calculation/avalara\", \"taxes/taxcodes/avalara\", \"reaction-auth-net\", \"reaction-auth-net/authnetSettings\", \"reaction-auth-net/authnetPaymentForm\", \"reaction-braintree\", \"reaction-braintree/braintreeSettings\", \"reaction-braintree/braintreePaymentForm\", \"example-paymentmethod\", \"example-paymentmethod/exampleSettings\", \"example-paymentmethod/examplePaymentForm\", \"reaction-paypal\", \"paypal/settings/express\", \"paypal/settings/payflow\", \"reaction-paypal/paypalDone\", \"reaction-paypal/paypalCancel\", \"payment/method/express\", \"payment/method/payflow\", \"reaction-stripe\", \"reaction-stripe/stripeSettings\", \"reaction-stripe/stripePaymentForm\", \"reaction-stripe/stripeConnectAuthorize\", \"reaction-stripe/stripeConnectMerchantSignup\", \"reaction-product-admin\", \"product-detail-simple\", \"reaction-product-variant\", \"reaction-search\", \"reaction-search/searchSettings\", \"reaction-shipping-rates\", \"shipping/settings/flatRates\", \"shipping/flatRates\", \"reaction-shippo\", \"dashboard/shippo\", \"shipping/settings/shippo\", \"reaction-sms\", \"reaction-sms/smsSettings\", \"reaction-social\", \"dashboard/social\", \"reaction-social/socialSettings\", \"reaction-social/reactionSocial\", \"pressed-stripe\", \"taxes-taxcloud\", \"taxes/settings/taxcloud\", \"taxes/taxcodes/taxcloud\", \"reaction-ui-search\", \"Search Modal\", \"ping-server\", \"pressed\", \"pressed-adobe\", \"pressed-analytics\", \"pressed-geolocation\", \"Geolocation\", \"membership-paymentmethod\", \"membership-paymentmethod/membershipPaymentSettings\", \"membership-paymentmethod/membershipPaymentForm\", \"pressed-mercury\", \"pressed-metadata\", \"pressed-postmates\", \"pressed-shipping\", \"pressed-shippo\", \"pressed-stripe-webhooks\", \"pressed-sync-balances\", \"pressed-theme\", \"about\", \"locations\", \"mission\", \"membership\", \"faqs\", \"contact\", \"careers\", \"terms\", \"subscriptions\", \"privacyPolicy\", \"shop\", \"pickup\", \"order\", \"membershipSignUp\", \"paymentSignUp\", \"accountSignUp\", \"inStoreSignUp\", \"inStoreSignUpThankYou\", \"login\", \"cart/myorder\", \"dashboard/pdf/warehouse\", \"signupThankYou\", \"reaction-wish-list\", \"dashboardProductImporter\", \"plans\", \"core\" ] } }",
    "code" : 17280,
    "codeName" : "KeyTooLong"
}
hfmw commented 6 years ago

@zrisher approximately how many shops does this become an issue at?

zrisher commented 6 years ago

@willmoss1000 Per my second message, even with a single shop this is a problem.

Here's our entry for roles.__global_roles__ for our admin user:

[
    "owner",
    "admin",
    "guest",
    "account/profile",
    "product",
    "tag",
    "index",
    "cart/checkout",
    "cart/completed",
    "createProduct",
    "reaction-shipping",
    "shipping",
    "settings/shipping",
    "reaction-accounts",
    "accounts",
    "account/verify",
    "reaction-accounts/accountsSettings",
    "dashboard/accounts",
    "reaction-catalog",
    "catalog/settings",
    "reaction-checkout",
    "reaction-connectors",
    "connectors",
    "settings/connectors",
    "reaction-dashboard",
    "dashboardPackages",
    "dashboard",
    "shopSettings",
    "reaction-discounts",
    "reaction-email",
    "email/settings",
    "reaction-i18n",
    "reaction-i18n/i18nSettings",
    "reaction-layout",
    "reaction-logging",
    "reaction-orders",
    "orders",
    "dashboard/orders",
    "dashboard/pdf/orders",
    "reaction-payments",
    "payments",
    "payment/settings",
    "reaction-revisions",
    "catalog/settings/revisions/general",
    "reaction-router",
    "reaction-notification",
    "notifications",
    "reaction-taxes",
    "taxes",
    "taxes/settings",
    "taxes/settings/rates",
    "reaction-taxes/flatRateCheckoutTaxes",
    "reaction-templates",
    "Templates",
    "templates/settings",
    "templates/settings/email",
    "reaction-ui",
    "reaction-ui/uiDashboard",
    "dashboard/uiThemeDetails",
    "reaction-ui-navbar",
    "reaction-ui-tagnav",
    "reaction-migrations",
    "reaction-analytics",
    "reaction-analytics/reactionAnalytics",
    "dashboard/analytics",
    "reaction-analytics/reactionAnalyticsSettings",
    "reaction-connectors-shopify",
    "settings/connectors/shopify",
    "reaction-default-theme",
    "discount-codes",
    "discount-codes/customDiscountCodes",
    "discount-codes/discountCodesCheckout",
    "reaction-inventory",
    "dashboard/inventory",
    "reaction-jobcontrol",
    "reaction-marketplace",
    "reaction-marketplace/marketplaceShopSettings",
    "marketplaceShops",
    "sellerShopSettings",
    "reaction-marketplace/marketplaceMerchantSettings",
    "taxes-avalara",
    "taxes/settings/avalara",
    "taxes/addressValidation/avalara",
    "taxes/calculation/avalara",
    "taxes/taxcodes/avalara",
    "reaction-auth-net",
    "reaction-auth-net/authnetSettings",
    "reaction-auth-net/authnetPaymentForm",
    "reaction-braintree",
    "reaction-braintree/braintreeSettings",
    "reaction-braintree/braintreePaymentForm",
    "example-paymentmethod",
    "example-paymentmethod/exampleSettings",
    "example-paymentmethod/examplePaymentForm",
    "reaction-paypal",
    "paypal/settings/express",
    "paypal/settings/payflow",
    "reaction-paypal/paypalDone",
    "reaction-paypal/paypalCancel",
    "payment/method/express",
    "payment/method/payflow",
    "reaction-stripe",
    "reaction-stripe/stripeSettings",
    "reaction-stripe/stripePaymentForm",
    "reaction-stripe/stripeConnectAuthorize",
    "reaction-stripe/stripeConnectMerchantSignup",
    "reaction-product-admin",
    "product-detail-simple",
    "reaction-product-variant",
    "reaction-search",
    "reaction-search/searchSettings",
    "reaction-shipping-rates",
    "shipping/settings/flatRates",
    "shipping/flatRates",
    "reaction-shippo",
    "dashboard/shippo",
    "shipping/settings/shippo",
    "reaction-sms",
    "reaction-sms/smsSettings",
    "reaction-social",
    "dashboard/social",
    "reaction-social/socialSettings",
    "reaction-social/reactionSocial",
    "pressed-stripe-webhooks",
    "taxes-taxcloud",
    "taxes/settings/taxcloud",
    "taxes/taxcodes/taxcloud",
    "reaction-ui-search",
    "Search Modal",
    "ping-server",
    "pressed",
    "pressed-adobe",
    "pressed-geolocation",
    "Geolocation",
    "membership-paymentmethod",
    "membership-paymentmethod/membershipPaymentSettings",
    "membership-paymentmethod/membershipPaymentForm",
    "pressed-mercury",
    "pressed-metadata",
    "pressed-postmates",
    "pressed-shipping",
    "pressed-shippo",
    "pressed-stripe",
    "pressed-sync-balances",
    "pressed-theme",
    "about",
    "locations",
    "mission",
    "membership",
    "faqs",
    "contact",
    "careers",
    "terms",
    "shop",
    "pickup",
    "order",
    "membershipSignUp",
    "paymentSignUp",
    "accountSignUp",
    "login",
    "cart/myorder",
    "dashboard/pdf/warehouse",
    "signupThankYou",
    "reaction-wish-list",
    "dashboardProductImporter",
    "plans",
    "core",
    "inStoreSignUp",
    "inStoreSignUpThankYou",
    "pressed-analytics",
    "subscriptions",
    "privacyPolicy"
]

This object alone prevents the field from being indexed.

It seems the only way to fix this problem would be to refactor Reaction's permission system to create less roles, or move to a more relational storage pattern.

Akarshit commented 6 years ago

@zrisher I have also seen this while doing load-testing on reaction. A workaround is to use the "hashed" index instead of MongoDB's default index. The command below creates the index and would reduce the query time significantly. db.users.createIndex({ roles: "hashed" })

brent-hoover commented 6 years ago

Using the hashed index does not seem to solve the problem. Here are the stats we are seeing:

METRICS FOR THESE QUERIES QUERY INEFFICIENCY SCORE 2093430 EXECUTION COUNT 4320 AVERAGE EXECUTION TIME 62224 MS

Akarshit commented 6 years ago

@zenweasel I think these stats would have been while the data was being indexed into the DB, because last time also after I added the index the problem was fixed. I think the Atlas UI shows us any problems in the last 24 hours, which can be changed from a dropdown.

Akarshit commented 6 years ago

I just checked the logs again and yes the execution time is pretty bad(~ 16 secs).

@spencern @ticean maybe this is something you guys would want to look at, improving this could really help the load tests.

spencern commented 6 years ago

@Akarshit this is something we're focusing on, but it may not resolve the issue for existing versions or installs of Reaction.

as @zrisher notes

It seems the only way to fix this problem would be to refactor Reaction's permission system to create less roles, or move to a more relational storage pattern.

We're going to be changing the permissions system and we'll have more details about that soon.

zrisher commented 6 years ago

Great, thanks @spencern!

kieckhafer commented 4 years ago

We've moved away from storing roles on the user in 3.x, and I don't believe this ticket is still relevant. Closing, feel free to re-open if you feel it's still an issue.