Database consistency issues

[Stephen Finucane]

On Thu, 2018-08-09 at 02:01 +1000, Daniel Axtens wrote:
> > 
> > I will prepare a patch that adds the foreign keys.
> 
> Further to this:
> 
> Patchwork 2.1 has that Comment has a foreign key relationship to
> submission, not patch.
> 
> Patch *inherits* from Submission. This was, in hindsight, one of the
> cases where an ORM and object-orientation do not sit well
> together. Regardless, there's a migration
> (0009_add_submission_model.py) that renames the old Patch to
> Submission. It creates a new Patch table, which has Submission as a
> foreign key.
> 
> Now Submission doesn't have a foreign key to Patch, because a
> Submission can be either a Patch or a CoverLetter or the body of a
> comment. So deleting a Patch in 2.1 won't delete the Submission, so
> it won't trigger the Comment foreign key, and you'll be left with
> dangling submissions and comments.

Oh, interesting. As you note, Submission doesn't have foreign key to
Patch; rather, it's the other way round as Patch is a "submodel" of
Submission. However, I'd always assumed [*] that there would be some
kind of uniqueness constraint here that would ensure that deleting a
Patch instance would result in the Submission instance also being
deleted. Clearly, if this ever does happen (and I really don't know if
it does. Again, this was a straight up assumption), it's something
that's enforced by Django itself, i.e. in software instead of at the DB
level.

> I'm not immediately sure how best to address this except to continue to
> unpick the general existence of Submission which is part of my long term
> plan.
> 
> [Stephen, do we allow comments on cover letters? Can we just add a
> foreign key to Comment referencing Patch?

Yup. We have a foreign key on Submission [1] which means a comment can
be mapped to a Patch, a Cover Letter, or any other kind of submission
we might add in the future (I'm seen forks on GitHub that capture non-
patch/cover letter emails too, turning Patchwork into something like a
patch-focused Pipermail).

> Could we add nullable foreign
> keys to Submission referencing Patch/CoverLetter/Comment so as to
> enforce some more referential integrity? (a la Event)]

I think we can go one better and fold CoverLetter and Patch back into
Submission. Ruby on Rails uses single table inheritance [2], whereby
everything exists in one table and you simply provide different "views"
into this. This does mean you lose some features that the DB will
provide (like the ability to make sure the diff column is non-null for
patches) but the performance improvement might ultimately be worth it.
I have a half-finished patch sitting in my local repo for a solid three
months now that I really need to finish...

> In the case of Patchwork 1.1.3, I am more confused - the Comment model
> has a foreign key to Patch here. So if you try to delete Patch you
> should have got a referential integrity error like I did with
> e.g. Events.

In case you're curious, the reason this is the case is described at
[3]. In short, the commit message for each Patch used to be stored in a
linked Comment. I got rid of that in this commit. Probably doesn't help
with the actual issue though.

> (Patchwork pre 2.0 is a pain to test as it uses Vagrant
> VMs, which use VirtualBox by default, which breaks SecureBoot, so I'm
> not going to try to get it going tonight.)

Random thought: would it make sense to backport our Docker changes to
these versions? They're not bug fixes so I wouldn't normally be
inclined to backport this stuff but they're also not user facing.

Hope this helps somewhat.

Stephen

[*] In hindsight, making assumptions where there is the very real
possibility of data loss may not have been the best idea. Noted for
future reference.
[1] https://github.com/getpatchwork/patchwork/blob/f9772a0a3/patchwork/models.py#L598
[2] https://api.rubyonrails.org/classes/ActiveRecord/Inheritance.html
[3] https://github.com/getpatchwork/patchwork/commit/ef56359fb