Skip to content
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

Use thiserror for credential provider errors #12424

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Jul 31, 2023

What does this PR try to resolve?

Errors from credential providers currently must a single string. This leads to a lot of .map_err(|e|cargo_credential::Error::Other(e.to_string()), which loses the source() of these errors.

This changes the cargo_credential::Error to use thiserror and adds a custom serialization for std::error::Error that preserves the source error chain across serialization / deserialization.

A unit test is added to verify serialization / deserialization.

@rustbot
Copy link
Collaborator

rustbot commented Jul 31, 2023

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
Comment on lines 7 to 9
/// `UrlNotSupported` and `NotFound` errors both cause Cargo
/// to attempt another provider, if one is available. The other
/// variants are fatal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making it easier to use, a part of me worries that we are making it harder to correctly report the right error variant.

The way I've handled this in the error codes for my CLI is to have a sloppy error type for the implementation of each function call in main and then have a more constrained error type with explicit conversions used by main. This does require the structure of the code to align well with the error variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm definitely open to feedback on improving this.

Since it will be implemented by external crates, it's hard to know up front what all the variants should be. In the original implementation, I tried to anticipate the common ones and add variants, but in this change I decided to reduce the number of variants and have an Other case.

I'm not quite sure what you're suggesting here. Removing some of the From impls that convert to Other and requiring explicit conversions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what you're suggesting here. Removing some of the From impls that convert to Other and requiring explicit conversions?

Not quite a suggestion but effectively yes. Phrased differently: "I'm looking for input on whether we should reduce From impls to encourage explicit conversions so people are more likely to specify the correct error semantics".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made changes to remove the From impls for serde::Error and io::Error and added more error contexts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I wasn't saying it had to be done but was wanting to make sure we made a conscious decision. If this is what you feel is best great. If not, feel free to say so.

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall, I appreciate how much this cleans up the code

Comment on lines 38 to 42
/// A new variant was added to this enum since Cargo was built
#[error("unknown error kind; try updating Cargo?")]
#[serde(other)]
Unknown,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: was this meant to be part of the next commit?

Comment on lines +162 to +170
#[test]
pub fn unknown_kind() {
let json = r#"{
"kind": "unexpected-kind",
"unexpected-content": "test"
}"#;
let e: Error = serde_json::from_str(&json).unwrap();
assert!(matches!(e, Error::Unknown));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: was this meant to be part of the next commit?

@epage
Copy link
Contributor

epage commented Jul 31, 2023

Overall, I'm fine with it as-is and leave it to arlosi if any of the remaining points are changed.

@bors r=arlosi

@bors
Copy link
Collaborator

bors commented Jul 31, 2023

📌 Commit 70b584e has been approved by arlosi

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@bors
Copy link
Collaborator

bors commented Jul 31, 2023

⌛ Testing commit 70b584e with merge 29a6f2f...

@epage
Copy link
Contributor

epage commented Jul 31, 2023

@bors cancel

I got the command wrong. Feel free to r= me

@bors
Copy link
Collaborator

bors commented Jul 31, 2023

☀️ Test successful - checks-actions
Approved by: arlosi
Pushing 29a6f2f to master...

@bors bors merged commit 29a6f2f into rust-lang:master Jul 31, 2023
20 checks passed
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Aug 2, 2023
Update cargo

8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a
2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000
- `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429)
- ci: rewrite bump check and respect semver (rust-lang/cargo#12395)
- fix(update): Tweak CLI behavior (rust-lang/cargo#12428)
- chore(deps): update compatible (rust-lang/cargo#12426)
- Display crate version on timings graph (rust-lang/cargo#12420)
- chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427)
- Use thiserror for credential provider errors (rust-lang/cargo#12424)
- Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422)

r? `@ghost`
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Aug 2, 2023
Update cargo

8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a
2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000
- `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429)
- ci: rewrite bump check and respect semver (rust-lang/cargo#12395)
- fix(update): Tweak CLI behavior (rust-lang/cargo#12428)
- chore(deps): update compatible (rust-lang/cargo#12426)
- Display crate version on timings graph (rust-lang/cargo#12420)
- chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427)
- Use thiserror for credential provider errors (rust-lang/cargo#12424)
- Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422)

r? ``@ghost``
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2023
Update cargo

10 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..020651c52257052d28f6fd83fbecf5cfa1ed516c
2023-07-31 00:26:46 +0000 to 2023-08-02 16:00:37 +0000
- Update rustix to 0.38.6 (rust-lang/cargo#12436)
- replace `master` branch by default branch in documentation (rust-lang/cargo#12435)
- `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429)
- ci: rewrite bump check and respect semver (rust-lang/cargo#12395)
- fix(update): Tweak CLI behavior (rust-lang/cargo#12428)
- chore(deps): update compatible (rust-lang/cargo#12426)
- Display crate version on timings graph (rust-lang/cargo#12420)
- chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427)
- Use thiserror for credential provider errors (rust-lang/cargo#12424)
- Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422)

r? `@ghost`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 3, 2023
Update cargo

10 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..020651c52257052d28f6fd83fbecf5cfa1ed516c
2023-07-31 00:26:46 +0000 to 2023-08-02 16:00:37 +0000
- Update rustix to 0.38.6 (rust-lang/cargo#12436)
- replace `master` branch by default branch in documentation (rust-lang/cargo#12435)
- `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429)
- ci: rewrite bump check and respect semver (rust-lang/cargo#12395)
- fix(update): Tweak CLI behavior (rust-lang/cargo#12428)
- chore(deps): update compatible (rust-lang/cargo#12426)
- Display crate version on timings graph (rust-lang/cargo#12420)
- chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427)
- Use thiserror for credential provider errors (rust-lang/cargo#12424)
- Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422)

r? `@ghost`
@ehuss ehuss added this to the 1.73.0 milestone Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants