Implement middleware types to allow intercepting client & request handling #232
Conversation
…dling Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
==========================================
- Coverage 74.07% 73.94% -0.13%
==========================================
Files 403 407 +4
Lines 64138 64286 +148
==========================================
+ Hits 47508 47536 +28
- Misses 16630 16750 +120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Thomas Farr <[email protected]>
opensearch/src/client.rs
Outdated
| @@ -104,3 +104,9 @@ impl OpenSearch { | |||
| .await | |||
| } | |||
| } | |||
|
|
|||
| // Ensure that the client is `Send` and `Sync` | |||
There was a problem hiding this comment.
This is certainly useful but shouldn't be moved to tests?
|
|
||
| dyn_clone::clone_trait_object!(BoxedRequestHandler); | ||
|
|
||
| pub struct RequestPipeline<'a> { |
There was a problem hiding this comment.
Certainly debatable, but pipeline is a bit overloaded term here (to me), may be:
| pub struct RequestPipeline<'a> { | |
| pub struct RequestHandlerChain<'a> { |
Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
|
@reta Have made changes regarding your comments |
No rush on getting it merged, will need to go in a major release anyway due to the prior error type changes. Definitely open to feedback, don't want to do it half-baked. So take as much time as you like to go over it |
| @@ -80,11 +83,32 @@ enum Kind { | |||
| #[cfg(feature = "aws-auth")] | |||
| #[error("AwsSigV4 error: {0}")] | |||
| AwsSigV4(#[from] crate::http::aws_auth::AwsSigV4Error), | |||
|
|
|||
| #[error("request initializer error: {0}")] | |||
| RequestInitializer(#[source] BoxError<'static>), | |||
There was a problem hiding this comment.
I think we could get right of boxing (and this BoxError) type altogether and just use the reference &'static (dyn std::error::Error + Sync + Send):
#[error("request initializer error: {0}")]
RequestInitializer(#[source] &'static (dyn std::error::Error + Sync + Send)),
#[error("request pipeline error: {0}")]
RequestPipeline(#[source] &'static (dyn std::error::Error + Sync + Send)),
It is not necessarily a better option (in my opinion) but it is a bit simpler from "less abstractions" standpoint, curious what do you think about it.
There was a problem hiding this comment.
If we took a &'static reference it would mean any errors returned would need to be defined as consts or leaked memory. The + 'static bound here just means the contained data must be owned (or contain only &'static references), such that we can hold onto the type for an indeterminate amount of time. https://doc.rust-lang.org/rust-by-example/scope/lifetime/static_lifetime.html
A Box is the best way to take ownership of the arbitrary error type and "erase" it.
There was a problem hiding this comment.
A Box is the best way to take ownership of the arbitrary error type and "erase" it.
That is a strong argument, thank you. I have no other comments left
Description
Implement middleware types to allow intercepting client & request handling
Split from #132
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.