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

using `#joins` does not imply `readonly = true`. #10769

Merged
merged 2 commits into from May 27, 2013

Conversation

Projects
None yet
4 participants
@senny
Member

senny commented May 27, 2013

Fixes #10615

Before this patch using #joins resulted in a readonly result. However the executed query only selects the columns from the primary table. For example:

Author.joins(:posts) # SELECT authors.* FROM ...

As far as I can tell there is no reason we need to mark these records as readonly.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 27, 2013

Member

@rafaelfranca @jonleighton can you take a look?

Member

senny commented May 27, 2013

@rafaelfranca @jonleighton can you take a look?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 27, 2013

Member

@neerajdotname maybe you could also take a look?

Member

senny commented May 27, 2013

@neerajdotname maybe you could also take a look?

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname May 27, 2013

Member

I personally find that implicit_readonly to be a bit broken in the current state. For example in the following case save! should have blown up. But it does not in master.

class User < ActiveRecord::Base

  has_many :todos

  def self.lab
    User.delete_all
    Todo.delete_all
    user = User.create!
    user.todos.create(title: 'clean house')

    user = User.joins(:todos).select("users.*, todos.title as todos_title").first

    puts user.readonly? #> false
    user.todos_title = 'clean pet'
    user.save! # no error but the title is not changed in the db
  end
end

Finding out all the cases were implicit_readonly should be applied is a bit tricky. So it is best if it is left to the user.

@senny while you are at it, please also cleanup usage of implicit_readonly also from relation.rb file.

Add something like this to change log

Usage of `implicit_readonly` is being removed`. Please use `readonly` method 
explicitly to mark records as `readonly.

Example:
user = User.joins(:todos).select("users.*, todos.title as todos_title").readonly(true).first
user.todos_title = 'clean pet'
user.save! # will raise error

I'm +1 on this PR.

Member

neerajdotname commented May 27, 2013

I personally find that implicit_readonly to be a bit broken in the current state. For example in the following case save! should have blown up. But it does not in master.

class User < ActiveRecord::Base

  has_many :todos

  def self.lab
    User.delete_all
    Todo.delete_all
    user = User.create!
    user.todos.create(title: 'clean house')

    user = User.joins(:todos).select("users.*, todos.title as todos_title").first

    puts user.readonly? #> false
    user.todos_title = 'clean pet'
    user.save! # no error but the title is not changed in the db
  end
end

Finding out all the cases were implicit_readonly should be applied is a bit tricky. So it is best if it is left to the user.

@senny while you are at it, please also cleanup usage of implicit_readonly also from relation.rb file.

Add something like this to change log

Usage of `implicit_readonly` is being removed`. Please use `readonly` method 
explicitly to mark records as `readonly.

Example:
user = User.joins(:todos).select("users.*, todos.title as todos_title").readonly(true).first
user.todos_title = 'clean pet'
user.save! # will raise error

I'm +1 on this PR.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 27, 2013

Member

@senny is it marked as readonly with explicit select for a join table field?

Member

rafaelfranca commented May 27, 2013

@senny is it marked as readonly with explicit select for a join table field?

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname May 27, 2013

Member

@rafaelfranca if there is any select then @implicit_readonly = false.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L964

Parsing the select statement to see which of the columns belong to primary table and which ones belong to join table will be brittle and error-prone. Hence my take that lets leave it upto the user.

Member

neerajdotname commented May 27, 2013

@rafaelfranca if there is any select then @implicit_readonly = false.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L964

Parsing the select statement to see which of the columns belong to primary table and which ones belong to join table will be brittle and error-prone. Hence my take that lets leave it upto the user.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 27, 2013

Member

Parsing the select statement to see which of the columns belong to primary table and which ones belong to join table will be brittle and error-prone. Hence my take that lets leave it upto the user.

👍 to this as it's the approach taken with the references method added to the Active Record Query API.

Member

pixeltrix commented May 27, 2013

Parsing the select statement to see which of the columns belong to primary table and which ones belong to join table will be brittle and error-prone. Hence my take that lets leave it upto the user.

👍 to this as it's the approach taken with the references method added to the Active Record Query API.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 27, 2013

Member

@neerajdotname I removed the references from relation.rb can you take another look?

Member

senny commented May 27, 2013

@neerajdotname I removed the references from relation.rb can you take another look?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 27, 2013

Member

Seems good. We need to add this to the upgrading guide too. (Better to start soon)

Member

rafaelfranca commented May 27, 2013

Seems good. We need to add this to the upgrading guide too. (Better to start soon)

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname May 27, 2013

Member

@senny looks good. +1 from me.

Member

neerajdotname commented May 27, 2013

@senny looks good. +1 from me.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 27, 2013

Member

@rafaelfranca what do you want to have covered in the guides? you no longer need readonly when joins was called. You should apply it when you use a custom select but you would have to do that until now. As I see it there is not really a change you need to upgrade to.

Member

senny commented May 27, 2013

@rafaelfranca what do you want to have covered in the guides? you no longer need readonly when joins was called. You should apply it when you use a custom select but you would have to do that until now. As I see it there is not really a change you need to upgrade to.

rafaelfranca added a commit that referenced this pull request May 27, 2013

Merge pull request #10769 from senny/10615_join_should_not_return_rea…
…donly_records

using `#joins` does not imply `readonly = true`.

@rafaelfranca rafaelfranca merged commit 5a687e9 into rails:master May 27, 2013

@senny senny deleted the senny:10615_join_should_not_return_readonly_records branch May 27, 2013

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