Service Bus improvements #2935

RickWinter · 2025-08-20T23:19:24Z

Allow capturing off all types of errors within the ServiceBusError
Move ServiceBusClient to the clients folder.
Use constants for the keys
Fix default client version
Change method visibility to be pub(crate)

Allow capturing off all types of errors within the ServiceBusError Move servicebusclient to the clients folder. Use contants for the keys Fix default client version Change method visibility to be pub(crate)

Copilot

Pull Request Overview

This PR implements service bus improvements focusing on error handling, code organization, and configuration updates. The changes enhance error capturing capabilities by allowing ServiceBusError to wrap any standard error type, reorganize client code into a dedicated clients module, and update API versions throughout.

Enhanced ServiceBusError to accept any std::error::Error as source instead of just ServiceBusError
Moved ServiceBusClient from client module to clients module for better organization
Added constants for Service Bus operation keys and updated default API version

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sdk/servicebus/test-resources.bicep	Updates API versions from 2022-10-01-preview to 2024-01-01
sdk/servicebus/test-resources-post.ps1	Removes connection string retrieval logic
sdk/servicebus/azure_messaging_servicebus/test-resources-pre.ps1	Deletes empty pre-deployment script
sdk/servicebus/azure_messaging_servicebus/src/sender.rs	Updates import path from client to clients module
sdk/servicebus/azure_messaging_servicebus/src/receiver.rs	Updates import path and adds constants for Service Bus operation keys
sdk/servicebus/azure_messaging_servicebus/src/lib.rs	Changes module declaration from client to clients
sdk/servicebus/azure_messaging_servicebus/src/error.rs	Enhances ServiceBusError to accept any std::error::Error as source and adds comprehensive tests
sdk/servicebus/azure_messaging_servicebus/src/clients/servicebus_client.rs	Updates default API version and changes method visibility
sdk/servicebus/azure_messaging_servicebus/src/clients/mod.rs	New module file for clients organization

sdk/servicebus/azure_messaging_servicebus/src/error.rs

LarryOsterman · 2025-08-20T23:46:43Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

@@ -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)> {


Why have a source() function on ServiceBusError which is a different implementation from the trait version? The two appear to be basically identical.

sdk/servicebus/azure_messaging_servicebus/src/error.rs

LarryOsterman · 2025-08-20T23:49:20Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

    }
 }

 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)


Do you expect to provide descriptive texts for servicebus errors generated from other errors? If your general practice is to just use .to_string(), there's not a ton of value in the message parameter.

heaths · 2025-08-22T23:00:13Z

eng/scripts/setup-openssl-env.ps1

Needs a shebang:
#!/usr/bin/env pwsh

Needs a copyright header after that (Microsoft OSS rules).

You need to chmod +x the script. If you're on Windows, run git update-index --chmod=+x <path>.

heaths · 2025-08-22T23:02:56Z

eng/scripts/setup-openssl-env.ps1

+
+# Set VCPKG environment variables
+$env:VCPKG_ROOT = "C:\vcpkg"
+$env:OPENSSL_DIR = "C:\vcpkg\installed\x64-windows-static-md"


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 CONTRIBUTING.md say to use a repo-related path.

If you're going to codify those instructions:

They should be compatible with what I've already tested works in more ways than this.

Those instruction should be updated to have people run this script instead.

heaths · 2025-08-22T23:04:22Z

eng/scripts/setup-openssl-env.ps1

+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"


Why script it if you're not going to do it for them? Why is this script beneficial instead of just using the existing CONTRIBUTING.md if we don't basically do it all for them? Why not go that extra step?

heaths · 2025-08-22T23:04:48Z

eng/scripts/setup-openssl-env.ps1

+  Write-Host "  git clone https://github.com/Microsoft/vcpkg.git C:\vcpkg"
+  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"


What version? We may pin a version, which we already have in our eng/vcpkg.json that, BTW, you're not using.

heaths · 2025-08-22T23:05:37Z

sdk/servicebus/test-resources.bicep

@@ -35,7 +35,7 @@ resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2024-01-01' = {
    zoneRedundant: false
  }

-  resource rootSharedAccessKey 'AuthorizationRules@2022-10-01-preview' = {
+  resource rootSharedAccessKey 'AuthorizationRules@2024-01-01' = {


Just get rid of the versions. They inherit the api-version of the parent resource. It's cleaner and less chance of incompatibility.

heaths · 2025-08-22T23:06:58Z

sdk/servicebus/test-resources-post.ps1

@@ -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)


Now that you've removed everything that uses az CLI, 1) you should get rid of lines 39-41 because they are unnecessary and create an additional unnecessary dependency, but then 2) why have this script at all? It does nothing useful. Just delete it.

heaths · 2025-08-22T23:10:37Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

@@ -53,16 +53,16 @@ impl fmt::Display for ErrorKind {
 }

 /// A Service Bus specific error.
-#[derive(SafeDebug, Clone, PartialEq, Eq)]


The error type should just be Debug not SafeDebug. All the information is kinda important and you should make sure nothing inside it otherwise leaks. typespec::Error impl Debug.

heaths · 2025-08-22T23:11:21Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

 pub struct ServiceBusError {
    kind: ErrorKind,
    message: String,
-    source: Option<Box<ServiceBusError>>,
+    source: Option<Box<dyn std::error::Error + Send + Sync + 'static>>,


Why add Send, Sync, and 'static constraints? That makes it pretty difficult to send any error since not all implement those traits.

heaths · 2025-08-22T23:11:57Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

 }

 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 {


Why protect it? What if someone wanted to wrap our client and return their own instance of a ServiceBusError? I don't see any value in this.

heaths · 2025-08-22T23:12:01Z

sdk/servicebus/azure_messaging_servicebus/src/error.rs

@@ -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(


Service Bus improvements

c3fab5e

Allow capturing off all types of errors within the ServiceBusError Move servicebusclient to the clients folder. Use contants for the keys Fix default client version Change method visibility to be pub(crate)

Copilot AI review requested due to automatic review settings August 20, 2025 23:19

RickWinter requested review from heaths, ronniegeraghty and LarryOsterman as code owners August 20, 2025 23:19

Copilot AI reviewed Aug 20, 2025

View reviewed changes

sdk/servicebus/azure_messaging_servicebus/src/error.rs Show resolved Hide resolved

Fix up clippy failures

efee706

LarryOsterman reviewed Aug 20, 2025

View reviewed changes

Add a file that helps configure OpenSSL for running clippy

cd91892

RickWinter requested review from hallipr and weshaggard as code owners August 20, 2025 23:51

Construction of the Error is internal to the crate.

1727d84

heaths requested changes Aug 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Service Bus improvements #2935

Service Bus improvements #2935

Uh oh!

RickWinter commented Aug 20, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

LarryOsterman Aug 20, 2025

Uh oh!

Uh oh!

LarryOsterman Aug 20, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

heaths Aug 22, 2025

Uh oh!

Uh oh!

Service Bus improvements #2935

Are you sure you want to change the base?

Service Bus improvements #2935

Uh oh!

Conversation

RickWinter commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RickWinter commented Aug 20, 2025 •

edited

Loading