Launchpad itself

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

no-duplicate-recipe-names
Merge into 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://staging/~rockstar/launchpad/no-duplicate-recipe-names

Merge into:

lp://staging/launchpad/db-devel

Prerequisite:

lp://staging/~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://staging/~rockstar/launchpad/no-duplicate-recipe-names

High

Fix Released

Link a bug report

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 (community)	db	2010-05-26	Approve on 2010-05-27
Aaron Bentley (community)	code	2010-05-26	Approve on 2010-05-26
Review via email: mp+26080@code.staging.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.

Revision history for this message

Aaron Bentley (abentley) on 2010-05-26:

review: Approve (code)

Revision history for this message

Stuart Bishop (stub) wrote on 2010-05-27:

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

review: Approve (db)

Revision history for this message

Björn Tillenius (bjornt) wrote on 2010-05-27:

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 on 2010-05-27:

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)