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

ActiveRecord belongs_to update foreign_key not validated. #26557

Closed
develop-test1 opened this issue Sep 20, 2016 · 15 comments
Closed

ActiveRecord belongs_to update foreign_key not validated. #26557

develop-test1 opened this issue Sep 20, 2016 · 15 comments

Comments

@develop-test1
Copy link

Steps to reproduce

hi, I'm having problem with Rails 5 validation on updating association foreign key on the belongs_to object. I have Post and Author models, where Post belongs to Author. I'm trying to update Post Author via strong params:
params.require(:post).permit(:author_id).

However the author_id is not validated properly, so I can assign author_id that is invalid (Author of that ID doesn't exist). In Rails 4 this would raise 404 not found exception, but Rails 5 just assigns wrong foreign key. Is this desired behaviour?

Expected behavior

404 Not found exception should be raised.

Actual behavior

Invalid foreign_key id is being saved to the DB without validation.

System configuration

Rails version:
5.0.0.rc1
Ruby version:
ruby 2.2.3p173

@nynhex
Copy link

nynhex commented Sep 20, 2016

Can you provide a sample app that mimics the behavior?

@mechanicles
Copy link
Contributor

@develop-test1 Based on your given steps, I was not able to figure out how to trace it out properly.

It would be good if you provide more details about your association logic for Post & Author models.

Still, I went further and I tried following steps and it worked as expected.

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"
  # Activate the gem you are reporting the issue against.
  gem "activerecord", "5.0.0.rc1"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.belongs_to_required_by_default = true

ActiveRecord::Schema.define do
  create_table :authors, force: true do |t|
  end

  create_table :posts, force: true do |t|
    t.integer :author_id
  end
end

class Author < ActiveRecord::Base
  has_many :posts
end

class Post < ActiveRecord::Base
  belongs_to :author
end

class BugTest < Minitest::Test
  def test_association_stuff
    author= Author.create!
    post = Post.create!(author_id: author.id)
    assert_raises(ActiveRecord::RecordInvalid) do
      post.update!(author_id: 0) # Invalid id which is not present in the database
    end
  end
end

Above test passed successfully.
You might have passed optional: true parameter in your belongs_to method which causes this issue.

class Post < ActiveRecord::Base
  belongs_to :author, optional: true
end

Could you check that and let us know?

@develop-test1
Copy link
Author

Thank you @mechanicles for quick response.

I've checked and my model and I can confirm that it doesn't have optional: true attribute.

However if I set-up up with required attribute:

class Post < ActiveRecord::Base
  belongs_to :author, required: true
end

in fact i get expected behaviour:
{ "status": 422, "error": "Unprocessable Entity", "exception": "#<ActiveRecord::RecordInvalid: Validation failed: Author must exist>",

However seeting required: true restricts me from nullifying Author for the Post on updates. And one of the requirements is ability to set Author of the Post to null. Is there a way to validate foreign_key object, only if author_id is not null?

That way I could only allow for author_id to be null or be a valid id.

@develop-test1
Copy link
Author

develop-test1 commented Sep 21, 2016

Moreover it appears that changing (as described here: #18937):
/config/initializers/active_record_belongs_to_required_by_default.rb
Rails.application.config.active_record.belongs_to_required_by_default = true # Rails.application.config.active_record.belongs_to_required_by_default = false
has no effect on the behaviour.
It works only if I use belongs_to :author, required: true within the model itself.

@develop-test1
Copy link
Author

Based on this:

http://blog.bigbinary.com/2016/02/15/rails-5-makes-belong-to-association-required-by-default.html

My model behaves like Rails 4 model, however I'm using Rails 5. Any ideas?

@prathamesh-sonpatki
Copy link
Member

The blog post is old before we introduced https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults.rb.tt

I think if you have updated the app from Rails 4 to Rails 5, then you have

Rails.application.config.active_record.belongs_to_required_by_default = false

in your config/initializers/new_framework_defaults.rb which is why you have Rails 4 behavior.

If you want the Rails 5 behavior, then change it to

Rails.application.config.active_record.belongs_to_required_by_default = true

As such we need more details on what is exactly not working in this issue. please use http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#create-an-executable-test-case to create an bug test case or upload a sample application to github reproducing the issue. Thanks!

@develop-test1
Copy link
Author

Ok, I've found out what was the source of this behaviour.

It appears that:
gem 'deep_cloneable', '~> 2.2.1'

Causes this behavior. Removing this gem brought back expected behavior.

@prathamesh-sonpatki
Copy link
Member

Great, I am closing this issue as it is not related to Rails. Thanks for confirming!

@develop-test1
Copy link
Author

Hold on, removing this gem didn't fix the problem completely. Let me double check just to be 100% sure.

@develop-test1
Copy link
Author

develop-test1 commented Sep 21, 2016

Ok, so it appears that both:
# gem 'deep_cloneable', '~> 2.2.1'
and
# gem 'ransack', github: 'activerecord-hackery/ransack'
break the desired behaviour.

It appears that new Rails 5 defaults are not so much backwards compatible.

Anyhow, thanks all for help.

@mechanicles
Copy link
Contributor

mechanicles commented Sep 21, 2016

@develop-test1 If I got you correctly, you want to store author_id which should be valid, valid in the sense, it should be present in authors table, right? and you also want to store it as nil (NULL). If this is so, there is a work around like this,

class Post < ActiveRecord::Base
  belongs_to :author, optional: true

  validate :author_id_correctness

  def author_id_correctness
    if author_id.present? && Author.find_by(id: author_id).nil?
      errors.add(:author_id, "is invalid")
    end
  end
end

@develop-test1
Copy link
Author

Awesome! Thank you @mechanicles

@develop-test1
Copy link
Author

By the way, couldn't this workaround be available as a part of ActiveRecord methods? I.e:
belongs_to :author, optional: true, validated: true

mechanicles added a commit to mechanicles/rails that referenced this issue Sep 21, 2016
…rity.

By adding this support, model can able to save foreign_key to either
`nil` value or to the valid foreign_key value which means
foreign_key value must present in the foreign table.

    class Post < ActiveRecord::Base
      belongs_to :author, optional: :with_integrity
    end

    author = Author.create!
    post = Post.create!(author_id: author.id)

    post.author_id = 0 # Invalid id which is not present in database
    post.save! # Should raise 'ActiveRecord::RecordInvalid' error

    post.author_id = nil
    post.save! # No Error

Based on this issue's discussion rails#26557. Added this.
@stratigos
Copy link

I think the config.active_record.belongs_to_required_by_default might be overridden (or at least, its associated functionality) by some common gems, as Im seeing this issue myself in a brand new Rails 5.0.1 app with :belongs_to_required_by_default initialized to true. I am using devise, and I hear its exhibited when devise is used, as well as delayed_job and bugsnag.

It took me a while to find the relevant info across a few Github issues, so perhaps some attention should be called to this in Rails Guides?


I have an extremely simple application which is actually using devise_token_auth, which uses devise. The rest of my app, which is Rails 5.0.1, is extremely simple and has no significant custom logic built into any AR models. All of my :belongs_to associations can save 0, NULL, etc, for the FK. I can fix by forcing the association to be present in each model for now.

In any event, just posting this so there is some awareness that this issue is more widespread than just @develop-test1

@Viktor-Ivliev
Copy link

Viktor-Ivliev commented May 10, 2018

there is a minor bug 5.1.6

class DeviceCommand < ActiveRecord::Base
  belongs_to :device_command_group
  validates :device_command_group, presence: true
end

class DeviceCommandGroup < ActiveRecord::Base
  has_many :device_commands, dependent: :destroy
end
group  = DeviceCommand.create(name: 'Group 1')
strange_init_command  = group.device_commands.build(name: 'test', command: 'bal-bla')
strange_init_command.device_command_group_id = nil
strange_init_command.valid? = > true

although it is working:

class DeviceCommand < ActiveRecord::Base
  belongs_to :device_command_group, foreign_key: :device_command_group_id (or inverse_of: false)
  validates :device_command_group, presence: true
end

class DeviceCommandGroup < ActiveRecord::Base
  has_many :device_commands, dependent: :destroy
end
group  = DeviceCommand.create(name: 'Group 1')
strange_init_command  = group.device_commands.build(name: 'test', command: 'bal-bla')
strange_init_command.device_command_group_id = nil
strange_init_command.valid? = > false

This is due to the automatic inverse_or definition in the reflection library.
it is not critical. but it is important. Thanks

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

7 participants