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

Required belongs_to associations aren't being validated #665

Open
sobrinho opened this issue Feb 4, 2020 · 5 comments
Open

Required belongs_to associations aren't being validated #665

sobrinho opened this issue Feb 4, 2020 · 5 comments
Labels

Comments

@sobrinho
Copy link

sobrinho commented Feb 4, 2020

Contrived example:

class Category < ActiveRecord::Base
end

class Post < ActiveRecord::Base
  belongs_to :category, optional: false
end

Post.create!(category_id: nil)
#=> ActiveRecord::RecordInvalid: Validation failed: Category must exist

Post.create!(category_id: -1)
#=> ActiveRecord::RecordInvalid: Validation failed: Category must exist

Post.import [Post.new(category_id: nil], validate: true, raise: true
#=> Validation failed: Category must exist (ActiveRecord::RecordInvalid)

Post.import [Post.new(category_id: -1], validate: true, raise: true
#=> Imports the post without a category

I'm workarounding this with a safety check:

def import_posts(posts)
  category_ids = Category.pluck(:id)
  total = posts.count
  missing_category = posts.count { |post| category_ids.include? post[:category_id] }

  if missing_category > 0
    raise ArgumentError, "#{missing_category} posts missing category out of #{total}"
  end

  Post.import posts, validate: true, raise: true
end

Not sure if the issue is by design or even if we should deal with that but Rails does, it does check if the record in fact exists in the foreign table.

We might be concerned about retrieving all foreign records due to memory usage?

Maybe we can do it in another way for the activerecord-import like this (pseudo-code):

def import_posts(posts)
  category_ids = posts.map { |post| post[:category_id] }.compact.uniq
  found_category_ids = Category.where(id: category_ids).count

  if category_ids.length != found_category_ids
    raise ActiveRecord::RecordInvalid, "#{posts.length - found_category_ids} posts missing category out of #{posts.length}"
  end

  Post.import posts, validate: true, raise: true
end
@jkowens
Copy link
Collaborator

jkowens commented Feb 6, 2020

You could possibly add a validation to check the presence of category_id on Post, but the reason we purposely don't do the validation of belongs_to associations is because it executes a database query. Running a query for each record just seems to contradict the purpose of this library 😁

@sobrinho
Copy link
Author

sobrinho commented Feb 6, 2020

@jkowens but we can do one single query to query all the records like my second example.

It basically counts how many categories there are in all posts and makes one query to count to check how many of them really exists in the database.

@jkowens
Copy link
Collaborator

jkowens commented Feb 6, 2020

Now that I'm thinking about it, I seem to recall that AR-Import does try to automatically validate the presence of the foreign key of the belongs_to association (ie category_id).

I would expect this to raise an error:

Post.import [Post.new(category_id: nil], validate: true, raise_error: true

Can you confirm that?

As regards your example code, and the details about not validating the presence of belong_to associations in the database, I think this would be a good addition to the readme.

@sobrinho
Copy link
Author

sobrinho commented Feb 6, 2020

Sorry, misreport on that one, using nil it does validate but using inexistent ids doesn't.

@sobrinho
Copy link
Author

sobrinho commented Feb 6, 2020

Fixed in original report.

@jkowens jkowens added the docs label Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants