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

Counter cache seems to broken in 4.2.1 #19437

Closed
kapso opened this issue Mar 21, 2015 · 20 comments
Closed

Counter cache seems to broken in 4.2.1 #19437

kapso opened this issue Mar 21, 2015 · 20 comments
Milestone

Comments

@kapso
Copy link

kapso commented Mar 21, 2015

When I create a new Address object I see the following error - ActiveModel::MissingAttributeErrormissing attribute: address_count

This was obviously working in 4.2.0

class Address < ActiveRecord::Base
  belongs_to :user, counter_cache: :address_count
end
@rafaelfranca
Copy link
Member

Does the address_count column exists?

@kapso
Copy link
Author

kapso commented Mar 21, 2015

Yes it does, same code works in 4.2.0

@rafaelfranca
Copy link
Member

Could you create an reproduction script with this template https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb?

@rafaelfranca rafaelfranca added this to the 4.2.2 milestone Mar 21, 2015
@rohitpaulk
Copy link
Contributor

I can't seem to reproduce this.

https://gist.github.com/rohitpaulk/25726816c1e3692510ba

^ That works on both 4.2.0 and 4.2.1.

@kapso
Copy link
Author

kapso commented Mar 22, 2015

This is on Postgres & Ruby 2.2.1

  • postgres_ext (2.4.1)
  • pg (0.18.1)
ERROR! ActiveModel::MissingAttributeError: missing attribute: address_count
/Users/kapil/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/activerecord-4.2.1/lib/active_record/attribute_methods/read.rb:93:in `block in _read_attribute'
/Users/kapil/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/activerecord-4.2.1/lib/active_record/attribute_set.rb:31:in `block in fetch_value'
/Users/kapil/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/activerecord-4.2.1/lib/active_record/attribute.rb:150:in `value'
/Users/kapil/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/activerecord-4.2.1/lib/active_record/attribute_set.rb:31:in `fetch_value'
/Users/kapil/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/activerecord-4.2.1/lib/active_record/attribute_methods/read.rb:93:in `_read_attribute'

@rohitpaulk
Copy link
Contributor

Modified code from the gist above to use:

ActiveRecord::Base.establish_connection(
  adapter: 'postgresql',
  database: 'rails_test',
  host: 'localhost',
  username: 'test',
  password: 'test'
)

Tests still pass.

Here's my Gemfile:

gem 'rails', '4.2.1'
gem 'pg', '0.18.1'
gem 'postgres_ext', '2.4.1'

@kapso - Can you try running the script and confirm whether the problem occurs?

@kapso
Copy link
Author

kapso commented Mar 23, 2015

So I found the issue -- in the User model I have a default_scope defined which did not have counter cache column, after adding address_count it works now. Again this wasn't the behavior in 4.2.0, where _read_attribute was not error'ing out.

This does not work in 4.2.1 -

default_scope { select(:id, :name) }

This works in 4.2.1 -

default_scope { select(:id, :name, :address_count) }

@rafaelfranca
Copy link
Member

Agree that the behavior have changed but I think the current behavior is correct. Adding a select as default_scope is dangerous and for me it should always be respected. So if you asking explicitly to always query only these two attributes Rails should respect it. This way the behavior is always predictable.

Worth to investigate why it was working before and maybe revert it on the stable branch, but I'd expect this change to be present on Rails 5.

@kapso
Copy link
Author

kapso commented Mar 23, 2015

The problem is that in lot of places we do explicit .select scope so like User.select(:id, :name) ...this is to avoid loading all columns when we really dont need them. So essentially this change means, its not recommended to follow such a pattern?

@user = User.select(:id).find(1)
@user.addresses.create(address_hash)

in the above case address creation will not work unless we do this - User.select(:id, :address_count) which is again different from pre 4.2.1 versions.

@rafaelfranca
Copy link
Member

I don't see why to not recommend to follow such pattern, you can still do it, but remember to load all the columns you will need, and in your case, you are needed the counter cache column.

again, I agree it is different from pre 4.2.1 version, never sais that is not, just that this new behavior does make more sense to me, but it is worth to investigate why this was working, why it was changed before taking any decision.

@kapso
Copy link
Author

kapso commented Mar 23, 2015

Agree, I think an explanation from the community on why this behavior was changed would be nice :)

@rafaelfranca
Copy link
Member

@kapso don't worry, I think we are going to revert on 4.2.2, but maybe we will keep it for 5.0.

@sgrif
Copy link
Contributor

sgrif commented Mar 23, 2015

I still need a full stack trace from the actual error to fix it for 4.2.2

@sgrif
Copy link
Contributor

sgrif commented Mar 23, 2015

Never mind, I found where we're reading it. The behavior was changed to fix #18689

@sgrif
Copy link
Contributor

sgrif commented Mar 23, 2015

Fixed for 4.2.2 by 0e2e9e2. I agree with @rafaelfranca that we should leave the behavior as is on master. If we change our minds, however, that change should cherry pick cleanly.

@sgrif sgrif closed this as completed Mar 23, 2015
@kapso
Copy link
Author

kapso commented Mar 24, 2015

Thanks! What is the timeline for 4.2.2?

@rafaelfranca
Copy link
Member

@kapso we usually release one new version each month, but no promises

@jaredbeck
Copy link
Contributor

@rafaelfranca @sgrif I'm really impressed by your dedication to semver here. A lot of other projects would not go through the trouble to patch such an edge-case, especially when the new behavior is better.

@kapso
Copy link
Author

kapso commented Jun 19, 2015

@rafaelfranca looks like this wasnt reverted in 4.2.2 ?

@rafaelfranca
Copy link
Member

4.2.2 was a security release, it only include the commits to fix the security issues

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

5 participants