Page MenuHomePhabricator

Improve MCR handling of RevisionAccessException family
Open, MediumPublic

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

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenFeatureNone
OpenBUG REPORTNone
OpenNone
StalledNone
OpenFeatureNone
DuplicateNone
ResolvedNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
ResolvedNone
ResolvedNone
OpenFeatureNone
OpenNone
OpenFeatureNone
StalledNone
OpenNone
OpenNone
OpenNone

Event Timeline

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 renamed this task from kcaaaaaaaa to Improve MCR handling of RevisionAccessException family.Jul 2 2018, 10:11 AM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
daniel triaged this task as Medium priority.Mar 5 2019, 5:34 PM
daniel updated the task description. (Show Details)
daniel added a project: User-Daniel.
daniel added a subscriber: BPirkle.
daniel subscribed.