antelope icon indicating copy to clipboard operation
antelope copied to clipboard

Missing error details on API error

Open theblockstalk opened this issue 2 years ago • 8 comments

The error created from an error response from API calls is missing and not printing extra information in the json.error.details object that would be useful for developers to see. Only the json.error.name and json.error.what is printed.

Error response from v1.chainpush_transaction:

{
      headers: {
        'access-control-allow-headers': '*',
        'access-control-allow-origin': '*',
        connection: 'close',
        'content-length': '531',
        'content-type': 'application/json',
        server: 'WebSocket++/0.7.0'
      },
      status: 500,
      json: {
        code: 500,
        message: 'Internal Service Error',
        error: {
          code: 3090003,
          name: 'unsatisfied_authorization',
          what: 'Provided keys, permissions, and delays do not satisfy declared authorizations',
          details: [
            {
              message: "transaction declares authority '${auth}', but does not have signatures for it under a provided delay of 0 ms, provided permissions ${provided_permissions}, provided keys ${provided_keys}, and a delay max limit of 3888000000 ms",
              file: 'authorization_manager.cpp',
              line_number: 532,
              method: 'check_authorization'
            }
          ]
        }
      },
      text: '{"code":500,"message":"Internal Service Error","error":{"code":3090003,"name":"unsatisfied_authorization","what":"Provided keys, permissions, and delays do not satisfy declared authorizations","details":[{"message":"transaction declares authority \'${auth}\', but does not have signatures for it under a provided delay of 0 ms, provided permissions ${provided_permissions}, provided keys ${provided_keys}, and a delay max limit of 3888000000 ms","file":"authorization_manager.cpp","line_number":532,"method":"check_authorization"}]}}'
    }

theblockstalk avatar Aug 16 '22 13:08 theblockstalk

The information exists inside the thrown error, just not printed. Maybe consider that it is printed to cater for ignorant devs (oops).

Suggest this issue is closed.

theblockstalk avatar Aug 23 '22 06:08 theblockstalk

Displaying the details as it outputs the error is something I think would be useful as well. It's caught me off guard a few times where I have to manually go try queries or dig deeper into the error messages to find the actual root of the error.

@jnordberg any immediate thoughts on how we could improve this?

aaroncox avatar Sep 05 '22 15:09 aaroncox

We should improve this function. It tries to pluck out the relevant into to the error message string but could be a lot better.

jnordberg avatar Oct 19 '22 06:10 jnordberg

I would like to work on this. Current thinking is to return error.details[0].message even when what is defined.

@theblockstalk can you provide more examples to test against?

ericpassmore avatar Oct 24 '22 16:10 ericpassmore

Delving into the code, and now have a new/better understanding. I dove in and updated the method. It didn't work out too well as falsy values created verbose code, and forced isRejected tests on promises . @jnordberg Proposing a solution here, as always open to other ways of doing things.

  • Refactor APIError

    • Scope APIErrorData to the data we care about.
    • Typecheck parsed Json data against APIErrorData to eliminate falsy values
      • if needed we can do fine grained typechecks if we want to create field specific defaults
    • Typecheck added as as part of the constructor.
    • Format message is now safe with no falsy values, eliminating verbose existence checks
    • Get methods are simplified with no falsy values, and default values are eliminated
  • Refactor Tests

    • Fewer negative tests are needed because if/else conditions will collapse
    • Negative tests have catch chai assertion inside catch block.

Lots of alternatives.

ericpassmore avatar Oct 31 '22 18:10 ericpassmore

Regarding the scope of APIErrorData, I'm not sure we need to check every permutation. From looking at logs there are only a few data values we care about.

  1. Has nodeos Error.code, Error.what, and Error.details[].message
  2. No nodeos Error.code, no Error.what, no Error.details[]

We can consider one additional case, to make the code more robust 3) Has Error.code and may or may not have Error.what or Error.details[].message

Simplifying what cases to support will collapse the number of if/else statements. In the case of number two above, APIError would never be created. Thats ok because errors can come from anywhere, there is no guarantee of APIError, and consumers of eosio.core always need to instanceof check the returned error.

ericpassmore avatar Nov 01 '22 04:11 ericpassmore

Still have this on my queue. Will submit new PR with hopefully, improved Json error parsing and simpler testing.

ericpassmore avatar Nov 17 '22 19:11 ericpassmore

I ran into this again today while working on the @wharfkit session library. When casting an error thrown by the APIClient (e.g. String(error)) it will not show the details, however the details can be accessed through the .response properly on the error. I wanted to record this alternative approach for anyone searching for answers today.

Example:

try {
    // use APIClient for something
} catch (error: any) {
    console.log(error.response.json) // the API response
}

Improving this call like @jnordberg suggested is probably still the right path forward, but is not completed yet. I suspect that the inconsistencies in error responses from nodeos may cause that to be a rather lengthy undertaking to address from the client side.

aaroncox avatar Feb 14 '23 20:02 aaroncox