-
Notifications
You must be signed in to change notification settings - Fork 23
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
Changes from 4 commits
8eacefc
d1d6d0c
61a4e73
c27cb4b
8360fbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using existential operators ( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow, can't you just expand |
||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
} else { | ||
return 'Unknown API error' | ||
|
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' | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () { | ||
|
@@ -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 () { | ||
|
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\"}]}}" | ||
} |
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\"}]}}" | ||
} |
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\"}}" | ||
} |
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}" | ||
} |
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\"}]}}" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"status": 500, | ||
"json": { | ||
"code": 500 | ||
} | ||
} |
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}}" | ||
} |
There was a problem hiding this comment.
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