Page MenuHomePhabricator

Flow: failed API call doesn't provide exception message
Closed, ResolvedPublic

Description

Flow exceptions report a a specific "message from exception, used for debugging error" as well as a generic error code that is localized and shown to the user. It seems exceptions in API calls don't return the former, making them harder to debug.

To reproduce:

Stop the parsoid service and issue an action=flow-parsoid-utils request such as:

http://localhost/w/api.php?action=flow-parsoid-utils&format=json&from=wikitext&to=html&content=Hello&title=Main_Page

The API response is

{"error":{
  "code":"flow-error-process-wikitext",
  "info":"An error has occurred while processing HTML/wikitext conversion."
}}

but this error code is returned for a number of problems:

throw new WikitextException( 'Unknown source format: ' . $from, 'process-wikitext' );
throw new WikitextException( 'Failed contacting Parsoid', 'process-wikitext' );
throw new WikitextException( "Unknown format requested: " . $to, 'process-wikitext' );
throw new WikitextException( 'Parser only supports wikitext to HTML conversion', 'process-wikitext' );

It would be nice if the API response included the exception $message (instead of/as well as the English translation of the error code). It doesn't need to be localized and shouldn't be presented to the user.


Version: master
Severity: minor

Details

Reference
bz67000

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:33 AM
bzimport set Reference to bz67000.
bzimport added a subscriber: Unknown Object (MLST).
matthiasmullie triaged this task as Normal priority.Dec 11 2014, 4:59 PM
matthiasmullie set Security to None.
matthiasmullie removed a subscriber: Maryana.

I'll take a crack at this one. I isolated the issue to line 18 of /Flow/includes/api/ApiParsoidUtilsFlow.php. That command creates the error message displayed as the response, but it does so without the message passed to the exception constructor. I'm a little confused as to what the report means by "shouldn't be presented to the user." Since this is my first work with the MW API, I don't really know if the current error message is "presented to the user," or what I need to avoid. I was just going to concatenate the specific message to the translated string, with a space separating them. Would that work?

PS I couldn't reproduce the error just by stopping the Parsoid service. I had to temporarily modify the code in /Flow/includes/Parsoid/Utils.php.

I'll take a crack at this one. I isolated the issue to line 18 of /Flow/includes/api/ApiParsoidUtilsFlow.php. That command creates the error message displayed as the response, but it does so without the message passed to the exception constructor. I'm a little confused as to what the report means by "shouldn't be presented to the user."

It means that Flow (the user interface which posters interact with) should not display the detailed exception to the end user.

Since this is my first work with the MW API, I don't really know if the current error message is "presented to the user," or what I need to avoid.
I was just going to concatenate the specific message to the translated string, with a space separating them. Would that work?

If set, the 'info' is displayed to the user, so you need to make sure that does not overwhelm the user with technical detail.

The line you mentioned throws an exception indirectly using dieUsage (this is a core method). (Parsoid being down/broken is not really a usage error. But since this is supposed to be good first bug, we'll ignore that for now).

Take a look at dieUsage (and any code it calls), and see where you can put extra data (other than the code and info) that would be useful for people watching the network console (such as Flow developers), or to developers of bots that work with Flow, but would not be visible to the end user (since it doesn't affect code or info).

That extra info would be the full exception details from the WikitextException that was caught.

So like this?

$this->dieUsage( $this->msg( $code )->inContentLanguage()->useDatabase( false )->plain(),
				$code, $e->getStatusCode(), array( $e->getMessage() ) );

I think $extradata is supposed to be an associative array.

So what should the key be? Right now, it's just 0. Is everything else all right?

So what should the key be? Right now, it's just 0. Is everything else all right?

I don't think there are any relevant magic keys. Just use a key you think is descriptive. I will review and test when it's posted to Gerrit. Be sure to add me as a reviewer.

Change 199816 had a related patch set uploaded (by Happy5214):
Adding $message of caught WikitextException object to displayed error

https://gerrit.wikimedia.org/r/199816

jayvdb added a subscriber: jayvdb.Mar 26 2015, 3:05 AM

Change 199816 merged by jenkins-bot:
Adding $message of caught WikitextException object to displayed error

https://gerrit.wikimedia.org/r/199816

Mattflaschen-WMF closed this task as Resolved.Mar 27 2015, 4:58 AM
Mattflaschen-WMF assigned this task to happy5214.