Page MenuHomePhorge
Create Task
Maniphest T15631

"Map returned omits required key" exception for second result page (cursor) for Maniphest search ordered by custom field
Closed, ResolvedPublic
Actions

Description

Steps to reproduce:

  1. Phorge master at `cef12d8dc202decc7f4088cb3402f73d74c452f3, PHP 8.2.10
  2. Go to http://phorge.localhost/config/edit/maniphest.custom-field-definitions/
  3. Set the empty Database Value to
{
  "deadline.due": {
    "name": "Due Date",
    "view": true,
    "edit": true,
    "description": "Deadline for completing the task.",
    "search": true,
    "fulltext": true,
    "type": "date",
    "copy": false
  }
}
  1. Select Save Config Entry
  2. Go to http://phorge.localhost/maniphest/query/advanced/ and select search criteria which will result in more than 100 results
  3. Set Order By to Due Date (reversed)
  4. Click the Search button
  5. On the results page, scroll down and click the Next button (which results in an URL like http://phorge.localhost/maniphest/query/JA6ufSF1vjSX/?after=220 )

Expected outcome:

Results.

Actual outcome:

As most or all results will have no Due Date set, an exception is shown instead:

[2023-09-02 14:05:35] EXCEPTION: (Exception) Map returned by "newPagingMapFromCursorObject()" in class "ManiphestTaskQuery" omits required key "custom.deadline.due". at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:229]
arcanist(head=customOAuthUrlencodeNull, ref.master=df6c315ace5f, ref.customOAuthUrlencodeNull=c69b9749027f), phorge(head=master, ref.master=cef12d8dc202)
  #0 <#2> PhabricatorCursorPagedPolicyAwareQuery::getPagingMapFromCursorObject(PhabricatorQueryCursor, array) called at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:660]
  #1 <#2> PhabricatorCursorPagedPolicyAwareQuery::buildPagingClause(AphrontMySQLiDatabaseConnection) called at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:593]
  #2 <#2> PhabricatorCursorPagedPolicyAwareQuery::buildPagingWhereClause(AphrontMySQLiDatabaseConnection) called at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:515]
  #3 <#2> PhabricatorCursorPagedPolicyAwareQuery::buildWhereClauseParts(AphrontMySQLiDatabaseConnection) called at [<phorge>/src/applications/maniphest/query/ManiphestTaskQuery.php:355]
  #4 <#2> ManiphestTaskQuery::buildWhereClauseParts(AphrontMySQLiDatabaseConnection) called at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:505]
  #5 <#2> PhabricatorCursorPagedPolicyAwareQuery::buildWhereClause(AphrontMySQLiDatabaseConnection) called at [<phorge>/src/applications/maniphest/query/ManiphestTaskQuery.php:244]
  #6 <#2> ManiphestTaskQuery::loadPage() called at [<phorge>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:251]
  #7 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phorge>/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:400]
  #8 <#2> PhabricatorCursorPagedPolicyAwareQuery::executeWithCursorPager(AphrontCursorPagerView) called at [<phorge>/src/applications/search/engine/PhabricatorApplicationSearchEngine.php:1038]
  #9 <#2> PhabricatorApplicationSearchEngine::executeQuery(ManiphestTaskQuery, AphrontCursorPagerView) called at [<phorge>/src/applications/search/controller/PhabricatorApplicationSearchController.php:256]
  #10 <#2> PhabricatorApplicationSearchController::processSearchRequest() called at [<phorge>/src/applications/search/controller/PhabricatorApplicationSearchController.php:91]
  #11 <#2> PhabricatorApplicationSearchController::processRequest() called at [<phorge>/src/aphront/AphrontController.php:29]
  #12 <#2> AphrontController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/AphrontController.php:71]
  #13 <#2> AphrontController::delegateToController(PhabricatorApplicationSearchController) called at [<phorge>/src/applications/maniphest/controller/ManiphestTaskListController.php:20]
  #14 <#2> ManiphestTaskListController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #15 phlog(Exception) called at [<phorge>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
  #16 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, Exception) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
  #17 AphrontApplicationConfiguration::handleThrowable(Exception) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:296]
  #18 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #19 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Other comments:

Upstreaming from https://phabricator.wikimedia.org/T344241

Revisions and Commits

rP Phorge
D25504rPd42b3eb0b17a Fix "Map omits required key" exception by comparing result order against modern…

Related Objects

Event Timeline

aklapper created this task.Sep 2 2023, 14:11
Herald added subscribers: Cigaryno, Matthew, chris, tobiaswiese. · View Herald TranscriptSep 2 2023, 14:11
aklapper added a comment.Sep 2 2023, 17:09

<tl;dr>: This is a mismatch between legacy field keys still used for paging, versus modern field keys.

https://we.phorge.it/source/phorge/browse/master/src/applications/maniphest/query/ManiphestTaskQuery.php$965 does

foreach ($keys as $key) {
  if ($this->isCustomFieldOrderKey($key)) {
    $map += $this->getPagingValueMapForCustomFields($task);

The two values in the passed array $keys are custom.deadline.due and id.

In https://we.phorge.it/source/phorge/browse/master/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php$1757, isCustomFieldOrderKey($key) checks for

$prefix = 'custom:';
return !strncmp($key, $prefix, strlen($prefix));

But getPagingValueMapForCustomFields($task) still returns outdated custom:std:maniphest:deadline.due while the key is custom.deadline.due.
Due to dot versus colon in that mismatch, $map += $this->getPagingValueMapForCustomFields($task); is not triggered here anyway.

getPagingValueMapForCustomFields() function has the following code:

foreach ($fields->getFields() as $field) {
  $map['custom:'.$field->getFieldKey()] = $field->getValueForStorage();
}

I noted that higher up in in unrelated getBuiltinOrders(), there are two lines

$legacy_key = 'custom:'.$field->getFieldKey();
$modern_key = $field->getModernFieldKey();

In https://we.phorge.it/source/phorge/browse/master/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php$505, getModernFieldKey() returns 'custom.'.$this->getRawStandardFieldKey(); for non-built-in custom fields.

I assume that some fix must go into buildPagingClause() which initially defines the $keys then passed to several functions as a parameter but that's doing $keys[] = $order->getOrderKey().

valerio.bozzolan subscribed.Sep 4 2023, 10:54
aklapper added a comment.Sep 5 2023, 11:27

In theory this could be a two-liner: Changing the last character of the $prefix, and in PhabricatorCursorPagedPolicyAwareQuery.php

- $map[$field->getFieldKey()] = $field->getValueForStorage();
+ $map[$field->getModernFieldKey()] = $field->getValueForStorage();

In practice that leads to: Unhandled Exception ("PhutilTypeExtraParametersException") - Got unexpected parameters: customfield, customfield.index.table, customfield.index.key

aklapper added a revision: D25504: Fix "Map omits required key" exception by comparing result order against modern field keys.Jan 4 2024, 09:17
valerio.bozzolan awarded a token.Jan 12 2024, 22:12
valerio.bozzolan triaged this task as Normal priority.Mar 10 2024, 12:05

Interestingly I was able to reproduce, but only creating 101+ tasks manually and going to the next page. So after that ?after=100 is introduced.

Maybe this can reproduced just adding ?after= like ?after=1

valerio.bozzolan added a comment.Mar 10 2024, 16:25

Maybe unrelated. But after we fix PhabricatorCursorPagedPolicyAwareQuery, maybe we will find a crash test also for the first two lines:

$ grep -FR "'custom:'." .
./applications/search/field/PhabricatorSearchCustomFieldProxyField.php:    $this->setKey('custom:'.$field->getFieldIndex());
./infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php:        $saved->getParameter('custom:'.$field->getFieldIndex()));
./infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:        $legacy_key = 'custom:'.$field->getFieldKey();
./infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php:      $map['custom:'.$field->getFieldKey()] = $field->getValueForStorage();
aklapper closed this task as Resolved by committing rPd42b3eb0b17a: Fix "Map omits required key" exception by comparing result order against modern….Mar 21 2024, 18:25
aklapper claimed this task.
aklapper added a commit: rPd42b3eb0b17a: Fix "Map omits required key" exception by comparing result order against modern….
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0