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

Improved APIError Message #38

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/IDE_Debug.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ In the top Right Corner, select the checkbox `Store as Project File`. To save co

![Webstorm IDE Configuration](Webstorm2022-Run-EditConfiguration.png)

Optionally you may add `NODE_DEBUG=HTTP` as an environment variable to see http requests.
- :heavy_exclamation_mark: This won't show mocked requests.
- :blue_book: Separate multiple environment variables with semi-colon `;`

## Visual Studio

### <font color="red">Need help updating and verifying this section.</font>
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
"@types/node": "^18.6.5",
"@typescript-eslint/eslint-plugin": "^5.4.0",
"@typescript-eslint/parser": "^5.4.0",
"chai": "^4.3.4",
"chai": "^4.3.6",
"chai-as-promised": "^7.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try not to introduce new dependencies, chai is only used as an assert polyfill for browser tests and we don't use "expect" style testing

"eslint": "^8.26.0",
"eslint-config-prettier": "^8.1.0",
"eslint-plugin-es-x": "^5.3.1",
Expand Down
27 changes: 21 additions & 6 deletions src/api/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,31 @@ export class APIError extends Error {

static formatError(error: APIErrorData) {
if (
error.what === 'unspecified' &&
error.details[0].file &&
error.details[0].file === 'http_plugin.cpp' &&
error.details[0].message.slice(0, 11) === 'unknown key'
error?.what === 'unspecified' &&
error?.details?.[0]?.file === 'http_plugin.cpp' &&
error?.details?.[0]?.message?.slice(0, 11) === 'unknown key'
) {
// fix cryptic error messages from nodeos for missing accounts
return 'Account not found'
} else if (error.what === 'unspecified' && error.details && error.details.length > 0) {
} else if (error?.what === 'unspecified' && error?.details?.[0]) {
return error.details[0].message
} else if (error.what && error.what.length > 0) {
} else if (error?.what?.length > 0 && error?.details?.[0]?.message?.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using existential operators (?), we have had issues with downstream libraries and applications not being able to import our library due to those and had to scrub them from the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, do you have a suggestion on how to proceed? Optional values is the root issue because, detecting undefined | null | '' strings forced the negative tests for code coverage. If the code was restructured to never allow falsy values the optionals along with the negative tests could go away. No negative tests, no need for chai.except. Happy to discuss if you want to run through the options. If you know what you want just let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow, can't you just expand ? by doing (error != undefined && error.what != undefined) etc?

// return detailed messages
// dedupe text between message and details
const canonical_what_message: string = error.what.replace(':;', '').trim()
// intentionally check first message
// code not good enough to dedupe across entire message chain
const message_from_all_details: string = error.details
.map(({message}) => message)
.join(' , ')
if (error.details[0].message.includes(canonical_what_message)) {
return message_from_all_details
} else {
return ''.concat(canonical_what_message, ' : ', message_from_all_details)
}
} else if (error?.what?.length > 0) {
// lacks detailed message, fallback to error statement
// note this isn't very helpful to developers
return error.what
Copy link
Collaborator

@jnordberg jnordberg Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case that hits this would be good to ensure that the above if statement doesn't cause issues (make coverage will generate and open a test coverage report for you)

} else {
return 'Unknown API error'
Expand Down
119 changes: 114 additions & 5 deletions test/api.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import {assert} from 'chai'
import * as chai from 'chai'
// needed for assert.isRejected test
import chaiAsPromised from 'chai-as-promised'
chai.use(chaiAsPromised)
const assert = chai.assert
const expect = chai.expect

import {MockProvider} from './utils/mock-provider'
import {makeMockTransaction, signMockTransaction} from './utils/mock-transfer'
Expand Down Expand Up @@ -91,6 +96,12 @@ suite('api v1', function () {
)
})

test('jungle get_block (by id)', async function () {
const good_block_id = '02815e2f90ecf10acfb9dc7f2aa301a5a008b47c3d7a2743f9dd8dffcfa3762e'
const block_info = await jungle4.v1.chain.get_block(good_block_id)
assert.equal(Number(block_info.block_num), 42032687)
})

test('chain get_block (by num)', async function () {
const block = await eos.v1.chain.get_block(8482113)
assert.equal(Number(block.block_num), 8482113)
Expand Down Expand Up @@ -414,6 +425,98 @@ suite('api v1', function () {
}
})

test('api error with details', async function () {
// test improved, longer error message with failed permission
@Struct.type('transfer')
class Transfer extends Struct {
@Struct.field('name') from!: Name
@Struct.field('name') to!: Name
@Struct.field('asset') quantity!: Asset
@Struct.field('string') memo!: string
}

const info = await jungle.v1.chain.get_info()
const header = info.getTransactionHeader()
const action = Action.from({
authorization: [
{
actor: 'corecorecore',
permission: 'badperm',
},
],
account: 'eosio.token',
name: 'transfer',
data: Transfer.from({
from: 'corecorecore',
to: 'teamgreymass',
quantity: '0.0042 EOS',
memo: 'expected to fail',
}),
})
const transaction = Transaction.from({
...header,
actions: [action],
})
const privateKey = PrivateKey.from(
'5JW71y3njNNVf9fiGaufq8Up5XiGk68jZ5tYhKpy69yyU9cr7n9'
)
const signature = privateKey.signDigest(transaction.signingDigest(info.chain_id))
const signedTransaction = SignedTransaction.from({
...transaction,
signatures: [signature],
})
await expect(jungle.v1.chain.push_transaction(signedTransaction))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.have.property(
'message',
'Transaction exception : action\'s authorizations include a non-existent permission: {"actor":"corecorecore","permission":"foobar"} at /v1/chain/push_transaction'
)
.with.lengthOf.above(30)
})

// check an error with multiple error details returned
test('api error multi message', async function () {
await expect(jungle4.v1.chain.get_block('A1234'))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.have.property(
'message',
'Invalid block ID: A1234 , str.size() % 2 == 0: the length of hex string should be even number at /v1/chain/get_block'
)
})

// checks error message code is robust and can handle missing elements
test('api error sparse message', async function () {
// code only, no error.what , no error.details[].message
await expect(jungle4.v1.chain.get_block('AnswerCodeOnly'))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.has.property('code')
.and.does.not.have.property('message')
})

test('api error unspecified', async function () {
// code exists, error.what == unspecified, error.details[].message exists
await expect(jungle4.v1.chain.get_block('AnswerUnspecified'))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.has.property('code')
.and.does.not.have.property('message')
})

test('api error has what no details.message', async function () {
// code exists, error.what exists, no error.details[].message
await expect(jungle4.v1.chain.get_block('AnswerWhatOnly'))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.has.property('code')
.and.does.not.have.property('message')
})

test('api error no message error code only', async function () {
// only 500 code, no other details
await expect(jungle4.v1.chain.get_block('AnswerWithNothing'))
.to.eventually.be.rejected.and.be.an.instanceOf(APIError)
.and.has.property('message')
.and.does.not.have.property('name')
})

test('history get_actions', async function () {
const res = await eos.v1.history.get_actions('teamgreymass', 1, 1)
assert.equal(res.actions.length, 1)
Expand All @@ -424,8 +527,11 @@ suite('api v1', function () {
'03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7'
)
assert(Array.isArray(res.traces), 'response should have traces')
assert.equal(res.id, '03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7')
assert.equal(res.block_num, 199068081)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to look at why these started to fail. fixes get tests to pass.

assert.equal(
res.id.hexString,
'03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7'
)
assert.equal(Number(res.block_num), 199068081)
})

test('history get_transaction (no traces)', async function () {
Expand All @@ -434,8 +540,11 @@ suite('api v1', function () {
{excludeTraces: true}
)
assert.equal(res.traces, null, 'response should not have traces')
assert.equal(res.id, '03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7')
assert.equal(res.block_num, 199068081)
assert.equal(
res.id.hexString,
'03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7'
)
assert.equal(Number(res.block_num), 199068081)
})

test('history get_key_accounts', async function () {
Expand Down
21 changes: 21 additions & 0 deletions test/data/0b0762cc364146dc1750335b539c6715e9320563.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"status": 500,
"json": {
"code": 500,
"message": "Internal Service Error",
"error": {
"code": 3010008,
"name": "block_id_type_exception",
"what": "unspecified",
"details": [
{
"message": "Invalid block ID: A1234",
"file": "chain_plugin.cpp",
"line_number": 2096,
"method": "get_block"
}
]
}
},
"text": "{\"code\":500,\"message\":\"Internal Service Error\",\"error\":{\"code\":3010008,\"name\":\"block_id_type_exception\",\"what\":\"unspecified\",\"details\":[{\"message\":\"Invalid block ID: A1234\",\"file\":\"chain_plugin.cpp\",\"line_number\":2096,\"method\":\"get_block\"}]}}"
}
36 changes: 36 additions & 0 deletions test/data/4ddac1727a94acf2dfda68dea84d9444fc102c1d.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"headers": {
"access-control-allow-headers": "Origin, X-Requested-With, Content-Type, Accept",
"access-control-allow-origin": "*",
"connection": "close",
"content-length": "392",
"content-type": "application/json",
"date": "Tue, 25 Oct 2022 14:25:14 GMT",
"server": "nginx/1.18.0 (Ubuntu)"
},
"status": 500,
"json": {
"code": 500,
"message": "Internal Service Error",
"error": {
"code": 3010008,
"name": "block_id_type_exception",
"what": "Invalid block ID",
"details": [
{
"message": "Invalid block ID: A1234",
"file": "chain_plugin.cpp",
"line_number": 2096,
"method": "get_block"
},
{
"message": "str.size() % 2 == 0: the length of hex string should be even number",
"file": "variant.cpp",
"line_number": 740,
"method": "from_variant"
}
]
}
},
"text": "{\"code\":500,\"message\":\"Internal Service Error\",\"error\":{\"code\":3010008,\"name\":\"block_id_type_exception\",\"what\":\"Invalid block ID\",\"details\":[{\"message\":\"Invalid block ID: A1234\",\"file\":\"chain_plugin.cpp\",\"line_number\":2096,\"method\":\"get_block\"},{\"message\":\"str.size() % 2 == 0: the length of hex string should be even number\",\"file\":\"variant.cpp\",\"line_number\":740,\"method\":\"from_variant\"}]}}"
}
13 changes: 13 additions & 0 deletions test/data/6ba34a56eafd63c96b8f198f67e13af54724ff75.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"status": 500,
"json": {
"code": 500,
"message": "Internal Service Error",
"error": {
"code": 3010008,
"name": "block_id_type_exception",
"what": "random message for testing"
}
},
"text": "{\"code\":500,\"message\":\"Internal Service Error\",\"error\":{\"code\":3010008,\"name\":\"block_id_type_exception\",\"what\":\"random message for testing\"}}"
}
28 changes: 28 additions & 0 deletions test/data/759481b5cddc96fc790f6449ee4c2e82770d8035.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"headers": {
"access-control-allow-headers": "Origin, X-Requested-With, Content-Type, Accept",
"access-control-allow-origin": "*",
"connection": "close",
"content-length": "635",
"content-type": "application/json",
"date": "Tue, 25 Oct 2022 19:44:02 GMT",
"server": "nginx/1.18.0 (Ubuntu)"
},
"status": 200,
"json": {
"timestamp": "2022-10-25T19:36:02.500",
"producer": "batinthedark",
"confirmed": 0,
"previous": "02815e2ea45b167ea012ad16e8bfa6c055fda8eb5fc88a864cf8dbe10676fc2b",
"transaction_mroot": "0000000000000000000000000000000000000000000000000000000000000000",
"action_mroot": "a0d2d67035c34a5ec2b058ac07b8d57dd2036330262aba04ed67435b5ce89c7e",
"schedule_version": 50,
"new_producers": null,
"producer_signature": "SIG_K1_K2fhzvVFx7DUtkjhdJoaefmR7mhkq1HaPdCaPVYzC8xvkSJ8QEYpaYxKLD99AHWJWL1QM6vbFsQvjSHjxLMRcQBnofNeu6",
"transactions": [],
"id": "02815e2f90ecf10acfb9dc7f2aa301a5a008b47c3d7a2743f9dd8dffcfa3762e",
"block_num": 42032687,
"ref_block_prefix": 2145171919
},
"text": "{\"timestamp\":\"2022-10-25T19:36:02.500\",\"producer\":\"batinthedark\",\"confirmed\":0,\"previous\":\"02815e2ea45b167ea012ad16e8bfa6c055fda8eb5fc88a864cf8dbe10676fc2b\",\"transaction_mroot\":\"0000000000000000000000000000000000000000000000000000000000000000\",\"action_mroot\":\"a0d2d67035c34a5ec2b058ac07b8d57dd2036330262aba04ed67435b5ce89c7e\",\"schedule_version\":50,\"new_producers\":null,\"producer_signature\":\"SIG_K1_K2fhzvVFx7DUtkjhdJoaefmR7mhkq1HaPdCaPVYzC8xvkSJ8QEYpaYxKLD99AHWJWL1QM6vbFsQvjSHjxLMRcQBnofNeu6\",\"transactions\":[],\"id\":\"02815e2f90ecf10acfb9dc7f2aa301a5a008b47c3d7a2743f9dd8dffcfa3762e\",\"block_num\":42032687,\"ref_block_prefix\":2145171919}"
}
30 changes: 30 additions & 0 deletions test/data/840bd226f9d9ccdb3fdc42d17f563fff16893100.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"headers": {
"access-control-allow-headers": "Origin, X-Requested-With, Content-Type, Accept",
"access-control-allow-origin": "*",
"connection": "close",
"content-length": "365",
"content-type": "application/json",
"date": "Mon, 24 Oct 2022 21:01:20 GMT",
"server": "nginx/1.18.0 (Ubuntu)"
},
"status": 500,
"json": {
"code": 500,
"message": "Internal Service Error",
"error": {
"code": 3040000,
"name": "transaction_exception",
"what": "Transaction exception",
"details": [
{
"message": "action's authorizations include a non-existent permission: {\"actor\":\"corecorecore\",\"permission\":\"foobar\"}",
"file": "transaction_context.cpp",
"line_number": 769,
"method": "validate_referenced_accounts"
}
]
}
},
"text": "{\"code\":500,\"message\":\"Internal Service Error\",\"error\":{\"code\":3040000,\"name\":\"transaction_exception\",\"what\":\"Transaction exception\",\"details\":[{\"message\":\"action's authorizations include a non-existent permission: {\\\"actor\\\":\\\"corecorecore\\\",\\\"permission\\\":\\\"foobar\\\"}\",\"file\":\"transaction_context.cpp\",\"line_number\":769,\"method\":\"validate_referenced_accounts\"}]}}"
}
6 changes: 6 additions & 0 deletions test/data/a0a2eccf19a5ba65d934eab833a116b58fd81b1d.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"status": 500,
"json": {
"code": 500
}
}
11 changes: 11 additions & 0 deletions test/data/cf1e31d7228e6367559279548a86efa84569bb25.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"status": 500,
"json": {
"code": 500,
"message": "Internal Service Error",
"error": {
"code": 3010008
}
},
"text": "{\"code\":500,\"message\":\"Internal Service Error\",\"error\":{\"code\":3010008}}"
}