Counterintuitive priority setting via maniphest.edit conduit call
Open, Needs TriagePublic

Description

Hi,
To set a correct priority via maniphest.edit call you need to set priority transaction value as a string. However it is not a "normal"/"wishlist" type of string, but a priority value in DB. So successful call is like this:

{"type":"priority","value":"50"}

This fails (value has to be a string):

{"type":"priority","value":50}

Call is successful but priority is set to "wishlist" (0):

{"type":"priority","value":"normal"}
amckinley added a subscriber: amckinley.

It looks like what we really need is a new method like maniphest.querystatuses, called maniphest.querypriorities to avoid anyone trying to hardcode these values.

I'd like to fix the counter-intuitive behavior where {"type":"priority","value":50} fails; can we just change the type of priority to be an integer to get a little closer to reasonable behavior?

Ideally, we should maybe freeze maniphest.querystatuses (which has a sort of ridiculous output format, I think?) and provide maniphest.status.search and maniphest.priority.search, which return results similar to other modern *.search methods even if they aren't implemented quite the same way (at least make the output format similar enough that a client library with methods like "turn a x.search call into a list of objects" would work on these methods, too).

"Priority" currently has a string type because it's edited by a PhabricatorSelectEditField in ManiphestEditEngine, and "select" fields are always string-typed.

In theory, you could add some method like setYouCanUseIntegersForThis(true) on PhabricatorSelectEditField, but that feels a little fishy to me.

Maybe:

  • Add a required, unique string "key" to maniphest.priorities for each priority.
  • Freeze maniphest.querystatuses.
  • Provide maniphest.status.search.
  • Provide maniphest.priority.search.
  • Swap the options the EditField uses from integers to the new strings you introduced.
  • In the PriorityTransaction, convert the string (like "high") to an integer so the internal/storage/display format is the same.
  • Undo the mapping in frozen method maniphest.update.

Then "50" and 50 will both break against maniphest.edit, but we'll otherwise be in a fully sane world of "high", "low", etc.

We also probably have similar issues in maniphest.search with the "priority" constraint. Probably just break it at the same time? Not sure how bad off we are there.

Transitionally, we could continue to accept "50" for a while and then schedule a break in a few months.

The small bit of good news is that I managed to forsee this well enough that the output from maniphest.search for "priority" is a dictionary, so that would be easy to extend with a "key".

I guess we already have "keywords" and should probably accept any of them, too, so "!priority high" in mail, /priority high in a comment, and "high" in Conduit all work consistently even if the actual key is "elevated".

The config also looks like this right now:

{
  123: { ... }
  124: { ... }
}

That is, it's a dictionary. Every other similar config looks like this:

[
  {thing: 123},
  {thing: 124},
]

That is, it's a list.

I'm not sure if this worth breaking alongside other changes in order to improve consistency or not.

The major reason we do lists for everything else is that the keys in JSON maps are not necessarily ordered, and other languages don't always expose an ordered primitive when you decode an object, but sometimes we rely on order (for example, the list of statuses cares about the order you enter it in). This doesn't matter for priorities since we sort by the priority numbers. However, it's an exception in an otherwise consistent field, at least among all the stuff we've added somewhat recently.

That is, the specific error "use lists" is aimed at preventing is this:

  • You specify all your config in YAML (very cool) and deploy it with Chef (very cool).
  • One day (perhaps today, perhaps in the future) Chef (very cool) decides that since JSON objects are not ordered, it can read the object from YAML, freely reorder it, and write it in whatever order it prefers, perhaps "for performance".
  • After you deploy, your statuses now have some arbitrary order like "Closed, Invalid, Open, Unfixable, Reopened, Impossible, Double Verified, Haunted, Single Verified" instead of the nice logical order they previously had.

So should the return value for priority.search look something like this?

[
  { "value": 80,
    "name": "High",
    "short": "High",
    "color": "red",
    "keywords": [
      "high"
    ]},
  { "value": 50,
    "name": "Normal",
    "short": "Normal",
    "color": "orange",
    "keywords": [
      "normal"
    ]},
    // etc etc
]

Yeah, with whatever wrapping context maniphest.search has, so like:

{
   "data": [ (that list) ]
}

...or whatever.

Actual example output:

{
  "data": [
    {
      "name": "Unbreak Now!",
      "short": "Unbreak!",
      "color": "pink",
      "keywords": [
        "unbreak"
      ],
      "value": 100
    },
    {
      "name": "Needs Triage",
      "short": "Triage",
      "color": "violet",
      "keywords": [
        "triage"
      ],
      "value": 90
    },
    {
      "name": "High",
      "short": "High",
      "color": "red",
      "keywords": [
        "high"
      ],
      "value": 80
    },
    {
      "name": "Normal",
      "short": "Normal",
      "color": "orange",
      "keywords": [
        "normal"
      ],
      "value": 50
    },
    {
      "name": "Low",
      "short": "Low",
      "color": "yellow",
      "keywords": [
        "low"
      ],
      "value": 25
    },
    {
      "name": "Wishlist",
      "short": "Wish",
      "color": "sky",
      "keywords": [
        "wish",
        "wishlist"
      ],
      "value": 0
    }
  ]
}

That looks great to me.