Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing error details on API error #32

Open
theblockstalk opened this issue Aug 16, 2022 · 8 comments
Open

Missing error details on API error #32

theblockstalk opened this issue Aug 16, 2022 · 8 comments
Milestone

Comments

@theblockstalk
Copy link
Contributor

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
Copy link
Contributor Author

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.

@aaroncox
Copy link
Member

aaroncox commented Sep 5, 2022

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?

@jnordberg
Copy link
Collaborator

jnordberg commented Oct 19, 2022

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

@ericpassmore
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@aaroncox
Copy link
Member

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 aaroncox added this to the Unscheduled milestone Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants