Fix "Map omits required key" exception by comparing result order against modern field keys
ClosedPublic
Actions

Authored by aklapper on Jan 4 2024, 09:17.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15631: "Map returned omits required key" exception for second result page (cursor) for Maniphest search ordered by custom field

Commits

rPd42b3eb0b17a: Fix "Map omits required key" exception by comparing result order against modern…

Summary

Sorting a Maniphest search query by custom fields throws a "Map returned omits required key" exception.

The isCustomFieldOrderKey() check still tested against legacy field key format (for example [custom:]std:maniphest:deadline.due) while the code passes modern field key format (for example custom.deadline.due).

After fixing this, PhutilTypeSpec::checkMap() throws an exception when a non-optional (extra) key $column is in $columns but not in the array of type parameters below to check against:
"Got unexpected parameters: customfield, customfield.index.table, customfield.index.key"
Thus add optional types for customfields in buildPagingClauseFromMultipleColumns() to allow them instead of throwing another exception.

Closes T15631

Test Plan

Have a custom field (with search: true) defined via http://phorge.localhost/config/edit/maniphest.custom-field-definitions/
Have two tasks have the custom field set
Go to http://phorge.localhost/maniphest/query/ , select the custom field (e.g. "Due Date (Reversed)") in the "Order By" field, and click the "Search" button. Iterate through the results via the "Next" button if needed

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Jan 4 2024, 09:17

Owners added a reviewer: O1: Blessed Committers.Jan 4 2024, 09:17

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 4 2024, 09:17

aklapper requested review of this revision.Jan 4 2024, 09:17

Harbormaster completed remote builds in B1001: Diff 1609.Jan 4 2024, 09:17

valerio.bozzolan edited the test plan for this revision. (Show Details)Jan 12 2024, 22:14

lgtm

Thanks \o/ Can reproduce.

I tried all Order By fields in Maniphest, with and without ?after= and finally no crash with ?after=1 or anything. Tried also Differential and still works.

I think nobody noticed because usually people rarely go to the second page of their search engine in Phorge. I mean, Phorge shows 100 (!) results in the first page. That is up to 5~ times more stuff on first page compared than other popular applications. For instance in GitLab I'm always hitting the next page since it shows just 20 elements. Same with GitHub. Still, though, it amazes me that there is no bug in secure.dot on this.

This revision is now accepted and ready to land.Mar 10 2024, 16:41

Closed by commit rPd42b3eb0b17a: Fix "Map omits required key" exception by comparing result order against modern…. · Explain WhyMar 21 2024, 18:25

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPd42b3eb0b17a: Fix "Map omits required key" exception by comparing result order against modern….