Page MenuHomePhabricator

Allow subclasses of IndexPager to process the offsets from the querystring before using them to build SQL
Open, Needs TriagePublic

Description

IndexPager assumes that offsets from the querystring are always strings and treats them as such when building the query, see code. This works just fine with MySQL, but not with SQLite. If the thing that those values are compared to is not a string, the comparison will evaluate to false in SQLite because the operands have different types. This leads to issues such as T312054, where the query is something like:

select * from (select count(*) as n from foo) tmp where n = X

X is the value from the querystring, and since it is a string, the comparison will always be false, see this snippet. In principle, I can think of 3 places where this could be fixed:

  • In the subclass, casting the first operand (the one that is not from the querystring) to the appropriate value. This looks wrong though, especially in cases like the example above where we'd be casting the result of count() to string, whereas the conversion should be the other way around.
  • In IndexPager. Note that you have no way to determine the intended type for a value from the querystring; for instance, timestamps are numeric but they are stored as strings in the database.
  • At the database level. Unfortunately, I don't think this is possible in SQLite, because that's just the way it works. Newer versions have a strict type (see T289521), but I don't think it would enable autocasts in comparisons.

So I would propose adding a new method to IndexPager, e.g.,

protected function processQueryOffsets( array &$offsets ): void {
  // The default implementation does nothing, subclasses should override the method.
}

that is called after the explode() and is passed an array of offset_name => offset_value_from_querystring. Each subclass should know the intended type of each offset and could cast it accordingly.

Alternatives are welcome, too.