Unable to easily delete has_many without iterating and deleting each

[robacarp]

This pattern isn't ideal:

class User < Granite::ORM::Base
  has_many :posts

  before_destroy :clean_up_posts

  def clean_up_posts
    posts.map &.destroy
  end
end

class Post < Granite::ORM::Base
  belongs_to :user
end

Because it causes N delete queries to be executed. It would be nice to have a way to destroy an entire collection in one go. So I tried some SQL:

  def clean_up_posts
    query = <<-SQL
    DELETE FROM posts WHERE user_id = ?
    SQL

    Post.exec query, [id]
  end

This may be a completely unrelated issue, but the exec and scalar query methods aren't parameterized on the models, so:

wrong number of arguments for 'Post.exec' (given 2, expected 0..1)
Overloads are:
 - Granite::ORM::Querying#exec(clause = "", &block)

As a result, I ended up with this:

  def clean_up_posts
    query = <<-SQL
    DELETE FROM posts WHERE user_id = #{id}
    SQL

    Post.exec query
  end

I don't think this is immediately going to result in a compromisable injection vulnerability...in my case. But it's probably worth adding parameterized exec and scalar methods to the base.

[shayneoneill]

Wouldnt the better way to handle this be to ensure DB foreign key Constraints are properly set to cascade? Keep in mind cascade also has to deal with update contingencies too. This is something modern RDBMS handle well already. Why duplicate functionality?

[robacarp]

@shayneoneill leveraging RDBMS constraints could be a great way to handle this

[drujensen]

@shayneoneill @robacarp I think adding db constraints in the migration scripts would be perfect. I always hated rails for not properly supporting RDBMS constraints. We should look into this solution further.

Maybe we can add this as a feature of #182 WDYT?

[Adam-Stomski]

I'm pretty sure it should be easy to implement here: https://github.com/amberframework/granite/blob/master/src/granite/association_collection.cr
something like:

user.posts.delete_all

[drujensen]

@Adam-Stomski yeah, we can add a delete_all here fairly easily. We can also support cascade deleting children rows on a has_many relationship.

I guess the question is if we should take advantage of the RDBMS to handle the relationship integrity. For example, you can specify a cascading delete in the relationship in the database:

CREATE TABLE rooms (
    room_no INT PRIMARY KEY AUTO_INCREMENT,
    room_name VARCHAR(255) NOT NULL,
    building_no INT NOT NULL,
    FOREIGN KEY (building_no)
        REFERENCES buildings (building_no)
        ON DELETE CASCADE
);

The nice thing about this is that your data will never get out of sync (unlike Rails) where someone might access the database directly and delete a row without deleting the children creating orphans.

But, How can we do this without controlling the migration scripts?

[robacarp]

Rails did eventually add support for most database level constraints, but it was vacant for far too long.

But, How can we do this without controlling the migration scripts?

The answer is, I don't think we do. If someone wants to add that to the database, they'll have to add that manually. Adding #delete_all to the association_collection is a great next step though. It'll be needed even if granite someday gets control over db constraints.

[Adam-Stomski]

I guess the question then is, should granite follow ActiveRecord and add two methods:

1. destroy_all - just runs destroy on each record in loop, runs callbacks
2. delete_all - deletes all records in a single query, no callbacks

wdyt? I will have some time this week, can add it

[shayneoneill]

@robacarp if memory serves me right, David Hansson use to harbor some really bad ideas about database consistency, feeling that constraints belonged in the code not the database. Something I personally consider reckless. Constraints should be opt-out, not opt-in. It can't be assumed the coder understands the implications of orphaning records well enough to make the right decisions around deploying transactions and sanity checking to avoid corruption. Django does (and always has done) the right thing here, creating constraints and appropriate rules at the SQL level to make sure programmers won't shoot themselves in the foot. Unless, of course, they chose not and disable it. And occasionally there are use cases for that too (Ie existing database, or strange schemas)