Page MenuHomePhorge

Custom integer fields: fix search by array of possible values
ClosedPublic

Authored by valerio.bozzolan on Mar 7 2024, 15:05.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 17:44
Unknown Object (File)
Fri, Apr 26, 17:44
Unknown Object (File)
Fri, Apr 26, 17:44
Unknown Object (File)
Fri, Apr 26, 17:44
Unknown Object (File)
Fri, Apr 26, 17:43
Unknown Object (File)
Thu, Apr 25, 22:56
Unknown Object (File)
Tue, Apr 23, 07:08
Unknown Object (File)
Mon, Apr 22, 11:26

Details

Summary

This minimal changes seems the natural expansion of this search function,
that "seems" designed to only search with a single value, but the backend
is designed to receive an array of possible values.

Look at the same method in PhabricatorStandardCustomFieldLink that already
works also for array inputs.

To get extra confidence look at the method withApplicationSearchContainsConstraint()
that works perfectly with arrays (to be honest it only works with arrays in mind...)

This feature allows to avoid crashes when extension developers tries to run "IN" queries
because for example they try to follow our performance guidelines:

https://we.phorge.it/book/contrib/article/n_plus_one/

Closes T15752

Test Plan

Have a nice custom integer field, like this one for Maniphest:

{
  "mycompany.estimated-hours": {
    "name": "Estimated Hours",
    "type": "int",
    "caption": "Estimated number of hours this will take.",
    "search": true
  }
}

You can reproduce T15752 before the change. It just works after the change.

Or, just trust your instincts by looking at the same method in PhabricatorStandardCustomFieldLink.

Also, use the "normal" frontend search page. Still works as usual.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Be more friendly against the backend logic, and with PhabricatorStandardCustomFieldLink

Also skip empty arrays like PhabricatorStandardCustomFieldLink

add inline documentation because we can

Small recap:

Before

$valueapplyApplicationSearchConstraintToQuery(..., $value)
null
0WHERE field IN( 0 )
1WHERE field IN( 1 )
array(1,2)crash

After

$valueapplyApplicationSearchConstraintToQuery(..., $value)
null
0WHERE field IN( 0 )
1WHERE field IN( 1 )
array(1,2)WHERE field IN (1, 2)
This revision is now accepted and ready to land.Mar 30 2024, 11:01