fix: add panic error when multiple HTTP features are enabled #3003
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3003 +/- ##
=======================================
+ Coverage 81.0% 81.2% +0.1%
=======================================
Files 126 126
Lines 24753 24727 -26
=======================================
+ Hits 20070 20091 +21
+ Misses 4683 4636 -47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that enabling more than one HTTP client feature results in a clear panic and adds a test for that case.
- Adds a runtime
panic!when multiple client features are active to prevent ambiguous configurations - Introduces a unit test that verifies the panic is triggered under conflicting feature sets
Co-authored-by: Björn Antonsson <ban@rhsh.net>
| #[cfg(any( | ||
| all(feature = "hyper-client", feature = "reqwest-client"), | ||
| all(feature = "hyper-client", feature = "reqwest-blocking-client"), | ||
| all(feature = "reqwest-client", feature = "reqwest-blocking-client") | ||
| ))] | ||
| { | ||
| return Err(ExporterBuildError::InternalFailure("Can't enable more than one HTTP client features simultaneously. Please choose only one: hyper-client, reqwest-client, or reqwest-blocking-client (default feature)".to_string())); | ||
| } |
There was a problem hiding this comment.
I'm not sure this is a good idea.
Because of feature unification, if you depend transitively on a crate, it might enable features which you do not explicitely have
A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change
https://doc.rust-lang.org/cargo/reference/features.html#feature-unification
I think a cleaner idea would be to define an order of precedence between clients (for instance reqwest-client > hyper-client > reqwest-blocking-client) and use the client with the higher precendence if multiple features are enabled
Fixes #2994
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes