small functional improvements #662
Conversation
`.customize()` function was not accesible from outside of the crate because trait StripeRequest was not exported anywhere Signed-off-by: Robin Ilyk <[email protected]>
There was a problem hiding this comment.
Thanks for contributing this @robko23, there are definitely lots of improvements that can be made!
I know this is in draft, but I'd be happy to merge this with the added assert! either changed to an error or removed since the other 2 changes should be pretty uncontroversial improvements.
| @@ -109,7 +109,9 @@ impl Client { | |||
| let mut last_error = StripeError::ClientError("invalid strategy".into()); | |||
|
|
|||
| if let Some(key) = strategy.get_key() { | |||
| req_builder = req_builder.header(HeaderName::from_static("Idempotency-Key"), key); | |||
| const HEADER_NAME: HeaderName = HeaderName::from_static("idempotency-key"); | |||
There was a problem hiding this comment.
Wow, that's a bad panic! Think this must've happened because the old usage of http-types instead of hyper/http supported this, and there must be no test coverage of idempotency key usage
async-stripe/src/hyper/client.rs
Outdated
| @@ -109,7 +109,9 @@ impl Client { | |||
| let mut last_error = StripeError::ClientError("invalid strategy".into()); | |||
|
|
|||
| if let Some(key) = strategy.get_key() { | |||
| req_builder = req_builder.header(HeaderName::from_static("Idempotency-Key"), key); | |||
| const HEADER_NAME: HeaderName = HeaderName::from_static("idempotency-key"); | |||
| assert!(!key.is_empty(), "idempotency key is empty"); | |||
There was a problem hiding this comment.
Probably would be better to return an error here, could add a variant to StripeError. Even better would be to validate on construction of RequestStrategy, but that's not currently possible without hiding more of the internal representation there.
There was a problem hiding this comment.
yeah, I put that assertion here because I wanted to narrow down the out of bound index and then I left it there because it seemed a good sanity check.
However I also think that having a proper error would be better, so I created a commit that adds a newtype for the idempotency key. It is basically a string that cannot be empty or longer than 255 chars. It has its own error, because I think its mostly isolated from the rest of the logic. Having it mostly transparent struct also doesn't hide any implementation and representation
idempotency key cannot be empty and cannot be longer than 255 characters and having asserts before their usage is not the best error handling strategy. having it in a newtype guarantees that every use of this key is valid and no assert or runtime panic is needed. Refs: arlyon#662 (comment) Refs: https://docs.stripe.com/api/idempotent_requests Signed-off-by: Robin Ilyk <[email protected]>
|
I've removed the assert and if you want to I can mark it as ready and you can merge it, but there will be more small changes from me as I use it more. I didn't want to overwhelm you with these one line PRs so my idea was to pack it in a single PR with many small commits. But if you don't mind this, I can break it down to more PRs |
idempotency key cannot be empty and cannot be longer than 255 characters and having asserts before their usage is not the best error handling strategy. having it in a newtype guarantees that every use of this key is valid and no assert or runtime panic is needed. Refs: arlyon#662 (comment) Refs: https://docs.stripe.com/api/idempotent_requests Signed-off-by: Robin Ilyk <[email protected]>
|
Sorry for the delay here @robko23! I think the newtype approach is the right one generally, and I think that commit will be useful at a later point, but I'd imagine there will be more rounds of iteration on We can also make an issue to start tracking / discussing what the most useful form of I'd be happy to merge if you don't mind just reverting the |
|
Okay, so I cherry picked the commit with the newtype and I will create a new pull request with it. Then I removed this from this branch. Then I changed the commit with the assert and removed it, but I still kept the |
robko23
changed the title
idempotency key cannot be empty and cannot be longer than 255 characters and having asserts before their usage is not the best error handling strategy. having it in a newtype guarantees that every use of this key is valid and no assert or runtime panic is needed. Refs: arlyon#662 (comment) Refs: arlyon#662 (comment) Refs: https://docs.stripe.com/api/idempotent_requests Signed-off-by: Robin Ilyk <[email protected]>
|
Ok, sorry for the annoyance, but CI should be good now, so if you can rebase I'll merge it! |
|
Hey thanks for this @robko23 ! I have been MIA for a little bit (was very ill for a couple of months the end of last year but I am back on form). @mzeitlin11 seems to be carrying the torch just fine in the mean time but I will stamp anyways. EDIT: have updated the branch via a merge commit, will merge the PR if everything is green |
|
Seems like there is a genuine clippy lint here. Feel free to @ either myself or Matthew when this is addressed |
http crate does not allow uppercase characters in header name and it was causing crashes when hitting thing branch with error "index out of bounds." The error should happen at compile time, but for some reason the HeaderName::from_static was executed at runtime, so this commit forces it to be executed at compile time by extracing it into the const. Signed-off-by: Robin Ilyk <[email protected]>
|
hi @arlyon! Thanks for the response and I hope your health will be better in the future. About the clippy lint, I have decided to ignore ( |
idempotency key cannot be empty and cannot be longer than 255 characters and having asserts before their usage is not the best error handling strategy. having it in a newtype guarantees that every use of this key is valid and no assert or runtime panic is needed. Refs: arlyon#662 (comment) Refs: arlyon#662 (comment) Refs: https://docs.stripe.com/api/idempotent_requests Signed-off-by: Robin Ilyk <[email protected]>
|
Hi! Thank you for your thorough work. Could you please merge this as soon as possible? I need to use the idempotent request strategy. Thanks! |
Summary
Hi, first of all - amazing work what you've done on the next branch. The api is absolutely amazing, but it still has some rough edges.
I've done one integration on 0.40 and now I'm building another integration and I've decided to use the next branch, so that I can test its ergonomics and help to improve it.
I would consider this PR a draft for now, because I'm just starting with the integration and I probably will send more patches if something doesn't work or doesn't seem right.
For now this PR contains two small commits - everything is explained in detail in the commit message as to why I've done it, but here is a list of things I've done (I will be editing this list with the new patches as I create them):
StripeRequesttrait - allow.customize()to be used outside this crateIdempotency-Keytoidempotency-key- caused panic on runtimeChecklist
cargo make fmt