draftsman icon indicating copy to clipboard operation
draftsman copied to clipboard

draft_destroying dependencies doesn't take into account draft association changes

Open npafundi opened this issue 9 years ago • 5 comments

Hey Chris, this is a weird one, and might be worth a conversation.

Let's say you have Book and Shelf models. A Shelf has_many books, and a Book can only be on one Shelf at any given time.

Further, assume some Book is on Shelf 1, and you decide you want to move it to Shelf 2, and this is done via draftsman. Your Book will have an association of Shelf 1 (its old location), and the Book's draft will have an association of Shelf 2 (its new location).

If you draft_destroy Shelf 1, the Book should still exist because its draft is on Shelf 2. However, currently draft_destroy handles destroying all dependencies. The issue is that it uses all non-drafted dependencies, regardless of whether the association has changed.

In the case above, this would mean that the Book would be draft_destroyed when Shelf 1 was draft destroyed, even though the Book('s draft) is on Shelf 2.

I tried to simplify the example above, but this was somewhat confusing to write up. If I can clarify anything, please let me know. If you'd like to communicate another way (email/skype/phone), we would be more than happy to talk about it, since there doesn't seem to be an obvious solution.

Thanks Chris!

npafundi avatar Jul 22 '15 23:07 npafundi

Your description of the problem makes sense after thinking about it for a minute. Sounds like you guys are really pounding on this library, which is awesome.

I'm about to start using Draftsman more on my project and will look into this issue soon. Of course, I welcome any ideas on how to fix.

chrisdpeters avatar Jul 29 '15 02:07 chrisdpeters

Hey, sorry to leave this open so long. We're definitely putting draftsman through its paces :)

There seem to be a few potential solutions, but they all have some downside. I'll throw everything out there, but I think a couple of these would be pretty bad solutions (and I'll try to explain why). I'm hoping that these can be a jumping off point for discussion, and hopefully we can find something that works better.

1. Don't draft_destroy dependencies

  • This is the simplest solution -- when draft_destroying an object, don't draft_destroy its dependencies.
  • Unfortunately, this would require manually managing destruction of dependencies from the application.
  • Probably not a great solution :-1:

2. Leave draftsman alone; have the application copy objects rather than moving associations.

  • Potential for lots of duplication in the database, and (perhaps) a lot of work to get this working (for example, this would require copying every object, all associations, and so on.)
  • Would require a draft_destroy on the old object, and a draft_creation of the new one, all handled manually by the user application
  • Revert becomes much more challenging (tracking these changes)
  • Probably not a great solution :-1:

3. Reify drafted dependencies, and only draft_destroy them if they're still associated with the current object

  • This will lead to orphaned objects in the database. Using the example from the original post, let's assume the Book is moved to Shelf 2 (unpublished). Further assume that we implement this change, and only draft_destroy dependencies if they're still associated with this object. This would mean that if Shelf 1 is destroyed, we won't destroy the Book (good!). But if we draft_destroy Shelf 2, we also won't destroy the Book (bad!), because we're essentially using the shelf.books association, which only looks at the live data (normal ActiveRecord functionality). And, to make matters worse, I don't know a good way to find, say, shelf.drafted_books, because that would require reifying (or deserializing, or parsing) every draft object in the draft table, which could be very inefficient. [1]
  • If we're not worried about orphans, or if we can find a way to clean them up, the other problem is that this solution could slow things down if there are a lot of dependencies that need to be checked.
  • Still probably not a great solution, but perhaps better than the previous two?

[1] It is of course technically possible to look through every object in the drafts table to determine their current associations. For example, we could look directly at the serialized object of every Book, and create a list of all Books with a given association. Typically, this would be very inefficient -- especially in situations where draft_destroys are common and/or the draft table is large. However, this could be somewhat mitigated if the draft object column was jsonb, which may only be supported by PostgreSQL(?). It allows much faster indexing and searching of json data. However, this would leave people with other databases out in the cold.


I wrote all this out to help remember the possibilities, but I'm not really sold on any of these. I'd love to hear your thoughts and any alternatives.

As always, if you'd like clarification on any of this, please let me know. Thanks for your help!

npafundi avatar Aug 21 '15 21:08 npafundi

Thanks for the thoughtful writeup.

Initially, I'm wondering if your 3rd scenario would make sense, with a twist. If the database is Postgres, then use its faster indexes. If it's something else, then do the inefficient cleanup work using reify.

Perhaps let devs opt out of this logic via a global setting and/or model-level option and write their own management logic using background processing or whatever they want.

Definitely need to sleep on this one.

chrisdpeters avatar Aug 21 '15 21:08 chrisdpeters

Hi Nick

I am the new maintainer of Draftsman. I have been thinking about this issue, and have not been able to come to a conclusion. I am not sold on neither of the options yet.

I was wondering what your experience has been with this, and if you are now more inclined towards one of the options.

There is one other approach which is implementing a sort of confirmation, where in order for a "destroy" to be successful:

  • the item must have no associations with drafts, or
  • destroy passing an extra "force_drafts_destroy" parameter (or something like that)

If the item being destroyed has an association with an active Draft, then the destroy is aborted, and we leave it to the developer of each app to decide how to proceed.

I think Draftsman should not force any business behavior, since they are all too specific. But ideas and feedback are welcome.

jmfederico avatar Jan 30 '18 13:01 jmfederico

Hey @jmfederico, thanks for looping back around on this. I actually haven't been using Draftsman for quite some time; I moved on from the job where we put it in place. I recall that neither of the first two solutions above were very good (as described), which left the third (mediocre) idea. However, your ideas may be more reasonable.

Since I'm not really a stakeholder in it any more, I would say do whatever works best for your applications. I think that as long as there is a way to address this, consumers of Draftsman will be able to work around it.

Sorry for not having a more specific answer, and thanks for maintaining the gem!

npafundi avatar Mar 23 '18 19:03 npafundi