Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Behavior of #select changed #8797

Closed
dhh opened this Issue · 8 comments

5 participants

@dhh
Owner
dhh commented

3.2.9:

irb(main):003:0> Account.new.groups.select("count(groups.id) total, max(groups.updated_at) max_updated_at").first.max_updated_at
  Group Load (0.3ms)  SELECT count(groups.id) total, max(groups.updated_at) max_updated_at FROM `groups` WHERE `groups`.`account_id` = NULL LIMIT 1
=> nil

4.0.0.beta:

irb(main):001:0> Account.new.groups.select("count(groups.id) total, max(groups.updated_at) max_updated_at").first.max_updated_at
NoMethodError: undefined method `max_updated_at' for nil:NilClass
    from (irb):1
    from /Users/david/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/bundler/gems/rails-e63e280bed3a/railties/lib/rails/commands/console.rb:88:in `start'
    from /Users/david/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/bundler/gems/rails-e63e280bed3a/railties/lib/rails/commands/console.rb:9:in `start'
    from /Users/david/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/bundler/gems/rails-e63e280bed3a/railties/lib/rails/commands.rb:71:in `<top (required)>'
    from ./bin/rails:4:in `require'
    from ./bin/rails:4:in `<main>'

I had code that relied on the first thing working. You can certainly argue that it's a tad dodgy to rely on select working like this, but I'm not sure that's compelling enough to break backwards compatibility.

Was this change intentional or a biproduct of something else?

@jonleighton jonleighton was assigned
@al2o3cr

Wild guess - this is probably related to the solution in #5215.

@vipulnsward

Yes 0130c17 did this
//cc @jonleighton

@jonleighton
Collaborator

@dhh as noted above, we now don't issue any db query for collection associations where the owner is a new_record?. Which is presumably why this breaks: because your example uses Account.new.

I guess the solution is to prevent the optimisation happening when select has been used.

@dhh
Owner
@jonleighton
Collaborator

@dhh We were just discussing in basecamp. actually I misunderstood the issue.

The new behaviour is designed so that Account.new.groups == [], even if you happen to have groups in your database with account_id = NULL.

So actually I see it as counterintuitive that Account.new.groups would ever return records (which it might in 3.2). The change in 4.0 is designed to fix is so that Account.new.groups would only ever return records that have been built in memory, not ones that might exist in the db with null foreign keys.

So I don't think we should change anything about the 4.0 behaviour actually.

@fxn
Owner

Yep I agree. To me Article.new represents a brand new model. Orphan groups in the database do not belong to this object in my mind.

@dhh
Owner
@jonleighton
Collaborator

Ok, closing then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.