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

Incorrect attribute/column type fetched for a group by column on an associated table #30476

Closed
francisli opened this Issue Aug 30, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@francisli

francisli commented Aug 30, 2017

I've just encountered an interesting edge case that breaks grouped count queries in ActiveRecord.

Background: I'm currently in the process of migrating tables from integer to uuid primary keys in my postgresql db. As a result, some tables have an id of integer, and others have an id of uuid. In the following example, Note has a uuid id, and Organization has an integer id. If I execute the following query (simplified from my actual usage, but enough to demonstrate the issue):

Note.joins(:organization).group('organizations.id').count

I'll get a result of something like {nil => 1234}, instead of a collection of many organization ids and their counts.

The reason is, when execute_grouped_calculation is checking for the type of the result set columns:

)

It calls type_for which simply lops off the table name of the column (organization.id into id), and ends up getting the type of id on the Note class, which is uuid, not integer:

The typecast of the integer Organization id to a uuid results in nil for every row in the result set.

My current quick fix solution is to assume any column names with a . in it are references to model tables, and to check the type on that model:

    def type_for(field, &block)
      field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split(".").last
      if field.include?('.')
        field.to_s.split('.').first.classify.constantize.type_for_attribute(field_name, &block)
      else
        @klass.type_for_attribute(field_name, &block)
      end
    end

There's clearly ways that this can also break, and not the most optimal code (splitting the string multiple times, etc), so I didn't think it was necessarily a good solution to submit a pull request for, but this monkey patch gets my code working for now, and I suspect the maintainers will have better ideas...!

@composerinteralia

This comment has been minimized.

Show comment
Hide comment
@composerinteralia

composerinteralia Aug 30, 2017

Member

Any chance you could link to a gist with an executable test case in this format: https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb

Member

composerinteralia commented Aug 30, 2017

Any chance you could link to a gist with an executable test case in this format: https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb

@NoryDev

This comment has been minimized.

Show comment
Hide comment
@NoryDev

NoryDev Nov 2, 2017

I have this exact same bug. It will happen for every group on joined table where both tables have a column with the same name but a different type.

Thanks @francisli for pointing it out. I can confirm that your monkey patch would break some implementations: the argument passed to #group can be a SQL string including a dot. Like:

# count posts per day
Post.group("date_trunc('day', CAST(posts.created_at as timestamp))").count

In this case it would try to "date_trunc('day', CAST(posts".classify.constantize which raise a NameError. I use the following monkey patch, but it's not ideal either:

def type_for(field, &block)
  field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split(".").last
  if field.include?('.') && (field.to_s.split('.').first.classify.constantize rescue false)
    field.to_s.split('.').first.classify.constantize.type_for_attribute(field_name, &block)
  else
    @klass.type_for_attribute(field_name, &block)
  end
end

NoryDev commented Nov 2, 2017

I have this exact same bug. It will happen for every group on joined table where both tables have a column with the same name but a different type.

Thanks @francisli for pointing it out. I can confirm that your monkey patch would break some implementations: the argument passed to #group can be a SQL string including a dot. Like:

# count posts per day
Post.group("date_trunc('day', CAST(posts.created_at as timestamp))").count

In this case it would try to "date_trunc('day', CAST(posts".classify.constantize which raise a NameError. I use the following monkey patch, but it's not ideal either:

def type_for(field, &block)
  field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split(".").last
  if field.include?('.') && (field.to_s.split('.').first.classify.constantize rescue false)
    field.to_s.split('.').first.classify.constantize.type_for_attribute(field_name, &block)
  else
    @klass.type_for_attribute(field_name, &block)
  end
end
@NoryDev

This comment has been minimized.

Show comment
Hide comment

NoryDev commented Nov 2, 2017

@UlyssesInvictus

This comment has been minimized.

Show comment
Hide comment
@UlyssesInvictus

UlyssesInvictus Nov 21, 2017

This seems to be even more general than for group queries. See here: #31196

UlyssesInvictus commented Nov 21, 2017

This seems to be even more general than for group queries. See here: #31196

@rails-bot rails-bot bot added the stale label Feb 20, 2018

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot bot Feb 20, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

rails-bot bot commented Feb 20, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment