Skip to content

Commit 81ef7eb

Browse files
thejcfactorthejcfactor
committed
JSCBC-1348: Transactions - Provide error message where possible
Changes ======= * Use error message from binding exceptions where possible to provide further context on the error. * Remove potential throwing of error from transactional binding helpers as it can cause undesired behavior. * Update tests to confirm functionality. Change-Id: Ic3b2454cdb85bdb01524dfec8982a69db2cea61d Reviewed-on: https://review.couchbase.org/c/couchnode/+/227597 Reviewed-by: Mateusz <[email protected]> Tested-by: Jared Casey <[email protected]>
1 parent c64b62e commit 81ef7eb

File tree

5 files changed

+44
-27
lines changed

5 files changed

+44
-27
lines changed

‎lib/binding.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3426,19 +3426,22 @@ export interface CppTxnOperationFailed extends CppErrorBase {
34263426
should_not_retry: boolean
34273427
should_not_rollback: boolean
34283428
cause: CppTxnExternalException
3429+
message?: string
34293430
}
34303431

34313432
export interface CppTxnOpException extends CppErrorBase {
34323433
ctxtype: 'transaction_op_exception'
34333434
ctx: CppTransactionOpErrorContext | undefined
34343435
cause: CppTxnExternalException
3436+
message?: string
34353437
}
34363438

34373439
export interface CppTxnError extends CppErrorBase {
34383440
ctxtype: 'transaction_exception'
34393441
result: CppTransactionResult
34403442
cause: CppTxnExternalException
34413443
type: CppTxnFailureType
3444+
message?: string
34423445
}
34433446

