View Issue Details

IDProjectCategoryView StatusLast Update
0006434Talerotherpublic2020-10-11 21:00
ReporterFlorian Dold Assigned To 
PrioritynormalSeveritytweakReproducibilityhave not tried
Status confirmedResolutionopen 
Product Versiongit (master) 
Target Version0.9 
Summary0006434: clean up error codes
DescriptionWe should make a pass over the error codes list before we release 0.9.

There are many ECs that are not used, and others that have a name that's too generic, when it is actually an error that is very specific to some component (such as sync).
TagsNo tags attached.

Activities

Christian Grothoff

2020-08-25 20:50

manager   ~0016770

We also should consistently use a TALER_EC_{COMPONENT}_{ENDPOINT} prefix where applicable (so for ECs >= 1000).

In some cases, we recycle non-generic ECs across components or endpoints, which should be avoided.

In some cases, we re-use an EC and give the "real" error in the "detail", which we should avoid (the detail should be to elaborate in a way that the EC cannot).

For all generic ECs, we should document what the 'detail' should specify (like the name of the field in the case of a parameter missing or being malformed).

We should decide about additional generic ECs to add. I propose: failure to hash a contract, unexpected serialization failure for single-statement transaction, failure to start database transaction, failure to commit database transaction (all without 'details', or if details are given in private endpoints, then with the Postgres message).

For malformed fields where it would make sense to provide the *value* of the field in the 'detail', I am not sure if we should have a generic EC, or always have a _specific_ EC that encodes the name of the field. (I tend to having another libtalermhd error reporting function, give that the field name and parameter value and report both strings with a generic EC. Florian/Jonathan: any thoughts?)

Christian Grothoff

2020-08-25 20:50

manager   ~0016771

Let's discuss this a bit.

Florian Dold

2020-08-27 10:20

manager   ~0016777

I think we should have generic error codes for request parameters that are missing or malformed. Encoding the field name in the EC is IMHO counter-productive. It doesn't help clients with error handling, it doesn't help generate better error messages (to the contrary!).

For specific *semantic* checks of request parameters (or payloads), we should have specific error codes (we mostly already do this).

IMHO we should also get rid of the extra optional fields (which we mostly don't use anyway!) in the ErrorDetail schema (https://docs.taler.net/core/api-common.html#tsref-type-ErrorDetail).

The generic error response would be

interface ErrorResponseBody {
  code: number;
  hint: string; // hint must include the alphanumeric code name, but can contain more prose, too
  details?: any;
}

And for specific error codes, we can further refine the details with a more precise type. Why have the details in a separate fields and not at the top level? Because it makes types (both in the specification and in implementations in languages that have more expressive type systems) clearer. I can have a generic DatabaseErrorDetails or HttpRequestErrorDetail type and re-use them or even combine them.

Regarding generic errors: The one you propose make sense. I would also keep "INTERNAL_INVARIANT_FAILURE" (duplicate of INTERNAL_LOGIC_ERROR), "ENDPOINT_UNKNOWN", "NOT_IMPLEMENTED". JSON_INVALID should probably become REQUEST_JSON_INVALID, because there are other cases where JSON can be invalid.

The wallet has WALLET_UNEXPECTED_EXCEPTION, which we could change to a generic UNEXPECTED_EXCEPTION, which is similar to INTERNAL_INVARIANT_FAILURE but comes with a stack trace.

The current generic error codes CLIENT_INTERNAL_FAILURE and INVALID should be removed or at least be redefined to something more concrete.

Florian Dold

2020-08-27 10:38

manager   ~0016778

Also, the wallet right now uses separate fields for the alphanumeric name of the error code and the "message" part. So the "hint" field is specific to the *type* of the error, and the "message" is specific to the *instance* of the error.

We should converge on one approach for both the wallet error messages and the other components. I.e., do we want this:

{
  code: 7001,
  hint: "Error: WALLET_UNEXPECTED_EXCEPTION",
  message: "unexpected exception (BUG: transaction closed before transaction function returned)"
}

or do we want

{
  code: 7001,
  hint: "WALLET_UNEXPECTED_EXCEPTION: unexpected exception (BUG: transaction closed before transaction function returned)"
}

or some variation thereof?

IMHO it's very important to have the alphanumeric error code in the JSON, but it is often awkward having to combine it with an "error instance"-specific message.

Christian Grothoff

2020-08-27 11:47

manager   ~0016779

I'm for "hint: WALLET_UNEXPECTED_FOO: text from the comment we wrote on the enum value of WALLET_UNEXPECTED_FOO". Plus "details".

Florian Dold

2020-08-27 11:51

manager   ~0016780

That works for me, as long as all UIs actually render the details as pretty-printed JSON when no specific error handling is implemented for that particular error code.

Otherwise, in the example from above, the "BUG: transaction closed before transaction function returned" (which is the message from the actual JavaScript exception) would be hidden.

Christian Grothoff

2020-08-27 11:54

manager   ~0016781

Agreed. I expect that 'details' will often be a string, and in cases where not I think any reasonable auto-rendering of the JSON will be good
(like if it is a simple object, we might show the key:value pairs as a table, and an array as comma-separated values).

Florian Dold

2020-08-27 12:03

manager   ~0016782

The "details" should never be a plain string, as this would mean you can't add additional diagnostics information to an existing error code without breaking the schema in a backwards-incompatible way.

We should either:
(a) require the details to be a subtype of { message?: string } (i.e. every concrete details object *can* contain a message but doesn't have to, and *if* the message field is present it must be a string)
(b) lift the optional message field to the top-level of the error JSON

I'd be fine with both. The wallet currently does (b) but it's not hard to switch to (a).

Christian Grothoff

2020-08-27 12:14

manager   ~0016783

I think having a 'message' string is very common, while having other details is rare. Let's go with:

{
  code: 7001,
  hint: "WALLET_UNEXPECTED_EXCEPTION: unexpected exception",
  message: "BUG: transaction closed before transaction function returned)", // optional
  details: any, // optional
}

Issue History

Date Modified Username Field Change
2020-07-21 09:24 Florian Dold New Issue
2020-07-21 23:25 Christian Grothoff Severity minor => tweak
2020-07-21 23:25 Christian Grothoff Status new => confirmed
2020-07-24 12:02 Christian Grothoff Product Version => git (master)
2020-07-24 12:02 Christian Grothoff Target Version 0.8 => 0.8.1
2020-08-25 20:50 Christian Grothoff Note Added: 0016770
2020-08-25 20:50 Christian Grothoff Assigned To => Florian Dold
2020-08-25 20:50 Christian Grothoff Status confirmed => feedback
2020-08-25 20:50 Christian Grothoff Note Added: 0016771
2020-08-27 10:20 Florian Dold Note Added: 0016777
2020-08-27 10:21 Florian Dold Assigned To Florian Dold => Christian Grothoff
2020-08-27 10:38 Florian Dold Note Added: 0016778
2020-08-27 10:38 Florian Dold Status feedback => assigned
2020-08-27 11:47 Christian Grothoff Note Added: 0016779
2020-08-27 11:51 Florian Dold Note Added: 0016780
2020-08-27 11:54 Christian Grothoff Note Added: 0016781
2020-08-27 12:03 Florian Dold Note Added: 0016782
2020-08-27 12:14 Christian Grothoff Note Added: 0016783
2020-08-28 00:24 Christian Grothoff Assigned To Christian Grothoff => jonathanbuchanan
2020-10-11 20:59 Christian Grothoff Assigned To jonathanbuchanan => Christian Grothoff
2020-10-11 20:59 Christian Grothoff Target Version 0.8.1 => 0.9
2020-10-11 20:59 Christian Grothoff Description Updated View Revisions
2020-10-11 21:00 Christian Grothoff Assigned To Christian Grothoff =>
2020-10-11 21:00 Christian Grothoff Status assigned => confirmed