Reduce unwraps in GoogleAuthz channel builder#7
Reduce unwraps in GoogleAuthz channel builder#7aarashy wants to merge 1 commit intomechiru:masterfrom
Conversation
| parts.path_and_query = Some(PathAndQuery::try_from(s).unwrap()); | ||
| parts.path_and_query = Some(PathAndQuery::try_from(s)?); | ||
|
|
||
| head.uri = Uri::from_parts(parts).unwrap(); | ||
| Request::from_parts(head, body) | ||
| head.uri = Uri::from_parts(parts)?; | ||
| Ok(Request::from_parts(head, body)) |
There was a problem hiding this comment.
The calling context already returns a crate::auth::Result, so it seems better not to unwrap if we don't need to. Maybe you have an argument for why unwrap is safe here, in which case I can revert changes to this function.
There was a problem hiding this comment.
Since the api-key has already been verified here, there will be no error here.
| let private_key = EncodingKey::from_rsa_pem(sa.private_key.as_bytes())?; | ||
| let token_uri = Uri::from_maybe_shared(sa.token_uri.clone())?; | ||
| Ok(Self { | ||
| inner: Client::new(), | ||
| header: header("JWT", sa.private_key_id), | ||
| private_key: EncodingKey::from_rsa_pem(sa.private_key.as_bytes()).unwrap(), | ||
| token_uri: Uri::from_maybe_shared(sa.token_uri.clone()).unwrap(), |
There was a problem hiding this comment.
Avoiding these unwraps is critical for any client of this library who wants to propagate Encoding Key errors properly.
Currently, building Credentials from JSON succeeds as long as the private_key field is provided in the JSON, even if the value is not a valid RSA PEM key.
An alternative would be to catch and throw this error during the building of the Credentials, but I think it's reasonable to throw the error here.
There was a problem hiding this comment.
Make sence it.I don't think we need to declare a variable, just add ?. What do you think?
There was a problem hiding this comment.
It's a stylistic choice, I'm fine to do it without declaring the variable
| let private_key = EncodingKey::from_rsa_pem(sa.private_key.as_bytes())?; | ||
| let token_uri = Uri::from_maybe_shared(sa.token_uri.clone())?; | ||
| Ok(Self { | ||
| inner: Client::new(), | ||
| header: header("JWT", sa.private_key_id), | ||
| private_key: EncodingKey::from_rsa_pem(sa.private_key.as_bytes()).unwrap(), | ||
| token_uri: Uri::from_maybe_shared(sa.token_uri.clone()).unwrap(), |
There was a problem hiding this comment.
Make sence it.I don't think we need to declare a variable, just add ?. What do you think?
| parts.path_and_query = Some(PathAndQuery::try_from(s).unwrap()); | ||
| parts.path_and_query = Some(PathAndQuery::try_from(s)?); | ||
|
|
||
| head.uri = Uri::from_parts(parts).unwrap(); | ||
| Request::from_parts(head, body) | ||
| head.uri = Uri::from_parts(parts)?; | ||
| Ok(Request::from_parts(head, body)) |
There was a problem hiding this comment.
Since the api-key has already been verified here, there will be no error here.
|
|
||
| let credentials = Credentials::builder().build().await.unwrap(); | ||
| let service = GoogleAuthz::builder(service).credentials(credentials).build().await; | ||
| let service = GoogleAuthz::builder(service).credentials(credentials).build().await.unwrap(); |
There was a problem hiding this comment.
I am concerned that the public API here is broken.
What about the idea of pre-checking that we can successfully authenticate when we build the builder?
There was a problem hiding this comment.
I think changing the public API is the right thing to do, because the GoogleAuthz builder truly is fallible. Hiding the fallibility using internal unwraps doesn't really help anybody. The compiler will naturally help anyone moving to the new version of the crate.
My philosophy here is, in general, I feel fallible functions should return Result instead of hiding their fallibility using unwrap. I personally am only okay with unwrap when, within a function or in its immediate context, we've validated the necessary logical invariants.
do you mean to validate the RSA key while building the Credentials instead of when building the GoogleAuthz? I'm open to that, it's slightly more code change, but it makes the error be caught earlier. I don't necessarily think one way is better than the other
2 participants
Currently, the builder invokes
unwrap()when parsing the RSA key, which causes panics whenever malformed RSA keys are provided.It's more safe to acknowledge that these functions are fallible and return a
Result. This PR introduces a new Error variant,AuthBuilderError, and changesnewtotry_newfor types such asMetadataandServiceAccount.Sorry my rustfmt made a lot of extra formatting changes. I can try to back them out if you prefer.