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

[Paul Hummer]

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.

[Aaron Bentley (abentley)]

review: Approve (code)

[Stuart Bishop (stub)]

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

review: Approve (db)

[Björn Tillenius (bjornt)]

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)]

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)

=== added file 'database/schema/patch-2207-59-0.sql'
--- database/schema/patch-2207-59-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-59-0.sql	2010-05-28 03:51:35 +0000
@@ -0,0 +1,10 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+--- Index for owner and source package recipe name.
+ALTER TABLE SourcePackageRecipe ADD CONSTRAINT
+    sourcepackagerecipe__owner__name__key UNIQUE (owner, name);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 59, 0);