Dependent Destroy

[victornamuso]

I noticed there is an issue when destroying the parent record which throws a foreign key violation on the transition record. I got around it with a before_destroy. Would be nice to have one of the following:

• Make it the default behavior
• Add the ability to change the default behavior for an application with an initializer
• Add the ability to change the default behavior at the class level

OR

• Keep the default behavior as the same
• Add the ability to change the default behavior for an application with an initializer
• Add the ability to change the default behavior at the class level

Would like to hear your thoughts on this. If you'd like this done, I'd be happy to code it and issue a PR.

[rosskevin]

I think destroy should be the default behavior, this logging should not survive without the parent entity.

[victornamuso]

Do you think there should ever be a reason to turn it off at the class level or via config/initializer?

[smudge]

Definitely a breaking change. I've relied on orphaned transition records for deriving rough analytics / business intelligence (being able to see what happened historically even if you don't have the current parent record anymore). Not saying the non-destroy behavior has to be the default, but for my use case I would have been annoyed if they started being deleted along with the parent record.

[victornamuso]

@smudge Good point. What code are you using to avoid the foreign key violation?

[smudge]

@victornamuso In my case I just removed the foreign key constraint (in postgres) and used a vanilla index instead. (Because I wanted to preserve the deleted record's ID.) But you could also use dependent: :nullify to have Rails explicitly set the FK value to nil. Requires a nullable FK of course.

[victornamuso]

@smudge it seems like it would be cleaner to change the default implementation to be all inclusive so nothing out of the box would be needed post generator. Whether that is redefining the default migration, doing dependent: :nullify or dependent: :destroy, then make it configurable at the app and class level. Seems like handling it the way you do it would be the best, as it has the least amount of destruction. There's always just leaving it the way it is and having the generator read the configuration to determine the behavior as this would ensure maximum backward compatibility.

[rosskevin]

I'm not of the mindset that backward compatibility makes sense here. Having an FK of null limits the value of seeing what happened in the past. While there is still a log, tying together the actions against a specific parent instance would be quite difficult or even impossible.

I think dependent: destroy is a good default with the potential to change it for those that have some value with an abandoned log.

We can increment the version appropriately and list it as a breaking change. @victornamuso are you using this with Rails 5?

[smudge]

Yeah I don't want to prescribe a default -- to support my use case I think it would be fine to make the breaking change, increment the version accordingly, and provide a way to change the default behavior to allow for orphaned log entries.

[victornamuso]

@rosskevin Not yet, but I'll be making that move over early next year.

[krtschmr]

+dependent destroy. super important. or make it as an option, so we can select our own thing.

dependent: :destroy
dependent: false (default?)

[SirRawlins]

Hey folks, did we ever come to an agreement on this? Found myself bumping into it today. I'm currently redefining the has_many association myself in the model to add dependent destroy.

[victornamuso]

@SirRawlins Read back through it, and I don't think that we did. Any input on how we want to do this?

[rosskevin]

I reached agreement with myself on what should be the plan! Absent any dissent, this is the plan:

I'm not of the mindset that backward compatibility makes sense here.

and

I think dependent: destroy is a good default with the potential to change it for those that have some value with an abandoned log.

and

We can increment the version appropriately and list it as a breaking change

[victornamuso]

Thanks @rosskevin. Unless someone else wants it, I can get around to putting in a PR sometime this month.

[rosskevin]

Thanks @victornamuso

[btrd]

Is there any news on this issue ?

I remove the foreign_key: true in the migration to quick fix the issue, but I don't think it's the right solution.

[victornamuso]

@btrd, totally slipped off my radar sorry! I put a reminder on my calendar to work on this next week.

[btrd]

Awesone, thanks !

I can try to make a PR if you don't have the time to work on it.