Last Comment Bug 1112628 - disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them
: disallow deletion of releases that have a rule.mapping or rule.whitelist poin...
Status: RESOLVED FIXED
[lang=python][good first bug]
:
Product: Release Engineering
Classification: Other
Component: Balrog: Backend (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Ninad Bhat[:ninad101]
: Ben Hearsum (:bhearsum)
:
Mentors: Ben Hearsum (:bhearsum)
Depends on: 1304054
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-17 08:35 PST by Ben Hearsum (:bhearsum)
Modified: 2016-09-21 13:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
0001-bug-1140262-disallow-deletion-of-releases-that-have-.patch (5.21 KB, patch)
2016-02-26 11:05 PST, Kumar Rishabh
bhearsum: feedback+
Details | Diff | Splinter Review

Description User image Ben Hearsum (:bhearsum) 2014-12-17 08:35:26 PST
Right now it's possible to delete releases that still have rules pointing to them. This is bad because it leaves the rules in a very weird state. One could easily remove a release that is active without knowing it because the UI doesn't even give any sort of warning. We should probably disallow this at the database schema level with a foreign key. IIRC this wasn't implemented originally because sqlite doesn't support them. I'm sure we can find a way to work around this.
Comment 1 User image Kumar Rishabh 2016-02-26 11:05:28 PST
Created attachment 8724159 [details] [diff] [review]
0001-bug-1140262-disallow-deletion-of-releases-that-have-.patch

Not too sure if it is desired this way. But still would like to get some review.
Comment 2 User image Ben Hearsum (:bhearsum) 2016-02-26 11:56:54 PST
Comment on attachment 8724159 [details] [diff] [review]
0001-bug-1140262-disallow-deletion-of-releases-that-have-.patch

Review of attachment 8724159 [details] [diff] [review]:
-----------------------------------------------------------------

This is a very good start!

It looks like sqlite actually supports foreign keys as of 3.6.19 (which is ~6 years old). I'd prefer database level enforcement of this because it minimizes the possibility of a bug in our code, could you give that a shot?

A couple other things in line:

::: auslib/admin/views/releases.py
@@ +276,4 @@
>          return changeRelease(release, changed_by, transaction, exists, commit, self.log)
>  
>      @requirelogin
> +    def _check_rule_count(self, name, changed_by):

The web layer isn't the right place to do this, even if you end up implementing it yourself instead of using a foreign key. If the foreign key doesn't work out, this should go in https://github.com/mozilla/balrog/blob/master/auslib/db.py#L1089. Either way, you should add tests both at the web layer (which you're doing) and the database layer (like https://github.com/mozilla/balrog/blob/master/auslib/test/test_db.py#L1020)

I think the queries below aren't quite right either, but let's see if the foreign key works out before worrying about that.

::: auslib/test/admin/views/test_releases.py
@@ +132,5 @@
>          ret = self._delete("/releases/a", username="bob")
>          self.assertStatusCode(ret, 401)
>  
> +    def testDeleteWithRulesExisting(self):
> +        # Test the _delete method first mocking check_rule_count helper

I don't think you should be doing any mocking in these tests, regardless of whether this gets implemented as a foreign key or manually. There's no external services we're depending on either (the database is in-memory sqlite, so it's fine to depend on that).

I think you're also testing multiple cases in one test...they should be separated out into individual tests. I see at least:
testDeleteWithRuleMapping
testDeleteWithRuleWhitelist

..but there might be other cases to cover as well.
Comment 3 User image Ben Hearsum (:bhearsum) 2016-06-10 08:27:01 PDT
Kumar, are you still interested in working on this?
Comment 4 User image Ben Hearsum (:bhearsum) 2016-06-21 07:51:08 PDT
(In reply to Ben Hearsum (:bhearsum) from comment #3)
> Kumar, are you still interested in working on this?

Looks like not. Feel free to reassign yourself if you like, though.
Comment 5 User image [github robot] 2016-09-19 11:48:50 PDT
Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/38830e39e9c7d28c280b85b78ea66df6ef1bae66
Bug 1112628: disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them (#126). r=bhearsum
Comment 6 User image Ben Hearsum (:bhearsum) 2016-09-21 13:11:40 PDT
This is in production now, thanks Ninad!

Note You need to log in before you can comment on or make changes to this bug.