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

Respect new records for CollectionProxy#uniq #26980

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Nov 6, 2016

Currently if CollectionProxy has more than one new record,
CollectionProxy#uniq result is incorrect.

And CollectionProxy#uniq was aliased to distinct in a1bb6c8.
But the uniq method and the SELECT DISTINCT method are different
methods. The doc in CollectionProxy is for the SELECT DISTINCT
method, not for the uniq method.

Therefore, reverting the alias in CollectionProxy to fix the
inconsistency and to have the both methods.

@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@kamipo kamipo changed the title Respect new records for CollectionProxy#dictinct Respect new records for CollectionProxy#distinct Nov 6, 2016
@sgrif
Copy link
Contributor

sgrif commented Nov 7, 2016

I would have expected this code to just do SELECT DISTINCT and not do it in memory.

@kamipo
Copy link
Member Author

kamipo commented Nov 7, 2016

This method exists since the initial commit.

def uniq(collection = self)
collection.inject([]) { |uniq_records, record| uniq_records << record unless uniq_records.include?(record); uniq_records }
end

SELECT DISTINCT method was added since v3.2.0.rc1.
562583c#diff-bf6dd6226db3aab589916f09236881c7R190

If we expects SELECT DISTINCT, should we remove the method? (CollectionProxy already have SELECT DISTINCT method because inherits Relation but overridden by the method)

Currently if `CollectionProxy` has more than one new record,
`CollectionProxy#uniq` result is incorrect.

And `CollectionProxy#uniq` was aliased to `distinct` in a1bb6c8.
But the `uniq` method and the `SELECT DISTINCT` method are different
methods. The doc in `CollectionProxy` is for the `SELECT DISTINCT`
method, not for the `uniq` method.

Therefore, reverting the alias in `CollectionProxy` to fix the
inconsistency and to have the both methods.
@kamipo kamipo force-pushed the respect_new_records_for_collection_proxy_distinct branch from 68ff93d to 0ec967a Compare November 12, 2016 21:22
@kamipo kamipo changed the title Respect new records for CollectionProxy#distinct Respect new records for CollectionProxy#uniq Nov 12, 2016
@sgrif sgrif merged commit 07af54d into rails:master Nov 17, 2016
@kamipo kamipo deleted the respect_new_records_for_collection_proxy_distinct branch November 17, 2016 20:08
@seanlinsley
Copy link
Contributor

seanlinsley commented Mar 17, 2017

It would be useful to backport this to 5.0.x. Here's my failing test and the git bisect setup I used to discover this commit fixed it:

# Note that "good" and "bad" are reversed, because `git bisect` is only built to work in one direction.
# I use the trick shown here: http://stackoverflow.com/a/36157747/2651774 to get around that.

# git bisect start
# git bisect bad master
# git bisect good 5-0-stable^
# git bisect skip
# git bisect run bash -c "! ruby test.rb"

# fixed commit: 0ec967aa6655d62c92f72acb8d556a5b3f70762d

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 "rails", path: "."
  gem "sqlite3"
end

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

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

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
    t.string :comment
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments << Comment.create!(comment: 'test')
    post.comments << Comment.create!(comment: 'test')

    assert_equal ['test', 'test'], Post.last.comments.pluck(:comment)
    assert_equal ['test'], Post.last.comments.distinct.pluck(:comment)
  end
end

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

Successfully merging this pull request may close these issues.

None yet

5 participants