Launchpad itself

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

  1. no-duplicate-recipe-names
  2. Merge into db-devel
Proposed by Paul Hummer
Status: Merged
Approved by: Curtis Hovey
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 Approve
Björn Tillenius (community) db/code Needs Fixing
Stuart Bishop (community) db Approve
Aaron Bentley (community) code Approve
Review via email: mp+26080@code.launchpad.net

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.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve (code)
Revision history for this message
Stuart Bishop (stub) wrote :

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

review: Approve (db)
Revision history for this message
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)
Revision history for this message
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).
7+
8+SET client_min_messages=ERROR;
9+
10+--- Index for owner and source package recipe name.
11+ALTER TABLE SourcePackageRecipe ADD CONSTRAINT
12+ sourcepackagerecipe__owner__name__key UNIQUE (owner, name);
13+
14+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 59, 0);

Subscribers

People subscribed via source and target branches

to status/vote changes: