settings: catch IntegrityError and render error in output (Bug 2037991)

[cgsheeh]

Catch the IntegrityError thrown when trying to add a Phab API
key to a profile where there is already an existing PHID associated
with a user profile.

We add a test for this case, and a test for the happy patch since
it is currently untested. Move the phabdouble fixture into the
top-level conftest.py so it can be accessed from the ui app.

[github-actions]

View this pull request in Lando to land it once approved.

[zzzeid]

Can we not check for existing PHIDs here, avoiding the integrity error altogether?

[cgsheeh]

We could, but it would add another DB query on every API key update, and we'd still want to keep the IntegrityError handler to cover the potential race condition (unlikely but technically possible). Happy to change it, this just follows EAFP and is more efficient for the happy path. WDYT?

[zzzeid]

This endpoint is not used frequently, the extra DB query would be negligible.

The code flow would be more logical if we're explicit about detecting the error state, rather than working around the issue. The integrity error could also (in theory, though very unlikely) occur in a different situation, so here we'd be handling the expected error case that we already discovered.

[cgsheeh]

I added a check for a duplicate PHID. Personally I don't think it's a meaningful improvement - we're still catching the IntegrityError to catch the race condition, so it's more code for the same functionality - but happy to move forward with it.

[zzzeid]

Hmm what's the point of catching the IntegrityError? We don't need to do that as long as we know there is no duplicate value. Otherwise it would be a misleading error since if triggered, it is some other issue that is causing that and not the PHID.

I would suggest removing the try/except altogether.

[cgsheeh]

Hmm what's the point of catching the IntegrityError? We don't need to do that as long as we know there is no duplicate value. Otherwise it would be a misleading error since if triggered, it is some other issue that is causing that and not the PHID.

The IntegrityError handler covers the race between our .exists() and .save() — without it, two concurrent requests linking the same PHID would 500 instead of returning a clean 400. And since phabricator_phid is the only unique field we touch here, an IntegrityError can't really come from anywhere else.

Yes, the race is unlikely — but the bug is specifically about an unhandled server error, and switching to just an exists() check doesn't actually resolve that; it just makes the window smaller.

This is why I preferred just the try/except version — it was less code, handled both cases, and required fewer DB lookups on the happy path.

[zzzeid]

To handle the (very unlikely) race condition we could do the check and the save with a lock, but I think that's overkill. This endpoint is infrequently used, I think the basic issue is to present a meaningful error to the user and avoid having a server issue.

I won't split more hairs here but I think preventing an integrity error is better than handling it when it happens.

[zzzeid]

TL;DR this should be handled as a validation problem, and not a database integrity problem.

[zzzeid]

I won't block this PR on the outstanding details of the implementation, particularly since this endpoint is a legacy one, and requires a revamp/renovation anyway (it should go in lando.api, and should use class-based views, and have overall better validation.)

[zzzeid]

TL;DR this should be handled as a validation problem, and not a database integrity problem.