-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Using Includes and Unscoped #11036
Comments
/cc @jonleighton |
i'm having this issue with latest version 3.2.17. Preloader class seems to be a work around. |
Is this also failing with Rails 4? |
@rafaelfranca I believe it is still an issue in Rails 4. I posted about a related issue on SO earlier: That's not the SAME issue, but looking for a solution to that issue caused me to bump into this one. What I really need to do is eager load objects and their associations when all objects/associations have a default_scope that I want to ignore. In Rails 3.2.17, I was able to do this with nested unscoped blocks like this:
Both objects's default_scope would be ignored (see SQL in SO post - the default scope is for tenant_id) and I could say "s.checklist" and get the associated checklist. But once I move to Rails 4.1.0.rc1 (and all other versions of 4 that I tried), the same thing would not return the associated checklist because it would fall back on the default_scope when eager loading the checklist (apparently ignoring the nested unscoped block for Checklist). My next move was to try to create an ":unscoped_checklist" association for Submission like this:
This association does work in general (if I grab the submission and say submission.unscoped_checklist, the resulting SQL doesn't include the tenant_id from the default_scope). But if I try to eager load the associated checklist, the query for "Checklist load" uses the Checklist default_scope and is unable to load the checklist. Note that when I call s.checklist, the SQL query is looking at tenant_id because that's the Checklist default_scope, but when I call s.unscoped_checklist, it ignores the default scope. This is Rails 4.1.0.rc1:
So the "unscoped_checklist" association does work by bypassing the default_scope. But if I try to preload the unscoped_checklist association, it reverts to the default_scope:
Note in the Checklist Load query for s.unscoped_checklist, the Checklist default_scope is ignored, but when I try to preload the unscoped_checklist association, you see:
as default_scope is invoked for some reason. I'm happy to help more if I can. I'm new to Rails issues (and Github issues in general) so just tell me what else you need to replicate and I'll do my best. EDIT: I forgot to mention that the Preloader workaround seemed to work for me in Rails 3, but the class behaves differently now in Rails 4. When I used it in Rails 3.2.17, I got this:
So the associated checklist was preloaded as expected. And in Rails 4.1.0.rc1, I get this:
I assume the Preloader has been changed, but I couldn't figure out how to use it from the docs. EDIT 2: For completeness, I figured out the Rails 4 preloaded syntax and thought I should share: Rails 3.2.17
Rails 4.1.0.rc1
|
Could you create a self executable gist like this to show the issue? https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb |
Here you go: https://gist.github.com/JoshDoody/9531033 The description has the lines listed whose assertions should fail: "Failing tests for Rails 4 unscoped when used with eager loading (includes). Two assertions should fail: Line 88 and Line 110. Line 88 should pass in Rails 3, but won't in Rails 4. Line 110 leverages the 'unscoped association' added in Rails 4, but still fails." Let me know how else I can help. |
Thanks! ❤️ |
I found similar problems, gitst: https://gist.github.com/chloerei/9893738 |
Would love this to be sorted out 👍 |
+1 just ran into this today. kinda stuck on how to resolve it |
@calvinl I think the best way to look at it is you can EITHER unscope OR preload associations, but you can't preload unscoped associations. And I believe you can no longer chain unscoped associations either. I wound up creating unscoped associations, and I just call those directly when I need them. I recommend that you use the Rails Console to test any ideas you have to see if they work before trying to incorporate them into your code. Example of unscoped association (which must be called when needed, cannot be preloaded):
It seems like this issue isn't getting much traction, so hopefully we'll get some more +1s. |
@JoshDoody Any clue how I can do that with a polymorphic association? |
@calvinl Hm, I don't really know how you would do that. I'm pretty bad at polymorphism. But I'm glad you asked because this would affect something I have on my list to build, so at least I know this issue is looming there, too. Sorry I can't be of more help there. |
Note: I'm unable to bypass the default scope with the provided work-around, when using a namespaced class. i.e. the unscoped belongs_to association still applies scope to the query for the below convoluted example:
For my required functionality, I don't really need the association to be 'real'; I just need the object in this scenario. I added a method as follows
Not ideal, but works for my situation around this issue. Side note: As to why I'm nesting modules so deeply, it's because I'm relying on engines to provide default functionality to one mount point; and, due to namespacing issues when mounting an engine into multiple different namespaces, I've opted to define the single engine at one mount point, and include its functionality elsewhere via concerns a la TaskRabbit. Would have been dope not to do this, but I would have needed to override or write additional functionality for each mount point anyhow (i.e. not avoiding the nested module ugliness in the parent engine), and this avoids that complexity. |
@rafaelfranca Seems like this is still open and, as far as I can tell, hasn't really gotten much traction. Any idea if this will ever be resolved? It's making my code very, very ugly :) |
No idea. If someone want to work on it, it will be fixed. |
Think I figured it out, will post in a bit. |
Just came across this issue. Took me quite a while to figure out why a relation was |
Why is this issue closed as it's still occurring in Rails 5.0.2 and 5.0.1? Seems @bronzle has provided a fix but it isn't merged in.. @rafaelfranca |
I ran into the same issue on a Spree-based project, and ran through a bunch of failed approaches before implementing this crutch: https://gist.github.com/coderifous/33e24f7e63800e169b03a16eb7eebb5b I preferred this to other approaches because it doesn't involved monkey-patching, or any other clever tricks. It simply clears (and later restores) the In my project, dropped that file in
This was on a Rails 4 codebase, but it looks like it may still be relevant for Rails 5. Hopefully this can be useful for others until a proper solution is figured out. |
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>
The problem is still in 6.0 a work around that seems to work:
The relation is not storing the fact that it was created within a scope, so forcing the resolution of the relation e.g. via There are many other ways to do this, the important part is that the object needs to be retrieved while in the block, is not enough to create the relation in the block (at least that was not clear to me) |
I've been trying to grab a set of
ShipmentItem
with anincludes(:item)
but without the scope:I've also tried this:
I've even tried overriding the
item
method from withinShipmentItem
, but it doesn't work either:Is what I'm trying to do possible? Perhaps I need to restructure my query?
The text was updated successfully, but these errors were encountered: