Add custom error handling

[hannaeko]

Purpose

Make error management a bit more manageable.

Context

• Partially addresses Application specific error codes #819 (with limitations due to downstream dependencies; codes definition is not covered).
• Fixes Remove number suffix from Internal error messages #817

Changes

Built on top of #843, planned to be integrated with #844 to provide server error counters.

Adds a base error type Zonemaster::Backend::Error extended to the following classes:

• Zonemaster::Backend::Error::Internal (covers code -32603, but subclassed to add more meaning when applicable)
  • Zonemaster::Backend::Error::JsonError
• Zonemaster::Backend::Error::ResourceNotFound (custom code -32000)
• Zonemaster::Backend::Error::PermissionDenied (custom code -32001)
• Zonemaster::Backend::Error::Conflict (custom code -32002)

This type is used to differentiate actual unexpected server error and "normal" exceptions (a test that does not exist in the database should not result in a server error, even less in a json parsing error). Also I noticed that sometimes null is returned when a test is not found (like with get_test_progress) and sometimes an error is returned, what should be the correct behavior? Returning a custom error seems to be an accepted pattern from what I have seen in other implementations, it can also have some context about the error.

It also adds the possibility to add meaning and context to the error, both to the operator, in the logs, and to the user, in the response.

It theoretically adds the ability to set a custom error code but that is not yet usable due to a limitation of the JSON::RPC module. I have made a pull request to address that matter but the maintainer of the repo do not seem active anymore (the package on cpan has been marked for adoption).

If / when custom code are available in the JSON RPC module, the parameter validation should be moved after the method routing, relying on the external module to parse and validate the method (what it is already doing).

How to test this PR

% ./script/zmb get_test_params --test-id a260536b98076a81 | jq
{
  "jsonrpc": "2.0",
  "error": {
    "message": "Test not found",
    "data": {
      "test_id": "a260536b98076a81"
    },
    "code": -32603
  },
  "id": 1
}
% ./script/zmb add_api_user --username toto23 --api-key secret | jq
{
  "error": {
    "data": {
      "username": "toto23"
    },
    "code": -32603,
    "message": "User already exists"
  },
  "jsonrpc": "2.0",
  "id": 1
}

% ./script/zmb add_batch_job --domain example.com --username toto --api-key secret | jq
{
  "jsonrpc": "2.0",
  "error": {
    "message": "User not authorized to use batch mode",
    "code": -32603,
    "data": {
      "username": "toto"
    }
  },
  "id": 1
}

% ./script/zmb add_batch_job --domain example.com --username toto23 --api-key secret | jq
{
  "error": {
    "data": {
      "batch_id": 6,
      "creation_time": "2021-08-17 14:11:43"
    },
    "code": -32603,
    "message": "Batch job still running"
  },
  "id": 1,
  "jsonrpc": "2.0"
}

# Test result existence has been disable to show case a JsonError
% ./script/zmb get_test_results --test-id 123456789abcdef0 --lang fr | jq {
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": -32603,
    "data": {
      "test_id": "123456789abcdef0"
    },
    "message": "Internal server error"
  }
}

Generated logs:

2021-08-17T14:24:26Z [8847] ERROR - Caught Zonemaster::Backend::Error::JsonError in the `Zonemaster::Backend::DB::SQLite::test_results` method: malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "(end of string)") at /home/gbm/workspace/zonemaster/zonemaster-backend/lib/Zonemaster/Backend/DB/SQLite.pm line 280. Context: {'test_id' => '123456789abcdef0'}
2021-08-17T14:28:01Z [9080] ERROR - Caught Zonemaster::Backend::Error::Internal in the `Zonemaster::Backend::RPCAPI::get_test_results` method: Can't call method "plouf" on an undefined value at /home/gbm/workspace/zonemaster/zonemaster-backend/lib/Zonemaster/Backend/RPCAPI.pm line 432.
2021-08-17T14:28:28Z [9107] INFO - Test not found (code -32000). Context: {'test_id' => '123456789abcdef0'}
2021-08-17T14:41:27Z [9110] INFO - Batch job still running (code -32002). Context: {'batch_id' => 7,'creation_time' => '2021-08-17 14:41:26'}
2021-08-17T14:41:51Z [9111] INFO - User not authorized to use batch mode (code -32001). Context: {'username' => 'toto'}
2021-08-17T14:42:30Z [9107] INFO - User already exists (code -32002). Context: {'username' => 'toto23'}

[mattias-p]

I like this design!

[ghost]

This looks great!

[ghost]

Could add an indentation here before the unless to make it easier to read?

[hannaeko]

Done

[ghost]

Maybe you can remove the $_method since it is not used, my understanding is that _build_method() retrieve the method's name automatically.

[hannaeko]

Done

[mattias-p]

This looks really nice. I have one question/suggestion and a couple of formatting nits.

[mattias-p]

Really nice work!

[ghost]

Since the method name is not used anymore, is it still required to pass it to handle_exception()?

[hannaeko]

No, it can be removed

[matsduf]

$ make distcheck
"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
Not in MANIFEST: lib/Zonemaster/Backend/Errors.pm

[matsduf]

When MANIFEST is fixed, I will test.

[matsduf]

Looks fine.

[mattias-p]

I've tested this for v2021.2 and it LGTM.

The last test case listed under How to test this PR didn't work out of the box because it relies on a bug that has been fixed. Instead I injected a bug into Zonemaster::Backend::DB::PostgreSQL::test_progress() that ensures that the decoded JSON blob is invalid before attempting to parse it. After this I was able to successfully run that test case too.