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
merged 1 commit into from Nov 17, 2016

Conversation

Projects
None yet
5 participants
@kamipo
Member

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

This comment has been minimized.

rails-bot commented Nov 6, 2016

r? @sgrif

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

@kamipo kamipo changed the title from Respect new records for `CollectionProxy#dictinct` to Respect new records for `CollectionProxy#distinct` Nov 6, 2016

@sgrif

This comment has been minimized.

Member

sgrif commented Nov 7, 2016

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

@kamipo

This comment has been minimized.

Member

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)

Respect new records for `CollectionProxy#uniq`
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 kamipo:respect_new_records_for_collection_proxy_distinct branch to 0ec967a Nov 12, 2016

@kamipo kamipo changed the title from Respect new records for `CollectionProxy#distinct` to Respect new records for `CollectionProxy#uniq` Nov 12, 2016

@sgrif sgrif merged commit 07af54d into rails:master Nov 17, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:respect_new_records_for_collection_proxy_distinct branch Nov 17, 2016

@seanlinsley

This comment has been minimized.

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