anvc / scalar

Born-digital, open source, media-rich scholarly publishing that’s as easy as blogging.
Other
231 stars 73 forks source link

Handling database errors upon update/insert #113

Open burki opened 5 years ago

burki commented 5 years ago

When entering a media or page title that exceeds the length defined for scalar_db_versionstitle (currently varchar(255)), an unhandled exception is triggered in https://github.com/anvc/scalar/blob/master/system/application/models/version_model.php#L452

if (empty($version_id)) throw new Exception('Could not resolve version ID before saving to the semantic store.');

Therefore, /api/update will contain a response along the lines

Fatal error: Uncaught Exception: Could not resolve version ID before saving to the semantic store. in ...\system\application\models\version_model.php:452

which isn't a valid JSON-object, thus causing the line

var error = jQuery.parseJSON(obj.responseText);

in scalarapi.js:1878 to break ("SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data"), so that the user interface gets stuck with the message "Saving page..." :

   $.ajax({
      url: this.model.urlPrefix+'api/'+data['action'].toLowerCase(),
      data: tosend_formatted,
      type: "POST",
      dataType: 'json',
      success: function(json) {
        successCallback(json);
      },
      error: function(obj) {
        var error = jQuery.parseJSON(obj.responseText);
        errorCallback(error.error.message[0].value);
      }

If you point me to a proper error-response object from the API, I'll be willing to try to implement a proper response for this specific case.

burki commented 5 years ago

Following up on this, wrapping the database-calls in https://github.com/anvc/scalar/blob/a5875a3edeaa974f0e162e3dc0a2fc05d783bec3/system/application/controllers/api.php#L205 into a try / catch along the lines of

    try {
        $this->pages->save($save_content);

        //save the version
        $save_version = $this->_array_remap_version($this->data['content_id']);
        $this->data['version_id'] = $this->versions->create($this->data['content_id'], $save_version);
    }
    catch (Exception $e) {
        $this->_output_error(StatusCodes::HTTP_BAD_REQUEST, $e->getMessage());
    }

seems to work as a guard. If this seems sane, I'm happy to submit a pull request. In an additional step, the database error might be analyzed to make the current message (Something went wrong while attempting to save: Could not resolve version ID before saving to the semantic store.) a bit more specific (indicating which field is too long).

arthurian commented 4 years ago

We recently experienced this issue as well, although the cause was different. In our case, a corruption in the scalar_store_triple table caused the /api/update request to return an invalid response like this:

Array
(
    [0] => Can't find record in 'scalar_store_triple' via ARC2_StoreConstructQueryHandler
    [1] => MySQL error: Incorrect key file for table './scalar/scalar_store_triple.MYI'; try to repair it (INSERT IGNORE INTO scalar_store_triple (t, s, p, o, o_lang_dt, o_comp, s_type, o_type) VALUES (3815, 5994, 5995, 5970, 5, '1,1', 0, 2), (3816, 5994, 5996, 5968, 5, '2020-02-03', 0, 2)) in ARC2_StoreLoadQueryHandler
)
{
  "https://scalar.fas.harvard.edu/test-book/my-test.3/" : {
    "http://open.vocab.org/terms/versionnumber" : [
      { "value" : "3", "type" : "literal" }
    ],
    "http://purl.org/dc/terms/title" : [
      { "value" : “my test", "type" : "literal" }
    ],
    "http://rdfs.org/sioc/ns#content" : [
      { "value" : “my test", "type" : "literal" }
    ],
    "http://scalar.usc.edu/2012/01/scalar-ns#defaultView" : [
      { "value" : "plain", "type" : "literal" }
    ],
    "http://scalar.usc.edu/2012/01/scalar-ns#editorialState" : [
      { "value" : "draft", "type" : "literal" }
    ],
    "http://www.w3.org/ns/prov#wasAttributedTo" : [
      { "value" : "users/1", "type" : "literal" }
    ],
    "http://purl.org/dc/terms/created" : [
      { "value" : "2020-06-10T12:31:19-04:00", "type" : "literal" }
    ],
    "http://scalar.usc.edu/2012/01/scalar-ns#urn" : [
      { "value" : "urn:scalar:version:18722", "type" : "uri" }
    ],
    "http://www.w3.org/1999/02/22-rdf-syntax-ns#type" : [
      { "value" : "http://scalar.usc.edu/2012/01/scalar-ns#Version", "type" : "uri" }
    ]
  } 
}

The response didn't indicate an error, so as far as the client knew, the update was successful. The client would proceed to call JSON.parse(), but since it's not valid JSON, that step would fail and the JS would raise an unhandled exception. This would then cause any outstanding requests to be aborted, leaving the page and its relations in an incomplete state.

The reason the response didn't return an error response is due to the error handling in rdf_store->save_by_urn() which is called when a new version is created. The save_by_urn() method prints errors instead of throwing an exception:

https://github.com/anvc/scalar/blob/c76aa2cde555bd69bbdc99f2a29fc66578ba7a56/system/application/libraries/RDF_Store.php#L137-L140

From the user point of view, they would see the Saving page... message indefinitely (it never appeared to succeed from their point of view and no error was shown). Another side effect of the update failing was that any relations associated with the page appeared to be deleted. In reality, the new version was successfully created, but the metadata was not saved because of the corrupt table error, and the relations were not associated with the new version because the client did not issue the subsequent /api/relate requests that would typically follow an /api/update.

I think the update suggested by @burki is a good start to ensure that any exceptions that occur within the update method are properly returned as an error to the client.

In addition to that update, we should throw an exception or return an error value in the rdf_store->save_by_urn() so that it doesn't fail silently. Something like this perhaps:

$this->store->insert($resource->index, $g);
if ($errs = $this->store->getErrors()) {
    throw new Exception($errs);
}

These updates should help surface these errors a bit better to the client, although I don't know that it fully addresses the issue of what happens when an error potentially leaves a page in an incomplete state.