Merge lp:~rockstar/launchpad/no-duplicate-recipe-names into lp:launchpad/db-devel

Proposed by Paul Hummer on 2010-05-26
Status: Merged
Approved by: Curtis Hovey on 2010-05-27
Approved revision: no longer in the source branch.
Merged at revision: 9408
Proposed branch: lp:~rockstar/launchpad/no-duplicate-recipe-names
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~rockstar/launchpad/remove-sourcepackage-from-recipe
Diff against target: 14 lines (+10/-0)
1 file modified
database/schema/patch-2207-59-0.sql (+10/-0)
To merge this branch: bzr merge lp:~rockstar/launchpad/no-duplicate-recipe-names
Reviewer Review Type Date Requested Status
Curtis Hovey (community) rc 2010-05-27 Approve on 2010-05-27
Björn Tillenius (community) db/code 2010-05-26 Needs Fixing on 2010-05-27
Stuart Bishop db 2010-05-26 Approve on 2010-05-27
Aaron Bentley (community) code 2010-05-26 Approve on 2010-05-26
Review via email:

Description of the change

This branch makes sure that recipe names are unique among a single person's recipes. Aaron and I had assumed that this was so, but on his suspicion, we investigated. I wrote a test to make sure his suspicion was correct, and then wrote a db patch to be applied to make sure this is correct.

There will have to be a follow-up branch for the UI in recipe add/edit that catches the IntegrityError and puts an error in the form, but I wanted to keep this branch really simple.

To post a comment you must log in.
Aaron Bentley (abentley) :
review: Approve (code)
Stuart Bishop (stub) wrote :

DB patch is good. patch-2207-59-0.sql

review: Approve (db)
Björn Tillenius (bjornt) wrote :

The db patch itself is fine, I do have some objections on the test, though.

Not sure if this is just a temporary test that you plan to replace, or not. If it is temporary, you should add an XXX saying so.

The issue is that you shouldn't rely on IntegrityError. I think the test you wrote shows why. First of all, the error doesn't happen when you call the method. It happens later, when you call store.flush(), which makes it really hard to see exactly what went wrong.

The second thing is that if you rely on IntegrityError, all that you know is that something went wrong. Your test doesn't show that it's the constraint that you added that got triggered, it could just as well be something else that went wrong. Catching IntegrityError is a bit like catching Exception. It's something you should do only if you need to make the code fail more gracefully, like logging an OOPS.

Third, if you trigger an Integrity error, the transaction gets hosed. So if you try to read something more from the db, you will get an error.

DB constraints are mainly a safety net for broken code, and ideally should never be triggered. It's better to look before you leap in this case.

On a side note, I don't understand the comment "The metadata supplied when a SourcePackageRecipe is created is present on the new object." It seems to relate neither to the test, nor to the next line.

So, +1 for the patch, -1 for the test.

review: Needs Fixing (db/code)
Curtis Hovey (sinzui) wrote :

Thanks for removing the awkward test. I am approving this in conjunction with the branch that permits the callsite to check for existance before making a mistake.

review: Approve (rc)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'database/schema/patch-2207-59-0.sql'
2--- database/schema/patch-2207-59-0.sql 1970-01-01 00:00:00 +0000
3+++ database/schema/patch-2207-59-0.sql 2010-05-28 03:51:35 +0000
4@@ -0,0 +1,10 @@
5+-- Copyright 2010 Canonical Ltd. This software is licensed under the
6+-- GNU Affero General Public License version 3 (see the file LICENSE).
8+SET client_min_messages=ERROR;
10+--- Index for owner and source package recipe name.
12+ sourcepackagerecipe__owner__name__key UNIQUE (owner, name);
14+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 59, 0);


People subscribed via source and target branches

to status/vote changes: