"current_shard" getting added to ActiveRecord::Result to_hash/to_a methods

[swordfish444]

Description

Calling raw sql queries on the ActiveRecord connection with methods such as exec_query and select_all include the @current_shard value in the resulting AtiveRecord::Result object. If you call .to_hash, to_a or to_ary on the result, the current_shard property gets incorrectly merged into the results.

Example

ActiveRecord::Base.connection.select_all("select first_name from users").to_a

=> [{"first_name"=>"Via", "current_shard"=>"readonly"}, {"first_name"=>"Account", "current_shard"=>"readonly"}]

Expected

The resulting array of objects represents the SQL query executed

Actual

The resulting array of objects contains a new property; current_shard

 rails: 5.1.5
 octopus: 0.9.1
 ruby: 2.5.0

[swordfish444]

@thiagopradi We are in the middle of production rollout and if you have any workarounds, let me know. Very much appreciated!

[shyam-habarakada]

@thiagopradi why are we doing this? https://github.com/thiagopradi/octopus/blob/master/lib/octopus/result_patch.rb#L11

[swordfish444]

@thiagopradi @shyam-habarakada This issue stalled our deployment to production and to resolve it I had to comment out the following code in result_patch.rb

module Octopus::ResultPatch
  attr_accessor :current_shard

  # private
  # 
  # def hash_rows
  #   if current_shard.blank?
  #     super
  #   else
  #     foo = super
  #     foo.each { |f| f.merge!('current_shard' => current_shard) }
  #     foo
  #   end
  # end
end

class ActiveRecord::Result
  prepend Octopus::ResultPatch
end

This code was introduced in 382c099

For our use of Octopus, this fixed all of the issues we were experiencing where current_shard was attached to the results. No side-effects could be observed from making this change.

[patbl]

I used the workaround mentioned in the previous comment and it seemed not to cause any problems:

module Octopus::ResultPatch
  private

  def hash_rows
    super
  end
end

But it seems that that behavior must've been added for a reason.

[swordfish444]

@patblWe've been running our Production SaaS with this patch since April and no issues so far. @thiagopradi are you able to comment on whether this will have any adverse effects?

Merging current_shard into the query results seems like a regression as its modifying the results with a property that was likely used at some point/still being used to help facilitate shard management.