ErwinM / acts_as_tenant

Easy multi-tenancy for Rails in a shared database setup.
MIT License
1.56k stars 264 forks source link

6.1.2 DEPRECATION WARNING #251

Closed pruzicka closed 10 months ago

pruzicka commented 3 years ago

Hi, this showed thoday in my logs

DEPRECATION WARNING: Merging (("exercises"."organization_id" = $1 OR "exercises"."organization_id" IS NULL)) and ("exercises"."organization_id" = $1) no longer maintain both conditions, and will be replaced by the latter in Rails 6.2. To migrate to Rails 6.2's behavior, use relation.merge(other, rewhere: true).

I believe the this is the cause of it? if ActsAsTenant.configuration.require_tenant && ActsAsTenant.current_tenant.nil? && !ActsAsTenant.unscoped?

excid3 commented 3 years ago

Thanks @pruzicka, definitely want to get that one fixed. Do you have any example code to share that replicates it?

excid3 commented 3 years ago

The test suite with Rails master and Ruby 3 doesn't seem to output any warnings. https://github.com/ErwinM/acts_as_tenant/runs/1875710003?check_suite_focus=true

Run bundle exec rake
/opt/hostedtoolcache/Ruby/3.0.0/x64/bin/ruby -I/home/runner/work/acts_as_tenant/acts_as_tenant/vendor/bundle/ruby/3.0.0/gems/rspec-core-3.10.1/lib:/home/runner/work/acts_as_tenant/acts_as_tenant/vendor/bundle/ruby/3.0.0/gems/rspec-support-3.10.2/lib /home/runner/work/acts_as_tenant/acts_as_tenant/vendor/bundle/ruby/3.0.0/gems/rspec-core-3.10.1/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
..................................................................................

Finished in 0.49165 seconds (files took 4.1 seconds to load)
82 examples, 0 failures

Wondering if this is specific code in your app?

pruzicka commented 3 years ago

Interesting. So Organization is tenant and Exercise class has acts_as_tenant(organization, has_global_records: true, optional: true)

class Organization < ApplicationRecord has_many :exercises

def number_of_exercises exercises.size end

end

When I call organization.number_of_exercises I do get above mentioned deprecation warning message.

excid3 commented 3 years ago

Thanks! I wonder if it's the optional true + has global records.

pruzicka commented 3 years ago

Thanks! I wonder if it's the optional true + has global records.

Well, no. acts_as_tenant(:organization, has_global_records: true) produces exactly same warning :(. ... and how else would you produce global_record with nil as tenant_id if not with optional: true?

jairovm commented 3 years ago

FYI guys, I'm getting same warning message and I think it's related to how AR handles relationships between models plus default_scope, I have these models

class Study < ApplicationRecord
  has_many :flows
end

class Flow < ApplicationRecord
  acts_as_tenant :study, has_global_records: true
end

and when I do this in my rails console current_tenant.flows.count I get this

DEPRECATION WARNING: Merging ((`flows`.`study_id` = ? OR `flows`.`study_id` IS NULL)) and (`flows`.`study_id` = ?)
no longer maintain both conditions, and will be replaced by the latter in Rails 6.2.
To migrate to Rails 6.2's behavior, use `relation.merge(other, rewhere: true)`. (called from <main> at (irb):7)

   (0.9ms)  SELECT COUNT(*)
            FROM `flows`
            WHERE (`flows`.`study_id` = 1 OR `flows`.`study_id` IS NULL) AND `flows`.`study_id` = 1

on the other hand, if I do this Flow.where(study_id: current_tenant.id).count there's no warning message even though the SQL Query is the same

   (1.5ms)  SELECT COUNT(*)
            FROM `flows`
            WHERE (`flows`.`study_id` = 1 OR `flows`.`study_id` IS NULL) AND `flows`.`study_id` = 1

Just came across this line🤔

              # The user has defined their own default scope method, so call that
              evaluate_default_scope do
                if scope = default_scope
                  relation.merge!(scope)
                end
              end