Additional types of ResultSet should not contain keys of #attributes_to_define_after_schema_loads

[DmitryTsepelev]

First of all, let me explain my use case: I have a heavy JSONB DB column which I want to exclude from most of the queries, that's why I've decided to use ignored_columns feature. This column is populated via DB trigger, and sometimes I do want to have it loaded, in such cases I explicitly load it (for instance, using .select("*")). Also, I have a custom ActiveRecord::Type to make interactions with the underlying JSON easier. The problem is that with this combination the value I get when I call that accessor is a value from the DB, custom type is completely ignored.

Here is a simplier example with a string instead of JSON:

class DeveloperName < ActiveRecord::Type::String
  def deserialize(value)
    "Developer: #{value}"
  end
end

class AttributedDeveloper < ActiveRecord::Base
  self.table_name = "developers"

  attribute :name, DeveloperName.new

  self.ignored_columns += ["name"]
end

developer = AttributedDeveloper.create
developer.update_column :name, "name"

loaded_developer = AttributedDeveloper.where(id: developer.id).select("*").first
puts loaded_developer.name # should be "Developer: name" but it's just "name"

Here is a reason why it happens:

1. ignored_columns removes the column from the column_hash
2. #select_all can fill column_types for the result set when DB provides the types (only PostgreSQL does it, so other tests for other DBs cannot reproduce the problem)
3. when #find_by_sql prepares the ActiveRecord::Result, it uses column_hash to remove all the types coming from the DB because we know how to handle them
4. Ignored columns are not in the column_hash so #find_by_sql defines the accessor method with the DB-provided type and custom attribute is ignored

My solution is to remove attributes_to_define_after_schema_loads keys from the column_types along with column_hash keys because having the custom attributes means that we do know how to deserialize this field.

[rafaelfranca]

What you are seeing is the expected behavior in my opinion. Because you ignored the column Rails should not know how to handle it coming from the database.

[DmitryTsepelev]

Thanks for the feedback!

This behaviour seemed confusing to me, because the doc says that "Ignored columns won't have attribute accessors defined", but since we do have the accessor (because of select("*")) I expect it to work :) Also, I believe that behavior should be consistent across the databases - my test passes for MySQL and SQLite

[rafaelfranca]

I agree the behavior should be consistent, so we need to understand why with no changes your test pass in MySQL and SQLite and not in PostgreSQL and make it work in PostgreSQL without adding attributes_to_define_after_schema_loads back to column_hash again before removing it from column_type.

[rafaelfranca]

It seems that it is better to always use the connection.schema_cache.columns_hash here. attributes_to_define_after_schema_loads can contain attributes that are not in the database and we use this just to remove from column_types

[DmitryTsepelev]

I've traced the code to figure out why this issue does not affect other adapters. DatabaseStatements#exec_query returns ActiveRecord::Result, which accepts columns, rows and column_types to it's initializer. PostgreSQL DatabaseStatements provide all three arguments, while MySQL one - only two, so column_types are always empty in that case. Querying#find_by_sql calls result_set.column_types as a part of instantiation.

[rafaelfranca]

Cool! Thank you for investigating. What do you think on my suggestion of always using the column_hash returned by the schema cache on that line?

[DmitryTsepelev]

I've added it to my commit, waiting for the tests to pass :) Thanks for the suggestion

[DmitryTsepelev]

connection.schema_cache.columns_hash works perfectly fine! I only had to add a small warmup setup to the instrumentation test (please let me know if there is a better way to do it) to make sure we have cache initialized before the first test - otherwise, the first event would be about the warmup instead of the desired one. Now all the specs are green

[rafaelfranca]

Great work on this