Details
- Reviewers
amckinley - Maniphest Tasks
- T920: Provide SMS Support
- Commits
- rP596435b35e30: Support designating a contact number as "primary"
- Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Do we have other examples of this kind of "exactly one row must have columnFoo set to bar" logic elsewhere? Off the top of my head I can think of primary email addresses. Wondering if it's worth factoring out into the data layer instead of mangling it like we do here.
src/applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php | ||
---|---|---|
36–43 | Maybe just don't show the button? | |
73 | "Designate" |
src/applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php | ||
---|---|---|
36–43 | (I think there's significant value in having the UI remain consistent, even though remotely reasonable users probably won't end up here.) |
Do we have other examples of this kind of "exactly one row must have columnFoo set to bar" logic elsewhere?
Sort of. Email addresses have a similar "primary" flag, although it's simpler because they can't currently be disabled. The logic there is a little more direct (in PhabriactorUserEditor->changePrimaryEmail()): load the old primary mail, un-make it primary, save it, make the new one primary, save it.
I think that version of things isn't too bad, although it doesn't converge to the right state under an arbitrary user running UpDaTe ... queries and marking multiple rows as isPrimary. It also relies on a heavier transactional lock with a longer lock window, although this is probably not a real concern.
Neither version prevents the database from being put in an invalid state with UpDaTe .... In the email case (where there's no "disabled" state), we can make that guarantee like this:
- Make isPrimary nullable.
- Store NULL to mean "not primary".
- Pick a particular value, probably "1", to mean "primary".
- Add a UNIQUE KEY (userPHID, isPrimary).
However, I generally think this approach is too messy to really embrace. In particular:
- To understand it, you have to know that when a row is bound by a unique key but has a NULL value for one of the columns named in the key, it is exempted from uniqueness constraints. I generally assume that the "typical" engineer knows a bit about keys but may not understand multipart keys in much detail, much less be comfortable with UNIQUE + NULL tricks.
- It means that isPrimary = 0 and isPrimary = NULL are two different possible states that should have the same meaning (and only one row is allowed to have isPrimary = 0).
- Code like setIsPrimary(0) will work if you test it, then fail if you go through the same flow again. Surprise!
- The correct code becomes setIsPrimary(NULL) or there needs to be a translation layer.
- We're out of luck anyway if we add a rule like "and disabled things can't be primary", as here, and we have to accept that ambitious users may UpDaTe ... us into an invalid state.
In this case, I think that the better long-term way to represent this might be something like a different table with <userPHID, contactNumberRole, contactNumberPHID> so you can have a "Primary MFA Number" and "Primary Pager Number" as separate entries (and they can be the same number or different numbers, and third party applications can define new roles without schema changes...) but I don't want to build out too much of that stuff so far ahead of the game.
In general this pattern is fairly rare. I'm kind of experimenting with this approach here, but all the ways I know of to handle this feel like they have roughly-similar tradeoffs in different directions.