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 _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