34443447
export interface CppTransactionErrorContext {

‎lib/bindingutilities.ts‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -482,9 +482,13 @@ export function endpointStateFromCpp(
482482
* @internal
483483
*/
484484
export function txnExternalExceptionStringFromCpp(
485-
cause: CppTxnExternalException
485+
cause: CppTxnExternalException,
486+
message?: string
486487
): string {
487488
if (cause === binding.txn_external_exception.unknown) {
489+
if (message) {
490+
return message
491+
}
488492
return 'unknown'
489493
} else if (
490494
cause ===
@@ -563,7 +567,7 @@ export function txnExternalExceptionStringFromCpp(
563567
return 'transaction_already_committed'
564568
}
565569

566-
throw new errs.InvalidArgumentError()
570+
return 'unknown'
567571
}
568572

569573
/**
@@ -580,23 +584,23 @@ export function txnOpExeptionFromCpp(
580584
const context = ctx ? ctx : undefined
581585
if (err.cause === binding.txn_external_exception.document_exists_exception) {
582586
return new errs.DocumentExistsError(
583-
new Error(txnExternalExceptionStringFromCpp(err.cause)),
587+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message)),
584588
context
585589
)
586590
} else if (
587591
err.cause === binding.txn_external_exception.document_not_found_exception
588592
) {
589593
return new errs.DocumentNotFoundError(
590-
new Error(txnExternalExceptionStringFromCpp(err.cause)),
594+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message)),
591595
context
592596
)
593597
} else if (err.cause === binding.txn_external_exception.parsing_failure) {
594598
return new errs.ParsingFailureError(
595-
new Error(txnExternalExceptionStringFromCpp(err.cause)),
599+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message)),
596600
context
597601
)
598602
} else if (err.cause === binding.txn_external_exception.couchbase_exception) {
599-
const cause = txnExternalExceptionStringFromCpp(err.cause)
603+
const cause = txnExternalExceptionStringFromCpp(err.cause, err.message)
600604
return new errs.CouchbaseError(cause, new Error(cause), context)
601605
}
602606

@@ -693,7 +697,7 @@ export function errorFromCpp(err: CppError | null): Error | null {
693697

694698
// BUG(JSCBC-1010): We shouldn't need to special case these.
695699
if (err.ctxtype === 'transaction_operation_failed') {
696-
const cause = txnExternalExceptionStringFromCpp(err.cause)
700+
const cause = txnExternalExceptionStringFromCpp(err.cause, err.message)
697701
if (cause == 'feature_not_available_exception') {
698702
const msg =
699703
'Possibly attempting a binary transaction operation with a server version < 7.6.2'
@@ -702,7 +706,7 @@ export function errorFromCpp(err: CppError | null): Error | null {
702706
)
703707
}
704708
return new errs.TransactionOperationFailedError(
705-
new Error(txnExternalExceptionStringFromCpp(err.cause))
709+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message))
706710
)
707711
} else if (err.ctxtype === 'transaction_op_exception') {
708712
let txnContext: ErrorContext | null = null
@@ -713,19 +717,21 @@ export function errorFromCpp(err: CppError | null): Error | null {
713717
} else if (err.ctxtype === 'transaction_exception') {
714718
if (err.type === binding.txn_failure_type.fail) {
715719
return new errs.TransactionFailedError(
716-
new Error(txnExternalExceptionStringFromCpp(err.cause))
720+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message))
717721
)
718722
} else if (err.type === binding.txn_failure_type.expiry) {
719723
return new errs.TransactionExpiredError(
720-
new Error(txnExternalExceptionStringFromCpp(err.cause))
724+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message))
721725
)
722726
} else if (err.type === binding.txn_failure_type.commit_ambiguous) {
723727
return new errs.TransactionCommitAmbiguousError(
724-
new Error(txnExternalExceptionStringFromCpp(err.cause))
728+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message))
725729
)
726730
}
727731

728-
throw new errs.InvalidArgumentError()
732+
return new errs.TransactionFailedError(
733+
new Error(txnExternalExceptionStringFromCpp(err.cause, err.message))
734+
)
729735
}
730736

731737
const baseErr = err as any as Error

‎lib/transactions.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,6 +782,8 @@ export class Transactions {
782782
await txn._rollback()
783783
if (e instanceof TransactionOperationFailedError) {
784784
throw new TransactionFailedError(e.cause, e.context)
785+
} else if (e instanceof TransactionFailedError) {
786+
throw e
785787
}
786788
throw new TransactionFailedError(e as Error)
787789
}

‎src/jstocbpp_transactions.hpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@ struct js_to_cbpp_t<cbcoretxns::transaction_operation_failed> {
374374
jsErr.Set("should_not_rollback",
375375
cbpp_to_js(env, !err.should_rollback()));
376376
jsErr.Set("cause", cbpp_to_js(env, err.cause()));
377+
jsErr.Set("message", cbpp_to_js(env, std::string(err.what())));
377378
return jsErr.Value();
378379
}
379380
};
@@ -436,16 +437,17 @@ struct js_to_cbpp_t<cbcoretxns::op_exception> {
436437
template <>
437438
struct js_to_cbpp_t<couchbase::core::transaction_op_error_context> {
438439
static inline Napi::Value
439-
to_js(Napi::Env env, const couchbase::core::transaction_op_error_context &res)
440+
to_js(Napi::Env env,
441+
const couchbase::core::transaction_op_error_context &res)
440442
{
441443
auto resObj = Napi::Object::New(env);
442444
resObj.Set("code", cbpp_to_js(env, res.ec()));
443445
resObj.Set(
444446
"cause",
445-
cbpp_to_js<
446-
std::variant<std::monostate, couchbase::core::key_value_error_context,
447-
couchbase::core::query_error_context>>(env,
448-
res.cause()));
447+
cbpp_to_js<std::variant<std::monostate,
448+
couchbase::core::key_value_error_context,
449+
couchbase::core::query_error_context>>(
450+
env, res.cause()));
449451
return resObj;
450452
}
451453
};

‎test/transactions.test.js‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ describe('#transactions', function () {
1111
H.skipIfMissingFeature(this, H.Features.Transactions)
1212
})
1313

14-
after(async function() {
14+
after(async function () {
1515
this.timeout(10000)
1616
const bmgr = H.c.buckets()
1717
await H.tryNTimes(3, 1000, bmgr.flushBucket.bind(bmgr), H.bucketName)
@@ -288,22 +288,24 @@ describe('#transactions', function () {
288288
await H.c.transactions().run(
289289
async (attempt) => {
290290
numAttempts++
291-
const remDoc = await attempt.get(H.co, testDocId)
292-
await attempt.replace(remDoc, { foo: 'baz' })
291+
const repDoc = await attempt.get(H.co, testDocId)
292+
await attempt.replace(repDoc, { foo: 'baz' })
293293
// This should fail due to CAS Mismatch
294-
// Note that atm the cause is set as unknown in the txn lib
295294
try {
296-
await attempt.replace(remDoc, { foo: 'qux' })
295+
await attempt.replace(repDoc, { foo: 'qux' })
297296
} catch (err) {
298297
assert.instanceOf(err, H.lib.TransactionOperationFailedError)
299-
assert.equal(err.cause.message, 'unknown')
298+
assert.isTrue(
299+
err.cause.message.includes('transaction expired') ||
300+
err.cause.message.includes('cas_mismatch')
301+
)
300302
}
301303
},
302304
{ timeout: 2000 }
303305
)
304306
} catch (err) {
305307
assert.instanceOf(err, H.lib.TransactionFailedError)
306-
assert.equal(err.cause.message, 'unknown')
308+
assert.isTrue(err.cause.message.includes('transaction expired'))
307309
}
308310
assert.isTrue(numAttempts > 1)
309311

@@ -326,19 +328,21 @@ describe('#transactions', function () {
326328
const remDoc = await attempt.get(H.co, testDocId)
327329
await attempt.replace(remDoc, { foo: 'baz' })
328330
// This should fail due to CAS Mismatch
329-
// Note that atm the cause is set as unknown in the txn lib
330331
try {
331332
await attempt.remove(remDoc)
332333
} catch (err) {
333334
assert.instanceOf(err, H.lib.TransactionOperationFailedError)
334-
assert.equal(err.cause.message, 'unknown')
335+
assert.isTrue(
336+
err.cause.message.includes('transaction expired') ||
337+
err.cause.message.includes('cas_mismatch')
338+
)
335339
}
336340
},
337341
{ timeout: 2000 }
338342
)
339343
} catch (err) {
340344
assert.instanceOf(err, H.lib.TransactionFailedError)
341-
assert.equal(err.cause.message, 'unknown')
345+
assert.isTrue(err.cause.message.includes('transaction expired'))
342346
}
343347
assert.isTrue(numAttempts > 1)
344348

0 commit comments

Comments
 (0)