Page MenuHomePhabricator

Improve MCR handling of RevisionAccessException family
Open, NormalPublic

Description

A number of methods introduced with MCR can throw RevisionAccessException, but it's largely unclear what this means and when it happens. Currently there are two subclasses:

  • SuppressedDataException is thrown when an object contains data not visible to the user whom it was requested for. It is used inconsistently, e.g. RevisionRecord::getContent will return null when that is the case, while RevisionRecord::getSlot will return a fake SlotRecord that will throw when accessed.
  • IncompleteRevisionException is throw, according to its documentation, when "trying to access undefined fields on an incomplete RevisionRecord". It seems to be used when trying to access or store an unsaved revision with missing fields (as opposed to when reading invalid data from the database, which would be another natural interpretation).
  • A generic RevisionAccessException is thrown on some database inconsistencies (e.g. a revision record referencing a non-existing page) which probably should not happen unless there is data corruption or improper transaction handling. It is also thrown on invalid slot names, and the documentation mentions "slot data could not be lazy-loaded" as an error condition (whatever that means).

It is entirely unclear whether this is entirety of situations in which RevisionAccessException can be thrown; i.e. whether it is fine to ignore them when one is not dealing with unsaved revisions and has done an audience check. Further, it is not a helpful way of grouping exceptions: RevisionAccessException is a runtime error (ie. something that developers are not supposed to handle as it only happens in abnormal situations) while suppression is a situation that developers should handle (ie. not catching SuppressedDataException seems like a logic error). What makes this really annoying is that pretty much everything in the revision/slot classes is documented as throwing a RevisionAccessException, and it is unclear whether that includes suppressed data errors (looking at the code, the answer seems "no").

One way to deal with this situation would be to make RevisionRecord::getSlot return null on suppressed content like everything else, at which point SuppressedDataException could be killed. If that's not possible, at the very least it should be a separate exception not descending from RuntimeException.

Another situation that needs improvement is RevisionAccessException being triggered when a revision with a given ID is not found. This is currently treated as database corruption, but may in fact occur during regular operation doe to replication lag (the revision has not yet be replicated) or race conditions (the revision row has concurrently been removed doe to page deletion).

Related Objects

StatusAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenAnomie
Opendaniel

Event Timeline

Tgr created this task.Jun 25 2018, 1:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 25 2018, 1:32 PM
Vvjjkkii renamed this task from Improve MCR handling of RevisionAccessException family to kcaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot renamed this task from kcaaaaaaaa to Improve MCR handling of RevisionAccessException family.
CommunityTechBot added a subscriber: Aklapper.
daniel updated the task description. (Show Details)Mar 5 2019, 5:34 PM
daniel triaged this task as Normal priority.
daniel added a project: User-Daniel.
daniel added a subscriber: BPirkle.