Overwriting api errors

[bontric]

Overwriting api errors

In this specific situation (and potentially other situations as well) an api_error gets overwritten and therefore hides the initial error message.

If we take a look at handle_run in dispatch.c (76f5d29945d8f71f1031951e3154aa30543d69df) we can spot the problem:

//...
// dispatch.c:234
  params = api_run(pluginlongtermpk, function_name, callid, args, api_error);

  if (params == NULL) {
    error_set(api_error, API_ERROR_TYPE_VALIDATION,
        "Error creating run API request.");
    return (-1);
  }
//...

api_run might have set api_error already and it is then set a second time afterwards. The problem here is, that api_run sometimes sets api_error and sometimes doesn't.

//run.c:36
//..
  /* check if id is in database */
  if (db_apikey_verify(pluginlongtermpk) == -1) {
    error_set(api_error, API_ERROR_TYPE_VALIDATION, "API key is invalid.");
    return (NULL);
  }
//..
// run.c:63
  if (!meta->data.params.obj)
    return (NULL);
//..

This is something we should be aware of. Here are a few suggestions to handle this:

• Only set api_error on a high level (for example in the handle_ functions). We therefore wouldn't provide detailed information about the errors but this might be unnecessary anyways (discuss!) .
• Set api_error where the errors occurs. This will provide detailed information for the debugging of plugin <-> server interactions, but adds a lot of code to the functions.
• Allow api_error to contain multiple error messages, which will prevent us from overwriting them.

  Note

We're planning to implement error codes. If we keep this problem in mind during implementation, the issue will be resolved.

[mkind]

Only set apierror on a high level (for example in the handle functions). We therefore wouldn't provide detailed information about the errors but this might be unnecessary anyways (discuss!) .

Maybe I am wrong, but wasn't this the plan in the beginning?

Set api_error where the errors occur. This will provide detailed information for the debugging of plugin <-> server interactions, but adds a lot of code to the functions.

Some detailed information might be nice. However, I am not really clear about that.

Allow api_error to contain multiple error messages, which will prevent us from overwriting them.

To me this solution sounds like a big one with potential side effects.

[stze]

//...
// dispatch.c:234
  params = api_run(pluginlongtermpk, function_name, callid, args, api_error);

  if (params == NULL) {
    error_set(api_error, API_ERROR_TYPE_VALIDATION,
        "Error creating run API request.");
    return (-1);
  }
//...

this can be easily resolved by checking if api_error.isset is set to true after calling api_run. if it is set, return (-1).

//...
// dispatch.c:234
  params = api_run(pluginlongtermpk, function_name, callid, args, api_error);

  if (api_error->isset) {
    return (-1);
  }

  if (params == NULL) {
    error_set(api_error, API_ERROR_TYPE_VALIDATION,
        "Error creating run API request.");
    return (-1);
  }
//...

[mkind]

So far fixed for handle_run. However, we decided to specify details on error handling in a separate issue.