Fix the tenant scoping for update_all

[Amit909Singh]

Issue is described here: #195.

delete_all was fixed but update_all still have the tenant scoping issue. Sharing the queries so it is clear what is happening

MultiTenant.with(account) do Project.limit(limit).update_all(name: new_name) end generates a SQL query

"UPDATE "projects" SET "name" = $1 WHERE "projects"."id" IN (SELECT "projects"."id" FROM "projects" WHERE "projects"."account_id" = 1 AND "projects"."account_id" = 1 LIMIT $2)"

As you can see subquery has account_id condition added correctly but once it returns ids, top level query doesnt have account_id condition.

Ideally it should have generated

"UPDATE "projects" SET "name" = $1 WHERE "projects"."id" IN (SELECT "projects"."id" FROM "projects" WHERE "projects"."account_id" = 1 LIMIT $2) AND "projects"."account_id" = 1"

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20d09a8) 84.04% compared to head (054ef82) 84.23%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
+ Coverage   84.04%   84.23%   +0.19%     
==========================================
  Files          16       16              
  Lines         746      755       +9     
==========================================
+ Hits          627      636       +9     
  Misses        119      119              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gurkanindibay]

@Amit909Singh thanks for your contribution

[Laykou]

@Amit909Singh @gurkanindibay In original Rails 7.2 implementation here is condition to if updates.is_a?(Hash) and then there is different logic:

https://github.com/rails/rails/blob/69c86f9606e490b24e8628dc47f34fbec6b321d1/activerecord/lib/active_record/relation.rb#L581

This now creates issue for me when using Rails 7.2 with counter_cache and I tracked it down to this line.
In Rails 7.2 the condition looks like column_count = COALESCE(column_count, 0) + 1 because it is not sanitized with sanitize_sql_for_assignment.

Here with multitenant gem it is always sanitized with sanitize_sql_for_assignment which creates column_count = NULL query.

Do you think we should copy-paste the logic from the original Rails's update_all method?

[Laykou]

@Amit909Singh Can you please take a look at my comment above (also documented here #256 (comment))?

Do you think this creates a problem when updates is passed as a Hash?

Will this also work having multi-column primary key like combination of [setting_tenant_id, uuid]?

[Laykou]

@Amit909Singh @gurkanindibay Do you think it would be better to do it this way - using the original code from RubyOnRails and just overriding the stm.wheres on the last lines:

unless MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?
  stmt.wheres = [generate_in_condition_subquery]
end

This still has a limitation that it does not work with composite primary keys, right?

Full code:

    def update_all(updates)
      raise ArgumentError, 'Empty list of attributes to change' if updates.blank?

      return 0 if @none

      if updates.is_a?(Hash)
        if klass.locking_enabled? &&
          !updates.key?(klass.locking_column) &&
          !updates.key?(klass.locking_column.to_sym)
          attr = table[klass.locking_column]
          updates[attr.name] = _increment_attribute(attr)
        end
        values = _substitute_values(updates)
      else
        values = Arel.sql(klass.sanitize_sql_for_assignment(updates, table.name))
      end

      klass.with_connection do |c|
        arel = eager_loading? ? apply_join_dependency.arel : build_arel(c)
        arel.source.left = table

        group_values_arel_columns = arel_columns(group_values.uniq)
        having_clause_ast = having_clause.ast unless having_clause.empty?
        key = if klass.composite_primary_key?
                primary_key.map { |pk| table[pk] }
              else
                table[primary_key]
              end
        stmt = arel.compile_update(values, key, having_clause_ast, group_values_arel_columns)

        unless MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?
          stmt.wheres = [generate_in_condition_subquery]
        end

        c.update(stmt, "#{klass} Update All").tap { reset }
      end
    end

[Amit909Singh]

@Laykou yes, we need to take care of locking columns. I have the fix for locking column and composite primary keys based on same approach. Please let me know if you want me to take care of it.

[Laykou]

@Amit909Singh I would be glad for your help, since this is new for me. I was just able to get it to the point in my above comment. I'll be happy to review and test and maybe adjust the code if I see some potential improvements. Thank you!

[Amit909Singh]

Sure, I will share the co-authored branch where we can contribute.

[Amit909Singh]

@Laykou , here is the branch. I tried to address locking columns, GroupBy/Having clause and composite primary keys as different commits. Please refer the commits.

I reverted the added test for composite primary keys because active record natively doesn't support composite keys. We need to use gem composite_primary_keys which supports Rails 7.0 max.

[Laykou]

As of Rails 7.1 the composite primary keys are now native. We're doing upgrade to Rails 7.2 so I would be testing this against this version. Is it okay or you intend to support also older Rails versions?

[Amit909Singh]

Sounds good.