Error handling improvements

[adobeDan]

Changes to fix #40 - clean up error handling.

The basic idea is that errors have both a platform-independent code (to make it easier for clients to figure out what's happening) and the underlying platform error that occurred (so the client can dig into it if desired).

Right now the error codes are being determined by where in the fetch/set process the error occurs: opening the credential store, searching for the credential, reading/writing the credential, etc. This is marked draft because I am still adding detailed underlying error analysis of the platform errors to make sure the coding is correct. But it works on all three platforms so I welcome review.

NOTE: These are based on the platform reformulation done in #67, which has not been merged and introduces some minor breaking API changes. If you have issues with that platform reformulation, please make those suggestions in #67. The changes here could conceivably be back-ported to the existing release with a fully backward-compatible API, but I need to discuss with the other maintainers whether that's worth it or not.

[adobeDan]

I believe this PR also fixes #51

[phillipCouto]

Overall the code looks good, one comment with the struct type for error handling is it does make error handling a little less convenient at least with my current knowledge of rust. I may not be aware of rust syntax that can make this easier but I had to change the following code in our application from:

let payload = match ring.get_password() {
  Ok(s) => s,
  Err(KeyringError::NoPasswordFound) => return Ok(None),
  Err(e) => return Err(Box::new(e)),
};

to:

let payload = match ring.get_password() {
  Ok(s) => s,
  Err(e) => {
    if let keyring::ErrorCode::NoEntry = e.code {
      return Ok(None);
    }
    return Err(Box::new(e));
   }
};

For me this isn't that much of a pain but there may be others who are checking for more of the keyring error codes that may find this a negative change.

[adobeDan]

@phillipCouto great review, thanks. WRT to the question about client code changing, I believe you can use a destructuring pattern to minimize your code changes like this:

let payload = match ring.get_password() {
  Ok(s) => s,
  Err(keyring::Error { code: keyring::ErrorCode::NoEntry, .. })  => return Ok(None),
  Err(e) => return Err(Box::new(e)),
};

I also introduced type conversions between Errors and ErrorCodes, so you can easily extract the error code using .into(), but that won't work in the pattern matching environment.

[adobeDan]

Hi @hwchen and @phillipCouto, I believe all that's left to do now is more tests and documentation, so please give this a thorough review, especially as to issues around naming and structuring.

One serious question I have, which was raised indirectly by Phillip's comment above, is whether we want to let the error object be just an enum of codes (called "KeyringError" in the old code, and "ErrorCode" in the new code) rather than a structure. We can always embed the platform error in the enum codes that need them, and they can even be optional. That would really keep the API much more similar in shape to what it used to be (although I think we should call it just "Error" not "KeyringError").

[brotskydotcom]

Hi @hwchen and @phillipCouto, this PR is now complete and ready to merge. I'm going to do some code coverage analysis on the tests while you give it a final review.

[brotskydotcom]

This PR:

• fixes Add testing of README.md examples? #28 by adding a doc test for the README code.
• fixes Incompatible with 3rd party (native) credential writers who use their own Target and User Name conventions #60 by allowing for custom maps and fetching credential attributes.
• fixes No backend error for Linux? #54 by extending error codes across platforms.
• fixes Unreliable behavior of delete_password() across operating systems #39 by extending error codes across platforms.
• fixes KeyringError::NoPasswordFound is not used on MacOS #65 by extending error codes across platforms.
• fixes Add ability to access named keychains in macOS. #44 by adding keychain names on Mac and Linux.
• fixes MacOsKeychainError is not documented as far as I can tell #24 by extending error codes across platforms.

[hwchen]

I'll be taking a look at this this weekend. Just wanted to pop up and say I didn't disappear, but just had a tough week.

[brotskydotcom]

I'll be taking a look at this this weekend. Just wanted to pop up and say I didn't disappear, but just had a tough week.

No worries! I completely understand.

Also, I realize I want to revamp the examples to show the new capabilities and error handling, so let's not merge quite yet. I'm going to put this PR back in draft to indicate that.

[hwchen]

I think it's fine to have the Error be an enum. Now that platform errors have been consolidated, it looks clean and an enum is probably a bit more straightforward to use.

One general comment: I'm actually not sure why I originally treated passwords as utf8 strings. Perhaps it's better to treat passwords as bytes and leave encoding to the user.

Or perhaps have a Password type, so users could choose as_bytes() or to_string() (and we could also impl From both both strings and bytes). With this approach, users wouldn't have to find String::from_utf8 themselves, and keyring would also be able to remove its BadEncoding error variant (since we'd just defer to the regular Utf8Error from to_string()).

If we want to remove String, we can do it in another PR before releasing. Just wanted to mention it here because I was thinking about the BadEncoding variant.

More breaking changes, but we're already changing Error so this might be worth cleaning up also.

[brotskydotcom]

@hwchen Thanks for the review!

With respect to non-UTF8 passwords, my thinking is that this is an edge case we should not worry about in this release. No one has ever filed an issue saying that they need to be using arbitrary byte strings as passwords, and I suspect no one ever will. The modern trend is increasingly to represent binary data using a UTF8-compatible serialization format (such as base 64 or JSON), rather than to store binary data directly in the platform (which requires platform-specific implementations in order to deal with different word sizes and endian-ness on different platforms).

If we heard people asking for it, it would be trivial to add set_password_bytes and get_password_bytes calls to the interface, and they would be a backwards-compatible extension that could be done as a single-dot release (assuming we are talking about 1.x here).

It's also not the case that adding support for byte strings allows getting rid of the BadEncoding error, because they are needed as long as we keep the string-based calls in the interface (in order to inform the caller and also return the data).

I think a much more interesting point of real extension (maybe for a 2.x release :) would be to have a Keyring trait with generic set and get variants that take all sorts of types. This would allow people to plug in their own backends for the Keyring other than the ones we support, such as are available in the Python Keyring package. These other back ends could then support any types they wanted, and we could extend our current back end to support other types as well via encodings.

[brotskydotcom]

I'm going to merge this, so we can have all the fixes ready. I'll do a pre-release tag so we can test with it and @hwchen can do a 1.0 release when ready.

[hwchen]

Great, thanks!