-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ActiveRecord: use association's unscope
when preloading
#21550
Conversation
unscope
when preloadingunscope
when preloading
Hi @jbourassa, I think this topic is also addressed here: #11036, #11082, #16531, #18983. Seems like statements with unscoped (or unscope) + preloading are broken in AR4+. |
:class_name => "LazyBlockDeveloperCalledDavid", | ||
:join_table => "developers_projects", | ||
:foreign_key => "project_id", | ||
:association_foreign_key => "developer_id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! 😄 New code added to Rails should use the new hash syntax. foreign_key: "project_id"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I updated the PR accordingly.
Yes, they are still valid in latest 4.2-stable (not tried in master). I, particularly, want to fetch objects via associations like this: has_manny :comments, -> { unscoped } # fetch comments by-passing the default_scope But I didn't found a way to achieve that. So, finally I wrote a gem unscoped_associations, it works for has_many, has_one, belongs_to; but it causes some issues (N+1) on preloading an unscoped association. I also see similar issues in other gems extending/patching AR (zendesk/predictive_load#6, salsify/goldiloader#13), so would be really nice to have this merged it in 👍 and have a clear way to fetch objects via associations by-passing the |
1d99b14
to
d25321b
Compare
@eileencodes Sorry to ping you but I was expecting to hear from @rails-bot, but it's not happening. Any idea what needs to be done to get this in? Thanks! |
Thanks. I added a changelog entry for you as part of the merge commit, please make sure to add one when fixing bugs in the future. |
Also if you notice that after you update a PR that Travis hasn't picked it up, you can help us out by clicking close and re-open which should force it. |
ActiveRecord: use association's `unscope` when preloading
ActiveRecord: use association's `unscope` when preloading
Backported to 4.2 in 338894c |
🎉 awesome, thank you very much @sgrif. I'll make sure to edit the changelog next time. |
Thanks for the great patch! |
I'm using this patch but this still doesn't work:
We have a soft-deleted loyalty membership that is normally hidden by the default scope (
But this couples the referencing object to the column in the membership model, when we would prefer to do:
|
@GuyPaddock Please create a script we can use to reproduce the issue using this template, and open a new issue. |
@GuyPaddock have you found a workaround for unscoping the asosciation? I updated to rails 4.2.5.rc1 but i still wasn't able to do this. |
its still an issue in Rails 4.2.4. Any suggestion, how i can get this working in Rails 4.2.4 ? |
@kashifnaseer this has been released in This may not work with |
Still does not working for this case :( class Developer < ActiveRecord::Base
default_scope -> { where(awesome: [1,2,3]) }
end
class Project < ActiveRecord::Base
has_and_belongs_to_many :developers, -> { unscope(where: :awesome) },
end
mediocre_dev = Developer.create(awesome: 0)
project = Project.create
project.developers << mediocre_dev
project.developers.reload.size
# => 0 — but should be 1
Project.where(id: project.id).includes(:developers).first.developers.size
# => 0 — but should be 1 |
@nosleinfull 🤔 looks like the behaviour you're reporting is triggered regardless of preloading, eg there's no preloading in this piece of code:
right? I tried reproducing in a test on master using very similar setup (default scope with a lamda and |
rails/rails#11036 is still an open Rails bug, as some people mention in rails/rails#21550. The issue is that `includes` doesn't respect `unscoped`. I found a potential solution in rails/rails#11036 (comment) but our friend @markets has a gem, https://github.com/markets/unscoped_associations, that solves that too.
This fixes the RuntimeError we get when accessing deleted variants due to the variant being nil. rails/rails#11036 is still an open Rails bug, as some people mention in rails/rails#21550. The issue is that `includes` doesn't respect `unscoped`. I found a potential solution for the entire app in rails/rails#11036 (comment) but our friend @markets has a gem, https://github.com/markets/unscoped_associations, that solves that too.
This fixes the RuntimeError we get when accessing deleted variants due to the variant being nil. rails/rails#11036 is still an open Rails bug, as some people mention in rails/rails#21550. The issue is that `includes` doesn't respect `unscoped`. I found a potential solution for the entire app in rails/rails#11036 (comment) but our friend @markets has a gem, https://github.com/markets/unscoped_associations, that solves that too.
This fixes the RuntimeError we get when accessing deleted variants due to the variant being nil. rails/rails#11036 is still an open Rails bug, as some people mention in rails/rails#21550. The issue is that `includes` doesn't respect `unscoped`. I found a potential solution for the entire app in rails/rails#11036 (comment) but our friend @markets has a gem, https://github.com/markets/unscoped_associations, that solves that too.
* Make checkout controller add flash error if order contains any type of error Here we add translations for a particular case where the credit card expiry date is in the past * Stop using f_form_for Add labels for some fields, this was done automatically by rails foundation helper * Make checkout controller send bugsnag alerts on every checkout problem There are two new situations here: we will see order.errors after update_attributes fails but before order restart; and we will see how often the order is not complete when the workflow finishes (maybe none) * Add call to $evalAsync() after Loading and FlashLoader are updated so that a angular digest is triggered This is required so that Loading.clear triggers a refresh and makes its placeholder to be cleared * Remove pagination limits in BOM We can re-assess this later, but for now it looks like some of the BOM functionality won't work if results are returned across multiple pages. * Updating translations for config/locales/ca.yml * Updating translations for config/locales/ca.yml * Translate credit card brand so that active merchant code handles the payment correctly Adds a simple console.log statement in case there is an error adding the card * Switch to console.error so we get a bugsnag alert everytime a user has a problem with their card Add paymentMethodsAPI specific mapping function, we had some errors in production with mastercards probably caused by ActiveMerchant not handling the card type correctly * Bump rubocop from 0.80.1 to 0.81.0 Bumps [rubocop](https://github.com/rubocop-hq/rubocop) from 0.80.1 to 0.81.0. - [Release notes](https://github.com/rubocop-hq/rubocop/releases) - [Changelog](https://github.com/rubocop-hq/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v0.80.1...v0.81.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Updating translations for config/locales/pt_BR.yml * Updating translations for config/locales/en_FR.yml * Updating translations for config/locales/fr.yml * Update vendor/assets/angular-google-maps.min.js Fixes an issue where if the js library from maps.googleapis.com failed to load in the <head>, all of our subsequent Angular would completely break. See: https://github.com/angular-ui/angular-google-maps Note: `bluebird.js` is a new dependency of `angular-google-maps.js`. * Make Geo service calls more resilient in /shops page The Geo service is used heavily in the /shops page and especially in the search function. If the google maps js library has failed to load it was throwing a lot of fatal errors, so this change ensures the /shops page can at least: a) load, b) show some shops, and c) search for shops by name (but not location) * Add code comment for dependency * Cache counts used in homepage for 24 hours * Bump rubocop-rails from 2.5.0 to 2.5.1 Bumps [rubocop-rails](https://github.com/rubocop-hq/rubocop-rails) from 2.5.0 to 2.5.1. - [Release notes](https://github.com/rubocop-hq/rubocop-rails/releases) - [Changelog](https://github.com/rubocop-hq/rubocop-rails/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop-rails@v2.5.0...v2.5.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Updating translations for config/locales/de_DE.yml * Updating translations for config/locales/de_DE.yml * Update all locales with the latest Transifex translations * Revert accidental push of a byebug statement with the direct translations push to master 🙈 * Updating translations for config/locales/en_GB.yml * Add product to includes to avoid N+1 queries to fetch products when VO authorization is done right after this * Add comment to better explain variant_override_set.collection_to_delete * Move search params test case to a different context so that we dont have to set the producer of the products in the order This is working in master by chance of the factories but breaks in rails 4 because the orders in this test dont have products supplied by the producer which is a necessary condition in the context where it was * Add missing indexes to spree_orders and spree_products * Load closed shops in a separate request on /shops page * Add api/shops_controller and refactor * Enable Ruby Runtime Metrics in Datadog This automatically collects a bunch of Ruby's GC-related metrics that will come in handy while we tune the Unicorn workers. Some of theres are: * runtime.ruby.class_count * runtime.ruby.gc.malloc_increase_bytes * runtime.ruby.gc.total_allocated_objects * runtime.ruby.gc.total_freed_objects * runtime.ruby.gc.heap_marked_slots * runtime.ruby.gc.heap_available_slots * runtime.ruby.gc.heap_free_slots * runtime.ruby.thread_count Check https://docs.datadoghq.com/tracing/runtime_metrics/ruby/#data-collected for the complete list. The cool thing is that > Runtime metrics can be viewed in correlation with your Ruby services * Re-add setup instructions removed from docker-compose into Dockerfile and Docker.md * Updating translations for config/locales/en_PH.yml * Add optional unicorn-worker-killer configs * Make upper and lower bounds configurable * Bump ddtrace from 0.34.0 to 0.34.1 Bumps [ddtrace](https://github.com/DataDog/dd-trace-rb) from 0.34.0 to 0.34.1. - [Release notes](https://github.com/DataDog/dd-trace-rb/releases) - [Changelog](https://github.com/DataDog/dd-trace-rb/blob/master/CHANGELOG.md) - [Commits](DataDog/dd-trace-rb@v0.34.0...v0.34.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * make shop name a link on /account * Allow changing the connection pool size This allows us to tune for UK. The hypothesis from @kristinalim is: > From what I understand, it can result to Rails processes waiting for each other to complete, while the DB server can take more simultaneous connections. * Memoize OpenFoodNetwork::ScopeProductToHub This means we avoid fetching all of the hub's variants every time we scope a product. Applies to every product loaded when displaying a shops's product list. * Bump oj from 3.10.5 to 3.10.6 Bumps [oj](https://github.com/ohler55/oj) from 3.10.5 to 3.10.6. - [Release notes](https://github.com/ohler55/oj/releases) - [Changelog](https://github.com/ohler55/oj/blob/develop/CHANGELOG.md) - [Commits](ohler55/oj@v3.10.5...v3.10.6) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Updating translations for config/locales/en_FR.yml * Updating translations for config/locales/en_GB.yml * Updating translations for config/locales/de_DE.yml * Updating translations for config/locales/fil_PH.yml * Updating translations for config/locales/fil_PH.yml * Increase max characters for locale in spree_users There are many locales that have six (6) characters. * Fix flicker effect showing 3 buttons when clicking "Show Closed Shops" button * Add loading indicator when showing closed shops * Eager-load variant data for overridable products Cuts query count and page load time in half for this endpoint. * Remove arrows from cart and checkout buttons * Align buttons left and right on cart page * Adjust colours of primary buttons to use (bright) orange instead of (warning) red * Set cart buttons to fixed width and expand when screen is too small * Don't query distributed properties on enterprises that aren't active distributors Cuts page load on /shops by ~75% (with production data) and removes ~300 expensive and superfluous queries. * Updating translations for config/locales/en_NZ.yml * Don't re-use fat serializers when thin ones are needed. This cuts the pageload and query count in half, again. * Only select id in producers query * Bring Spree::Stock::Quantifier in to OFN This is the original unmodified Class from Spree. Modifications added in subsequent commits. * Modify Spree::Stock::Quantifier to not re-fetch stock items if they are already eager-loaded This helps to remove a big N+1 here, and will also be very helpful elsewhere in the app * Remove another N+1 in product serialization * Fix some rubocop issues * Refactor PagedFetcher so it makes one request at a time * Remove track_inventory_levels conditional This value is always true in OFN * Remove unused methods from ProductSimpleSerializer * Depend on a spree version in which spree_core doesnt depend on deface AND remove deface from list of dependencies * Upgrade nokogiri as much as possible (it's not an explicit dependency of OFN and we dont need to control the version now, so I remove it from Gemfile) * Update message for capybara with new upgrade blocker * Delete dead code from products helper * Select only enterprise id * Remove before delivery method in checkout controller, this differentiator is never used in OFN, only in Spree frontend code * Load variants in cart in one query Avoids an N+1 * Eager-load variant stock items Avoids another N+1 * Remove dead code, there's no Spree::Money in app/models/spree and the Spree::Money in lib/spree already has a class_eval with this function * Merge property with property_decorator both in our codebase * Updating translations for config/locales/ca.yml * Bump rubocop-rails from 2.5.1 to 2.5.2 Bumps [rubocop-rails](https://github.com/rubocop-hq/rubocop-rails) from 2.5.1 to 2.5.2. - [Release notes](https://github.com/rubocop-hq/rubocop-rails/releases) - [Changelog](https://github.com/rubocop-hq/rubocop-rails/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop-rails@v2.5.1...v2.5.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Allow User#default_card to work with eager-loading * Eager-load default card in customer serializer * Eager-load addresses in customer serializer * Refactor tag rules loading for customers Fixes N+1 queries on customer tags * Update all locales with the latest Transifex translations * Make sure taggable_type is 'Customer' when querying customer tags * Bump ddtrace from 0.34.1 to 0.34.2 Bumps [ddtrace](https://github.com/DataDog/dd-trace-rb) from 0.34.1 to 0.34.2. - [Release notes](https://github.com/DataDog/dd-trace-rb/releases) - [Changelog](https://github.com/DataDog/dd-trace-rb/blob/master/CHANGELOG.md) - [Commits](DataDog/dd-trace-rb@v0.34.1...v0.34.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Take sku overrides into account in customer totals report * Updating translations for config/locales/es_CR.yml * Updating translations for config/locales/es_CR.yml * Updating translations for config/locales/es_CR.yml * Updating translations for config/locales/pt_BR.yml * Updating translations for config/locales/es_CR.yml * Only load up variant overrides for relevant hubs * Move VariantOverrides class to app/services * Rename VariantOverrides class to VariantOverridesIndexed * Modify interface of VariantOverridesIndexed#indexed Stop using keyword args and accept variant_ids instead of line_items * Improve naming of variables in VariantOverridesIndexed for readability * Switch from old success/error to modern then/catch structure Catch() will get a few more errors then errors() Also, add try/catch inside catch to detect any errors parsing the response error payload * Improve unexpected error handling and add test cases for it * Clear Loading spinner when exception is caught * Make code simpler by extracting methods * If no flash is sent from the server, show the generic error * added missing translations to enterprise_relationship page * Add product translations * Make payment controller authorize stripe_sca payments before processing them or advancing order workflow (that also calls payment.process) * Fix long lines in payments_controller_spec * Fixed display all required tag * Update _enterprise_relationship.html.haml * Updating translations for config/locales/es.yml * Remove old migrations! 🎉 * Added placeholders * Remove superfluous conditional A migration's `up` method is only run when the migration needs to be applied. The only case we could have a higher version number is when a migration with a higher version got merged before the current one. And in that case, we still want this migration to fail, because it hasn't been applied yet. * Format and indent migration message It's much clearer to read this way: ``` == OldMigrationsRemoved: migrating =========================================== rake aborted! StandardError: An error has occurred, this and all later migrations canceled: You haven't updated your dev environment in a long time! Legacy migration files before 2019 have now been removed. Run `rails db:schema:load` before running `rails db:migrate`. ``` * Correct database commands * Update translations from Transifex * Add cache to mirror_db script Also added some better error handling around image syncing. * Update GETTING_STARTED.md Update link to osx catalina setup guide in wiki * Change Result of PriceSack Calculation from Integers to Floats * Add Price Sack Spec for Float Amounts * Move float test to a separate context * Remove some unnecessary code * Split orders_spec in two: tests for orders list page and tests for orders edit page * Skip deleted default_scope in OC notification This fixes the RuntimeError we get when accessing deleted variants due to the variant being nil. rails/rails#11036 is still an open Rails bug, as some people mention in rails/rails#21550. The issue is that `includes` doesn't respect `unscoped`. I found a potential solution for the entire app in rails/rails#11036 (comment) but our friend @markets has a gem, https://github.com/markets/unscoped_associations, that solves that too. * Address some Rubocop violations * Split long method * Memoize result of line items query No need to fetch twice what we just loaded from DB. * Make some specs faster by going directly to the order edit page and move incomplete order spec to a specific context * Preload line item's option_values This fixes an N+1 with the query ```sql SELECT "spree_option_values".* FROM "spree_option_values" INNER JOIN "spree_option_types" ON "spree_option_types"."id" = "spree_option_values"."option_type_id" INNER JOIN "spree_option_values_line_items" ON "spree_option_values"."id" = "spree_option_values_line_items"."option_value_id" WHERE "spree_option_values_line_items"."line_item_id" = 1679 ORDER BY spree_option_types.position asc ``` * Make spec simpler * Replace background with members with before with let statements * Changed translation paths * Fix rubocop issues * Make spec a bit more resilient * Extract print ticket spec to a separate file * Merging 6 specs in one takes around 1 minute of execution time * Do not render inventory items in the shipment that dont have a line item in the order * Added missing translations * Fix sorting of orders (wrong copied function call) * Add spec to cover orders_controller watch sortOptions * Update instagram and linkedin links * Add connect with us footer to customer order confirmation email * Create instagram css property and apply * Remove dead code * Remove dead code Dead since openfoodfoundation#3305 * Make search box font size be 16px so that no zoom happens on iphone * Update all locales with the latest Transifex translations * Eager-load properties in inject_enterprise_and_relatives * Updating translations for config/locales/fr.yml * Updating translations for config/locales/en_FR.yml * Updating translations for config/locales/fr.yml * Fix disappearing tags issue * Add PROFILE var to set production-like settings I took this from a recent newsletter I read. Sometimes replication performance issues locally is actually slower than production due to dev mode settings (code reloading, etc.), heavy de-only gems and the asset pipeline. The PROFILE env var switches these settings all at the same time, giving us an environment closer to production, essential for reliable profiling. Then, rack-mini-profiler is going to be more accurate. Apparently it's something [RubyGems](https://github.com/rubygems/rubygems.org/blob/b026df86ae9df0e4d4c7dae2f220fe2219d52350/config/environments/development.rb#L72-L92) and [CodeTriage](codetriage/CodeTriage@a3c9576) both use. * Use memory cache-store when profiling Or you want see any change when playing fragment-caching or other caching strategies. * Bump mini_racer from 0.2.9 to 0.2.10 Bumps [mini_racer](https://github.com/discourse/mini_racer) from 0.2.9 to 0.2.10. - [Release notes](https://github.com/discourse/mini_racer/releases) - [Changelog](https://github.com/rubyjs/mini_racer/blob/master/CHANGELOG) - [Commits](rubyjs/mini_racer@v0.2.9...v0.2.10) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Fixed missing translation, added keys to en.yml file * Updating translations for config/locales/es_CR.yml * Extract payment method logic from OrderPaymentFinder into the orders helper * Use OrderPaymentFinder methods instead of payments.last and payments.pending.last * Added spec for OrderPaymentFinder * Move method to more generic helper to use it in mailers * Update all locales with the latest Transifex translations * Do not trigger an orderChanged with null quantity When loading the page $watchGroup calls the listener function for every listed line item but with a set variant and null quantity and max_quantity. There's no point on computing an order change when there was none. This saves an empty request on the second most used endpoint of the app, specially busy when users are placing orders. * Remove config and sections related to google analytics in the cookies banner and cookies page * Remove trackers and google analytics * Remove ga_cookies_preference from DB * Add model definition to migration to make migration more resilient * Fix script for syncing public/ in AWS bucket to local * Add missing translations on order list page * Memoize distributor and order_cycle in Api::OrderCyclesController * Return early (before hitting the DB) in complex product list rendering if we already know the order cycle is closed * Avoid needlessly fetching the current user records (for authentication and API key checks) These endpoints are absolutely public, and don't need the current user at any point. * Delete some dead views * Eager-load line_item associations in order * Eager-load and update BasicEnterpriseSerializer * Remove smtp config from admin config page * Add migration to drop dead spree_mail_methods table and some dead mail_methods preferences Co-authored-by: Luis Ramos <luisramos0@gmail.com> Co-authored-by: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Co-authored-by: Transifex-Openfoodnetwork <transifex@openfoodnetwork.org> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Pau Pérez Fabregat <saulopefa@gmail.com> Co-authored-by: Eduardo <jeduardo824@gmail.com> Co-authored-by: Maikel <maikel@email.org.au> Co-authored-by: Kristina Lim <kristinalim.ph@gmail.com> Co-authored-by: Rob H <oeoeaio@gmail.com> Co-authored-by: jeffrey s hill md <jeffreyshillmd@jeffreys-MacBook-Pro.local> Co-authored-by: chrishil1 <chrishil@umich.edu> Co-authored-by: blainebillings <blaine.t.billings@gmail.com> Co-authored-by: David Cook <david@redcliffs.net> Co-authored-by: Lucas Hiago <lucashiago63@gmail.com> Co-authored-by: Robin Klaus <rmklaus12@gmail.com>
Preloading an association that is using
unscope
is dropping theunscope
part.As they say, 10 LoC is worth a thousand words:
This PR fixes that behaviour. This is my first PR on AR (:tada:) so I'm not all that familiar with the internals, let me know if anything seems wrong.