-
Notifications
You must be signed in to change notification settings - Fork 311
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 2 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,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,11 +53,11 @@ impl fmt::Display for ErrorKind { | |
} | ||
|
||
/// A Service Bus specific error. | ||
#[derive(SafeDebug, Clone, PartialEq, Eq)] | ||
#[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 { | ||
|
@@ -74,7 +74,7 @@ impl ServiceBusError { | |
pub fn with_source( | ||
RickWinter marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,44 +50,6 @@ $resourceGroup = $DeploymentOutputs['RESOURCE_GROUP'] | |
Write-Host "Service Bus Namespace: $namespaceName" | ||
Write-Host "Resource Group: $resourceGroup" | ||
|
||
# Retrieve connection strings (these contain secrets so aren't in Bicep outputs) | ||
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. Now that you've removed everything that uses |
||
Write-Host "Retrieving Service Bus connection strings..." | ||
|
||
try { | ||
$connectionString = az servicebus namespace authorization-rule keys list ` | ||
--resource-group $resourceGroup ` | ||
--namespace-name $namespaceName ` | ||
--name RootManageSharedAccessKey ` | ||
--query primaryConnectionString ` | ||
--output tsv | ||
|
||
$listenOnlyConnectionString = az servicebus namespace authorization-rule keys list ` | ||
--resource-group $resourceGroup ` | ||
--namespace-name $namespaceName ` | ||
--name ListenOnly ` | ||
--query primaryConnectionString ` | ||
--output tsv | ||
|
||
$sendOnlyConnectionString = az servicebus namespace authorization-rule keys list ` | ||
--resource-group $resourceGroup ` | ||
--namespace-name $namespaceName ` | ||
--name SendOnly ` | ||
--query primaryConnectionString ` | ||
--output tsv | ||
|
||
Write-Host "✅ Connection strings retrieved successfully" | ||
|
||
# Set additional outputs for the test pipeline | ||
if ($CI) { | ||
Write-Host "##vso[task.setvariable variable=SERVICEBUS_CONNECTION_STRING;issecret=true]$connectionString" | ||
Write-Host "##vso[task.setvariable variable=SERVICEBUS_LISTEN_ONLY_CONNECTION_STRING;issecret=true]$listenOnlyConnectionString" | ||
Write-Host "##vso[task.setvariable variable=SERVICEBUS_SEND_ONLY_CONNECTION_STRING;issecret=true]$sendOnlyConnectionString" | ||
} | ||
} | ||
catch { | ||
Write-Warning "Failed to retrieve connection strings: $($_.Exception.Message)" | ||
} | ||
|
||
Write-Host "##[endgroup]" | ||
|
||
Write-Host "Service Bus post-deployment setup completed successfully." |
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.
The error type should just be
Debug
notSafeDebug
. All the information is kinda important and you should make sure nothing inside it otherwise leaks.typespec::Error
implDebug
.