-
Notifications
You must be signed in to change notification settings - Fork 313
Service Bus improvements #2935
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
base: main
Are you sure you want to change the base?
Service Bus improvements #2935
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Setup script for OpenSSL environment to enable workspace-wide clippy and builds | ||
# Run this script before running cargo clippy --workspace or similar commands | ||
|
||
Write-Host "Setting up OpenSSL environment for Rust builds..." -ForegroundColor Green | ||
|
||
# Set VCPKG environment variables | ||
$env:VCPKG_ROOT = "C:\vcpkg" | ||
$env:OPENSSL_DIR = "C:\vcpkg\installed\x64-windows-static-md" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why a global directory when we may have different versions for different repos? I have several worktrees (repos) on my machine, which is why the existing docs in our root If you're going to codify those instructions:
|
||
$env:CMAKE_TOOLCHAIN_FILE = "C:\vcpkg\scripts\buildsystems\vcpkg.cmake" | ||
|
||
Write-Host "Environment variables set:" -ForegroundColor Yellow | ||
Write-Host " VCPKG_ROOT = $env:VCPKG_ROOT" | ||
Write-Host " OPENSSL_DIR = $env:OPENSSL_DIR" | ||
Write-Host " CMAKE_TOOLCHAIN_FILE = $env:CMAKE_TOOLCHAIN_FILE" | ||
|
||
# Check if vcpkg and OpenSSL are installed | ||
if (-not (Test-Path "C:\vcpkg\vcpkg.exe")) { | ||
Write-Host "WARNING: vcpkg not found at C:\vcpkg\vcpkg.exe" -ForegroundColor Red | ||
Write-Host "Please run the following commands to install vcpkg and OpenSSL:" -ForegroundColor Yellow | ||
Write-Host " git clone https://github.com/Microsoft/vcpkg.git C:\vcpkg" | ||
Comment on lines
+17
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why script it if you're not going to do it for them? Why is this script beneficial instead of just using the existing |
||
Write-Host " C:\vcpkg\bootstrap-vcpkg.bat" | ||
Write-Host " C:\vcpkg\vcpkg.exe integrate install" | ||
Write-Host " C:\vcpkg\vcpkg.exe install openssl:x64-windows-static-md" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What version? We may pin a version, which we already have in our |
||
} | ||
elseif (-not (Test-Path "C:\vcpkg\installed\x64-windows-static-md\lib\libssl.lib")) { | ||
Write-Host "WARNING: OpenSSL not found in vcpkg installation" -ForegroundColor Red | ||
Write-Host "Please run: C:\vcpkg\vcpkg.exe install openssl:x64-windows-static-md" | ||
} | ||
else { | ||
Write-Host "✓ vcpkg and OpenSSL are properly installed" -ForegroundColor Green | ||
Write-Host "" | ||
Write-Host "You can now run:" -ForegroundColor Cyan | ||
Write-Host " cargo clippy --workspace --all-features --all-targets --keep-going --no-deps" | ||
Write-Host " cargo build --workspace --all-features --all-targets" | ||
Write-Host " cargo test --workspace" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
//! Clients used to communicate with Azure Service Bus | ||
|
||
mod servicebus_client; | ||
|
||
pub use servicebus_client::{ | ||
CreateReceiverOptions, CreateSenderOptions, ServiceBusClient, ServiceBusClientBuilder, | ||
ServiceBusClientOptions, SubQueue, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,16 +53,16 @@ impl fmt::Display for ErrorKind { | |
} | ||
|
||
/// A Service Bus specific error. | ||
#[derive(SafeDebug, Clone, PartialEq, Eq)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error type should just be |
||
#[derive(SafeDebug)] | ||
pub struct ServiceBusError { | ||
kind: ErrorKind, | ||
message: String, | ||
source: Option<Box<ServiceBusError>>, | ||
source: Option<Box<dyn std::error::Error + Send + Sync + 'static>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why add |
||
} | ||
heaths marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
impl ServiceBusError { | ||
/// Creates a new Service Bus error. | ||
pub fn new(kind: ErrorKind, message: impl Into<String>) -> Self { | ||
pub(crate) fn new(kind: ErrorKind, message: impl Into<String>) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why protect it? What if someone wanted to wrap our client and return their own instance of a |
||
Self { | ||
kind, | ||
message: message.into(), | ||
|
@@ -71,10 +71,10 @@ impl ServiceBusError { | |
} | ||
|
||
/// Creates a new Service Bus error with a source error. | ||
pub fn with_source( | ||
pub(crate) fn with_source( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
kind: ErrorKind, | ||
message: impl Into<String>, | ||
source: ServiceBusError, | ||
source: impl std::error::Error + Send + Sync + 'static, | ||
) -> Self { | ||
Self { | ||
kind, | ||
|
@@ -94,8 +94,10 @@ impl ServiceBusError { | |
} | ||
|
||
/// Returns the source error, if any. | ||
pub fn source(&self) -> Option<&ServiceBusError> { | ||
self.source.as_deref() | ||
pub fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have a |
||
self.source | ||
.as_ref() | ||
.map(|e| e.as_ref() as &(dyn std::error::Error + 'static)) | ||
} | ||
} | ||
|
||
|
@@ -107,7 +109,9 @@ impl fmt::Display for ServiceBusError { | |
|
||
impl std::error::Error for ServiceBusError { | ||
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||
self.source.as_ref().map(|e| e as &dyn std::error::Error) | ||
self.source | ||
.as_ref() | ||
.map(|e| e.as_ref() as &(dyn std::error::Error + 'static)) | ||
} | ||
} | ||
|
||
|
@@ -123,12 +127,65 @@ impl From<azure_core::error::Error> for ServiceBusError { | |
_ => ErrorKind::Unknown, | ||
}; | ||
|
||
ServiceBusError::new(kind, error.to_string()) | ||
ServiceBusError::with_source(kind, error.to_string(), error) | ||
} | ||
} | ||
|
||
impl From<azure_core_amqp::AmqpError> for ServiceBusError { | ||
fn from(error: azure_core_amqp::AmqpError) -> Self { | ||
ServiceBusError::new(ErrorKind::Amqp, error.to_string()) | ||
ServiceBusError::with_source(ErrorKind::Amqp, error.to_string(), error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you expect to provide descriptive texts for servicebus errors generated from other errors? If your general practice is to just use |
||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn test_servicebus_error_can_store_any_std_error() { | ||
// Test that we can store any std::error::Error as a source | ||
let io_error = std::io::Error::other("test error"); | ||
let service_bus_error = | ||
ServiceBusError::with_source(ErrorKind::Unknown, "wrapper error", io_error); | ||
|
||
assert_eq!(service_bus_error.kind(), &ErrorKind::Unknown); | ||
assert_eq!(service_bus_error.message(), "wrapper error"); | ||
assert!(service_bus_error.source().is_some()); | ||
|
||
// Verify the source can be downcast to the original error type | ||
let source = service_bus_error.source().unwrap(); | ||
assert!(source.downcast_ref::<std::io::Error>().is_some()); | ||
} | ||
|
||
#[test] | ||
fn test_servicebus_error_implements_std_error() { | ||
let error = ServiceBusError::new(ErrorKind::InvalidRequest, "test message"); | ||
|
||
// Should implement std::error::Error | ||
let _: &dyn std::error::Error = &error; | ||
|
||
// Should return None for source when no source is set | ||
assert!(error.source().is_none()); | ||
} | ||
|
||
#[test] | ||
fn test_servicebus_error_with_chain() { | ||
let inner_error = std::io::Error::other("inner error"); | ||
let middle_error = | ||
ServiceBusError::with_source(ErrorKind::Amqp, "middle error", inner_error); | ||
let outer_error = | ||
ServiceBusError::with_source(ErrorKind::Unknown, "outer error", middle_error); | ||
|
||
// Check that we can traverse the error chain | ||
assert_eq!(outer_error.kind(), &ErrorKind::Unknown); | ||
assert_eq!(outer_error.message(), "outer error"); | ||
|
||
let source = outer_error.source().unwrap(); | ||
let middle_as_servicebus = source.downcast_ref::<ServiceBusError>().unwrap(); | ||
assert_eq!(middle_as_servicebus.kind(), &ErrorKind::Amqp); | ||
assert_eq!(middle_as_servicebus.message(), "middle error"); | ||
|
||
let inner_source = middle_as_servicebus.source().unwrap(); | ||
assert!(inner_source.downcast_ref::<std::io::Error>().is_some()); | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#!/usr/bin/env pwsh
chmod +x
the script. If you're on Windows, rungit update-index --chmod=+x <path>
.