Skip to content
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

Unexpected Type Casting in ActiveRecord::Calculations#pluck #28044

Closed
awmichel opened this issue Feb 17, 2017 · 8 comments
Closed

Unexpected Type Casting in ActiveRecord::Calculations#pluck #28044

awmichel opened this issue Feb 17, 2017 · 8 comments

Comments

@awmichel
Copy link

Steps to reproduce

  1. Create a model Post with integer-based primary keys.
  2. Create a model Comment with UUID-based primary keys.
  3. A Post should have many Comments and a Comment should belong to a Post.
  4. Attempt to pluck an array of Post and Comment IDs using Post.joins(:comments).pluck('posts.id', 'comments.id').

See the following code for an example and test case: https://gist.github.com/awmichel/ba34381fe418f761932b12ef8ff2063f

Expected behavior

The expected result is an array of arrays containing pairs of integer and UUID primary keys belonging to Post and Comment records.

Ex: [[1, "316d1975-6126-4475-bf8b-9c32ce4ee7a6"]]

Actual behavior

The actual behavior is an array of arrays containing pairs of integer and truncated UUID primary keys belonging to Post and Comment records.

Ex: [[1, 316]]

System configuration

Rails version: 5.0.1
Ruby version: 2.4.0

Additional Information

I came across this bug while debugging an exception thrown in cerebris/jsonapi-resources. The exception was undefined method `_type' for NilClass:Class. After spending some time debugging, I was able to trace the exception to this line which attempts to pluck model IDs and timestamps of the records it returns.

So far I've been able to track the bug down to this line. The result just before that line contains the full UUID of the related records. After calling cast_values the UUID is truncated down to the first X digits in the UUID.

@awmichel awmichel changed the title Unexpected Type Casting in ActiveRecord::Calculations#pluck Unexpected Type Casting in ActiveRecord::Calculations#pluck Feb 17, 2017
@alexcameron89
Copy link
Member

It seems to be happening because line 87 changes the type of the Comments column from ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Uuid to an ActiveModel::Type::Integer since Post has an attribute_type for id of ActiveModel::Type::Integer. The different id types are conflicting.

I don't see any examples of pluck being used with a join, but the alternative Post.joins(:comments).ids is having a similar issue.

@glupo
Copy link

glupo commented Feb 17, 2017

This actually comes both from adapter and type casting inside of pluck method. Adapter uses column names(which are the same in this case) for type definition. Type overrides for cast_values method are taken from initial model class. As a result types get mixed up and comments.id is casted to integer.

To work this around you can provide different names for identical named columns like this:
Post.joins(:comments).pluck('posts.id', 'comments.id as c_id')

@rails-bot
Copy link

rails-bot bot commented May 23, 2017

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 as completed May 30, 2017
@why-el
Copy link
Contributor

why-el commented Aug 10, 2017

This is still a problem in master. Below is a reproduction:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "activerecord", github: 'rails/rails' 
  gem "arel", github: 'rails/arel'
  gem "pg"
  gem 'byebug'
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "scratch")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  enable_extension 'uuid-ossp'
  enable_extension 'pgcrypto'

  create_table :users, force: true do |t|
  end

  create_table :profiles, force: true, id: :uuid do |t|
    t.integer :user_id
  end
end

class User < ActiveRecord::Base
  has_one :profile
end

class Profile < ActiveRecord::Base
  belongs_to :user
end

class BugTest < Minitest::Test
  def test_plucking_ids
    user1 = User.create!

    profile1 = Profile.create!(user_id: user1.id)

    assert_equal User.includes(:profile).pluck(:id), [user1.id, profile1.id]
  end
end

@DamienMetzger
Copy link

This is a HUGE issue as it silently cast UUID and create a collision issue, mixing records.

@knu
Copy link
Contributor

knu commented Nov 22, 2019

@why-el This is a nice repro, thanks!

I guess this line:

assert_equal User.includes(:profile).pluck(:id), [user1.id, profile1.id]

should read:

assert_equal User.joins(:profile).pluck('users.id', 'profiles.id'), [[user1.id, profile1.id]]

@willtcarey
Copy link
Contributor

A very similar reproduction that silently corrupts data. When a model has an integer ID field, it can still have a belongs_to relationship to a model that uses a UUID for the primary key and when the model saves the UUID will be truncated because it gets casted to an integer. We ran across this when using ActiveStorage and putting has_one_attached on a model which used a UUID primary key so this can happen even between code you don't own.

I'm thinking Rails could throw an exception when you set up the belongs_to relationship if one side of the relationship is a integer and the other is a UUID.
@rafaelfranca would you accept a pull request that did that?

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "activerecord", github: 'rails/rails' 
  gem "arel", github: 'rails/arel'
  gem "pg"
  gem 'byebug'
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "scratch")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  enable_extension 'uuid-ossp'
  enable_extension 'pgcrypto'

  create_table :users, force: true, id: :uuid do |t|
  end

  create_table :profiles, force: true do |t|
    t.integer :user_id
  end
end

class User < ActiveRecord::Base
  has_one :profile
end

class Profile < ActiveRecord::Base
  belongs_to :user
end

class BugTest < Minitest::Test
  def test_plucking_ids
    user1 = User.create!
    profile1 = Profile.create!(user: user1)

    assert_equal user1.id, profile1.user_id
  end
end

@kamipo
Copy link
Member

kamipo commented Aug 16, 2020

Fixed by #39264.

@kamipo kamipo closed this as completed Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants