Error descriptions are not exposed on API level about indy-sdk HOT 11 CLOSED

Here is my current preference, after the discussion that's taken place in the thread so far:

1. We implement error codes that are granular enough so that when we predict they will require a different response, we return a different code. In other words, code that calls the API should be able to decide how to handle an error using only the code.
2. We associate each error code with a 1- or 2-sentence error message that never varies by context. If we are using the same error code with multiple messages, we need multiple error codes.
3. We use logging for programmers to learn about specific details/context for a given message. All logged messages should include a command_handle so we can associate caller context with callee context.
4. We make sure we expose an API so consumers of the lib can configure logging.

from indy-sdk.

I totally agree that developer needs some way for better understanding what is wrong. Current error codes design tries to achieve 2 different things:

1. Provide reasonable codes for validation/error cases that can be recovered by an application. It always well-detailed code. All these codes must be handled by application code and some recovery procedure must be executed. For example, if revocation registry is full libsovrin raises AnoncredsRevocationRegistryFullError and application should create new revocation registry. This error codes can be handled without additional message as recovery procedure is clear. Actually, assigned message is static.
2. Provide error codes that help developer avoid invalid library usage: pass invalid handle, invalid json, invalid crypto structure and etc... In the most cases, the application should just panic or ask a user to send an error report if this error code was received. Types that libsovrin use (especially Anoncreds) are very complex, validation can't be performed on one code level without breaking code encapsulation and it seems overkill to map each possible validation case to the specific error code. It would be really nice to have a message with details. This message is dynamic and will help a lot to application developer, libsovrin developers and customer support. Also livsovrin have a lot of internal integrity checking and libsovrin error will be also reported as error codes instead of panic.

We don't actualluy process user input. So error messages are intended only to developers and i18n ins't required.

We are now performing refactoring of validation logic and error codes. It will be finished soon:

1. Amount of error codes will be reduced. Similar error code will be grouped if the recovery procedure is the same.
2. Better messages for each error case with propogation history.
3. Avoid implicit error propogation based on low-level messages. Better analisys of each case to determine precondition or invariant case.
4. Error logging will be more flexible. Our plan is provide error logging for each internal recovery or error propogation case.

Actually for me it looks like this logging can solve this developers problem.

from indy-sdk.

There are two possible approaches: 1) make functions return both an error code and error text, or 2) leave functions returning just an error code, but provide an API that looks up the associated text for an error code.

Posix generally favors error codes without text, and this approach has the advantage that it doesn't create localization problems, so I prefer approach 2. What do others think?

from indy-sdk.

Well the error text can be very dynamic. E.g. in the above example, the API error code for InvalidStructure is always 110, but the text could be "Invalid data json: [--detailed JSON parsing error here--]" or "Either raw or hash or enc must be specified", etc.

So I think only 1) would work?

from indy-sdk.

To me, if the same error code can be associated with significantly different error messages, then something is wrong with the design of the source code. An error code should always mean the same thing. For example, "file not found" might be the message for an error code, and the specific file might vary, but I wouldn't want the same error code to also mean "file permission error". We need the ability to have a KB article about each error code, to be able to seek help with an error code on stack overflow, etc.

If there is consensus about what I'm asserting (and there might not be; I'm curious...), then the only potential variation between error messages for a given code would be the specific context. For example, a "JSON parsing error" with code 123 might sometimes be associated with a missing curly brace at position 100, and sometimes with a missing quote mark at position 1000, but it would always be a JSON parsing error.

If that's true, then I think the value of the specific context is debatable. It's nice to return highly specific error messages, and as a programmer I have appreciated them when APIs have provided them. However, I'm not sure it's worth a significant complication to the API to achieve this; most of the time, the programmer can infer or recreate the context once they know the general problem. If it's a "file not found" error, the programmer can figure out which file is missing, and if it's a JSON parsing error, the programmer can parse the JSON independently to discover what's wrong.

The reason I'm pushing back is that I don't think there's a convenient way to return both an error code and a message from every API. If you can think of a clean way to do so, that doesn't add a lot of complication for the caller and the callee, let's explore...

from indy-sdk.

I agree, if the error codes are specific/helpful enough, then this library shouldn't have to expose a detailed error text. Right now, some error codes seem to be pretty broad, e.g. consider some of the error texts associated with CryptoInvalidStructure:

mod.rs:
101: return Err(SignusError::CryptoError(CryptoError::InvalidStructure(format!("Verkey key not found")))); 
120: return Err(SignusError::CryptoError(CryptoError::InvalidStructure(format!("Signature key not found")))); 
130: return Err(SignusError::CryptoError(CryptoError::InvalidStructure(format!("Public key not found")))); 
issuer.rs:
76: return Err(CryptoError::InvalidStructure(format!("List of attribute names is required to setup claim definition"))) 
prover.rs:
130: ok_or(CryptoError::InvalidStructure("Field v_prime not found".to_string()))?.v_prime, 
132: .ok_or(CryptoError::InvalidStructure("Field pkr not found".to_string()))?, 
134: .ok_or(CryptoError::InvalidStructure("Field revoc_reg not found".to_string()))?.accumulator, 
165: return Err(CryptoError::InvalidStructure("issuer is sending incorrect data".to_string())); 
242: .ok_or(CryptoError::InvalidStructure(format!("Schema not found")))?; 
244: .ok_or(CryptoError::InvalidStructure(format!("Claim definition not found")))?; 
252: .ok_or(CryptoError::InvalidStructure(format!("Predicate not found")))?; 
264: .ok_or(CryptoError::InvalidStructure(format!("Attribute not found")))?; 
300: .ok_or(CryptoError::InvalidStructure(format!("Attributes for claim {} not found", claim_uuid)))? 
304: .ok_or(CryptoError::InvalidStructure(format!("Encoded value not found")))?; 
350: .ok_or(CryptoError::InvalidStructure("Revocation registry not found".to_string()))? 
353: .ok_or(CryptoError::InvalidStructure("Field public_key_revocation not found".to_string()))?, 
391: .ok_or(CryptoError::InvalidStructure(format!("Claim not found")))?; 
470: return Err(CryptoError::InvalidStructure("Can not update Witness. I'm revoced.".to_string())) 
484: .ok_or(CryptoError::InvalidStructure(format!("Key not found {} in tails", accum.max_claim_num + 1 - j + mut_claim.i)))?)?; 
491: .ok_or(CryptoError::InvalidStructure(format!("Key not found {} in tails", accum.max_claim_num + 1 - j + mut_claim.i)))?)?; 
555: .ok_or(CryptoError::InvalidStructure(format!("Value by key '{}' not found in c1.encoded_attributes", k)))? 
557: .ok_or(CryptoError::InvalidStructure(format!("Value not found in c1.encoded_attributes")))? 
563: return Err(CryptoError::InvalidStructure("Predicate is not satisfied".to_string())) 
574: .ok_or(CryptoError::InvalidStructure(format!("Value by key '{}' not found in u1", i)))?; 
617: .ok_or(CryptoError::InvalidStructure(format!("Value by key '{}' not found in eq_proof.mtilde", k)))?; 
656: .ok_or(CryptoError::InvalidStructure(format!("Encoded Value not found in init_prook.c1")))?; 
681: .ok_or(CryptoError::InvalidStructure(format!("Encoded value not found")))? 
704: .ok_or(CryptoError::InvalidStructure(format!("Value by key '{}' not found in init_proof.u_tilde", i)))?; 

One simple approach could also be to keep things the way they are, and developers could just turn on logging if they need to see the error text.

from indy-sdk.

To me, if the same error code can be associated with significantly different error messages, then something is wrong with the design of the source code. An error code should always mean the same thing. For example, "file not found" might be the message for an error code, and the specific file might vary, but I wouldn't want the same error code to also mean "file permission error".

I am not sure that it is good error codes design. "file not found" can be raised in totally different cases with different recovery procedure:

1. Example one. File name can be provided by application and it is precondition checking that can be recovered.
2. Example two. It can be raised by storage backend and it is invariant checking. Most probable in this case only application panic is possible.

Same with json. We need different error codes for a case when user passed invalid json and for a case when we can't parse value stored in the wallet.

So in error codes design we are more thinking about recovery procedure than propagation of low-level error by specific error code. But this error code invormation is important for debugging and it would be nice to have detailed error message.

from indy-sdk.

This plan looks ok for me too.

from indy-sdk.

The plan is good as long as the application using libindy is in development.
The developer and QA have access to the logs.

When the application is released and logging is turned off and the user calls the hotline then things get complicated/expensive and then a message like '"/sdcard/.indy_client/wallet" not found' might remind the user to insert the sdcard where the wallets are stored. (Assuming the mobile client to allow the user to choose where to store the wallets (sdcard, usb stick)

There are cases when providing context to the application helps the application to guide the user to do the right thing - which might also be telling him to contact the 3rd party (issuer, verifier, agent, ...) but not the application developer to fix the error.

(I am currently trying to get the unit tests to work on Android and there are a ton of IO_ERROR which do not really help without description)

from indy-sdk.

Has error_chain been discussed already?
https://docs.rs/error-chain/0.11.0/error_chain/

Using error_chain might spare us a lot of boilerplate error handling. I like the principles they adhere to especially: don't loose any information about the error.
The in the last moment when we translate the error to the ErrorCode we loose data but it looks like this would be an improvement to the current error handling implementation.

error-chain is used in Firefox core, too.
e.g. https://dxr.mozilla.org/mozilla-central/source/media/audioipc/server/src/lib.rs#42

What do you think?

from indy-sdk.

We are using Jira to track issues now, please use this Jira IS-997 ticket for any further discussion.

from indy-sdk.