Page MenuHomePhabricator

Separate internal and external Query Cursors more cleanly, to fix pagination against broken objects
ClosedPublic

Authored by epriestley on Mar 18 2019, 6:19 PM.

Details

Summary

Ref T13259.

This is "infrastructure/guts" only and breaks some stuff in Query subclasses. I'll fix that stuff in a followup, it's just going to be a larger diff that's mostly mechanical.

When a user clicks "Next Page" on a tasks view and gets ?after=100, we want to show them the next 100 visible tasks. It's possible that tasks 1-100 are visible, but tasks 101-788 are not, and the next visible task is 789.

We load task ID 100 first, to make sure they can actually see it: you aren't allowed to page based on objects you can't see. If we let you, you could use "order=title&after=100", plus creative retitling of tasks, to discover the title of task 100: create tasks named "A", "B", etc., and see which one is returned first "after" task 100. If it's "D", you know task 100 must start with "C".

Assume the user can see task 100. We run a query like id > 100 to get the next 100 tasks.

However, it's possible that few (or none) of these tasks can be seen. If the next visible task is 789, none of the tasks in the next page of results will survive policy filtering.

So, for queries after the initial query, we need to be able to page based on tasks that the user can not see: we want to be able to issue id > 100, then id > 200, and so on, until we overheat or find a page of results (if 789-889 are visible, we'll make it there before overheating).

Currently, we do this in a not-so-great way:

  • We pass the external cursor (100) directly to the subquery.
  • We query for that object using getPagingViewer(), which is a piece of magic that returns the real viewer on the first page and the omnipotent viewer on the 2nd..nth page. This is very sketchy.
  • The subquery builds paging values based on that object (array('id' => 100)).
  • We turn the last result from the subquery back into an external cursor (200) and save it for the next time.

Note that the last step happens BEFORE policy (and other) filtering.

The problems with this are:

  • The phantom-schrodinger's-omnipotent-viewer thing isn't explicity bad, but it's sketchy and generally not good. It feels like it could easily lead to a mistake or bug eventually.
  • We issue an extra query each time we page results, to convert the external cursor back into a map (100, 200, 300, etc).
  • In T13259, there's a new problem: this only works if the object is filtered out for policy reasons and the omnipotent viewer can still see it. It doesn't work if the object is filtered for some other reason.

To expand on the third point: in T13259, we hit a case where 100+ consecutive objects are broken (they point to a nonexistent repositoryID). These objects get filtered unconditionally. It doesn't matter if the viewer is omnipotent or not.

In that case: we set the next external cursor from the raw results (e.g., 200). Then we try to load it (using the omnipotent viewer) to turn it into a map of values for paging. This fails because the object isn't loadable, even as the omnipotent viewer.


To fix this stuff, the new approach steps back a little bit. Primarily, I'm separating "external cursors" from "internal cursors".

An "External Cursor" is a string that we can pass in ?after=X URIs. It generally identifies an object which the user can see.

An "Internal Cursor" is a raw result from loadPage(), i.e. before policy filtering. Usually, (but not always) this is a LiskDAO object that doesn't have anything attached yet and hasn't been policy filtered.

We now do this, broadly:

  • Convert the external cursor to an internal cursor.
  • Execute the query using internal cursors.
  • If necessary, convert the last visible result back into an external cursor at the very end.

This fixes all the problems:

  • Sketchy Omnipotent Viewer: We no longer ever use an omnipotent viewer. (We pick cursors out of the result set earlier, instead.)
  • Too Many Queries: We only issue one query at the beginning, when going from "external" to "internal". This query is generally unavoidable since we need to make sure the viewer can see the object and that it's a real / legitimate object. We no longer have to query an extra time for each page.
  • Total Failure on Invalid Objects: we now page directly with objects out of loadPage(), before any filtering, so we can page over invisible or invalid objects without issues.

This change switches us over to internal/external cursors, and makes simple cases (ID-based ordering) work correctly. It doesn't work for complex cases yet since subclasses don't know how to get paging values out of an internal cursor yet. I'll update those in a followup.

Test Plan

For now, poked around a bit. Some stuff is broken, but normal ID-based lists load correctly and page properly. See next diff for a more detailed test plan.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mar 18 2019, 6:19 PM
epriestley requested review of this revision.Mar 18 2019, 6:21 PM

Old sequence:

  • For each page:
    • Convert the current external cursor ("100") into a real object by loading it with the "paging viewer".
    • Convert the real object into a map (array('id' => 100)).
    • Load the page (WHERE id > 100 LIMIT 100).
    • Convert the last item on the page into an external cursor ("200") and save it as the current external cursor.
    • Set a flag that changes the "paging viewer" into the Omnipotent viewer.
    • Filter the page.

In particular, this breaks on the second page if the new cursor ("200") can't load into a real object.


New sequence:

  • Convert the external cursor ("100") into an internal cursor.
  • For each page:
    • Convert the internal cursor into a map.
    • Load the page.
    • Save the last item on the page as the internal cursor.
    • Filter the page.
  • When complete: convert the last visible result back into an external cursor.
amckinley accepted this revision.Mar 18 2019, 9:51 PM
This revision is now accepted and ready to land.Mar 18 2019, 9:51 PM
This revision was automatically updated to reflect the committed changes.