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

include? in ActiveRecord::Associations::CollectionProxy return 1 instead of true #10979

Closed
achiinto opened this issue Jun 17, 2013 · 6 comments · Fixed by #10987
Closed

include? in ActiveRecord::Associations::CollectionProxy return 1 instead of true #10979

achiinto opened this issue Jun 17, 2013 · 6 comments · Fixed by #10987

Comments

@achiinto
Copy link

Foo has_many Bars
Foo.first.bars.include? bar

If false, it returns false
If true, it returns 1

When I do this,

the_bars = Foo.first.bars
the_bars.include? bar

if true, it returns true.

I am reproducing this in Rails 4.

@senny
Copy link
Member

senny commented Jun 18, 2013

I'll take a look.

@senny
Copy link
Member

senny commented Jun 18, 2013

I can confirm this behavior, I rewrote the has_many_association tests with the following result

HasManyAssociationsTest#test_include_checks_if_record_exists_if_target_not_loaded [test/cases/associations/has_many_associations_test.rb:1379]:
Expected: true
  Actual: 1


  2) Failure:
HasManyAssociationsTest#test_include_returns_false_for_non_matching_record_to_verify_scoping [test/cases/associations/has_many_associations_test.rb:1401]:
Expected: false
  Actual: nil


  3) Failure:
HasManyAssociationsTest#test_included_in_collection [test/cases/associations/has_many_associations_test.rb:1223]:
Expected: true
  Actual: 1

@senny
Copy link
Member

senny commented Jun 18, 2013

I did some research and it's enough if question mark methods return a truthy or falsy value. you can read more on that discussion on this PR: #5329

I'm closing this one as the described behavior is alright. Thanks for reporting 💛

@senny senny closed this as completed Jun 18, 2013
@fxn
Copy link
Member

fxn commented Jun 18, 2013

This one may be different, because this predicate for whatever reason documents that the singleton true is returned (it is not the way we normally document predicates).

I would revise in this case the return value to keep backwards compatibility (instead of relaxing the contract).

@senny senny reopened this Jun 18, 2013
@senny
Copy link
Member

senny commented Jun 18, 2013

@fxn thanks, I'm on it.

@fxn
Copy link
Member

fxn commented Jun 18, 2013

👍

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

Successfully merging a pull request may close this issue.

3 participants