From 41a231da1a81d4836703917ad0f376b843e8e53e Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 10:36:57 -0800 Subject: [PATCH 01/24] Enabled multithreaded calls to eventhubs management APIs --- .../azure-core-amqp/src/amqp/management.cpp | 96 ++++++++++---- .../src/amqp/private/management_impl.hpp | 14 +- .../test/ut/message_sender_receiver.cpp | 2 +- .../messaging/eventhubs/consumer_client.hpp | 11 +- .../messaging/eventhubs/producer_client.hpp | 12 +- .../src/consumer_client.cpp | 49 +++++-- .../src/private/eventhubs_utilities.hpp | 35 +---- .../src/producer_client.cpp | 46 +++++-- .../test/ut/consumer_client_test.cpp | 124 +++++++++++++++++- .../test/ut/processor_test.cpp | 43 +----- 10 files changed, 302 insertions(+), 130 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 2774126517..db8cece810 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -75,7 +75,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::ManagementOpenStatus ManagementClientImpl::Open(Context const& context) { - /** Authentication needs to happen *before* the management object is created. + /** Authentication needs to happen *before* the links are created. * * Note that we ONLY enable authentication if we know we're talking to the management node. * Other nodes require their own authentication. @@ -101,6 +101,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { messageReceiverOptions.MessageTarget = m_options.ManagementNodeName; messageReceiverOptions.Name = m_options.ManagementNodeName + "-receiver"; messageReceiverOptions.AuthenticationRequired = false; + messageReceiverOptions.SettleMode = _internal::ReceiverSettleMode::First; m_messageReceiver = std::make_shared( m_session, m_options.ManagementNodeName, messageReceiverOptions, this); @@ -165,10 +166,20 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { { messageToSend.ApplicationProperties.emplace("locales", locales); } - messageToSend.Properties.MessageId = m_nextMessageId; - m_expectedMessageId = m_nextMessageId; - m_sendCompleted = false; - m_nextMessageId++; + + // Set the message ID and remember it for later. + auto requestId = Azure::Core::Uuid::CreateUuid().ToString(); + + messageToSend.Properties.MessageId + = static_cast(requestId); + { + std::unique_lock lock(m_messageQueuesLock); + + Log::Stream(Logger::Level::Verbose) + << "ManagementClient::ExecuteOperation: " << requestId << ". Create Queue for request."; + m_messageQueues.emplace(requestId, std::make_unique()); + m_sendCompleted = false; + } auto sendResult = m_messageSender->Send(messageToSend, context); if (std::get<0>(sendResult) != _internal::MessageSendStatus::Ok) @@ -178,9 +189,15 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { rv.StatusCode = 500; rv.Error = std::get<1>(sendResult); rv.Message = nullptr; + { + std::unique_lock lock(m_messageQueuesLock); + // Remove the queue from the map, we don't need it anymore. + m_messageQueues.erase(requestId); + } return rv; } - auto result = m_messageQueue.WaitForResult(context); + + auto result = m_messageQueues.at(requestId)->WaitForResult(context); if (result) { _internal::ManagementOperationResult rv; @@ -188,6 +205,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { rv.StatusCode = std::get<1>(*result); rv.Error = std::get<2>(*result); rv.Message = std::get<3>(*result); + + { + std::unique_lock lock(m_messageQueuesLock); + // Remove the queue from the map, we don't need it anymore. + m_messageQueues.erase(requestId); + } return rv; } else @@ -452,6 +475,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } Models::AmqpValue ManagementClientImpl::IndicateError( + std::string const& correlationId, std::string const& condition, std::string const& description) { @@ -466,11 +490,19 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // Let external callers know that the error was triggered. m_eventHandler->OnError(error); } + if (!correlationId.empty()) + { + // Ensure nobody else is messing with the message queues right now. + std::unique_lock lock(m_messageQueuesLock); - // Complete any outstanding receives with an error. - m_messageQueue.CompleteOperation( - _internal::ManagementOperationStatus::Error, 500, error, nullptr); - + // Remove the queue from the map, we don't need it anymore. + if (m_messageQueues.find(correlationId) != m_messageQueues.end()) + { + // Complete any outstanding receives with an error. + m_messageQueues.at(correlationId) + ->CompleteOperation(_internal::ManagementOperationStatus::Error, 500, error, nullptr); + } + } return Models::_internal::Messaging::DeliveryRejected(condition, description, {}); } @@ -478,30 +510,50 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::MessageReceiver const&, std::shared_ptr const& message) { - if (message->ApplicationProperties.empty()) + if (m_options.EnableTrace) { - return IndicateError( - Models::_internal::AmqpErrorCondition::InternalError.ToString(), - "Received message does not have application properties."); + Log::Stream(Logger::Level::Informational) << "Received message: " << *message; } if (!message->Properties.CorrelationId.HasValue()) { return IndicateError( + "", Models::_internal::AmqpErrorCondition::InternalError.ToString(), "Received message correlation ID not found."); } - else if (message->Properties.CorrelationId.Value().GetType() != Models::AmqpValueType::Ulong) + else if (message->Properties.CorrelationId.Value().GetType() != Models::AmqpValueType::String) { return IndicateError( + "", Models::_internal::AmqpErrorCondition::InternalError.ToString(), "Received message correlation ID is not a ulong."); } - uint64_t correlationId = message->Properties.CorrelationId.Value(); + std::string requestId = static_cast(message->Properties.CorrelationId.Value()); + + // Ensure nobody else is messing with the message queues right now. + std::unique_lock lock(m_messageQueuesLock); + auto messageQueue = m_messageQueues.find(requestId); + if (messageQueue == m_messageQueues.end()) + { + return IndicateError( + requestId, + Models::_internal::AmqpErrorCondition::InternalError.ToString(), + "Received message correlation ID does not match request ID."); + } + + if (message->ApplicationProperties.empty()) + { + return IndicateError( + requestId, + Models::_internal::AmqpErrorCondition::InternalError.ToString(), + "Received message does not have application properties."); + } auto statusCodeMap = message->ApplicationProperties.find(m_options.ExpectedStatusCodeKeyName); if (statusCodeMap == message->ApplicationProperties.end()) { return IndicateError( + requestId, Models::_internal::AmqpErrorCondition::InternalError.ToString(), "Received message does not have a " + m_options.ExpectedStatusCodeKeyName + " status code key."); @@ -509,6 +561,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { else if (statusCodeMap->second.GetType() != Models::AmqpValueType::Int) { return IndicateError( + requestId, Models::_internal::AmqpErrorCondition::InternalError.ToString(), "Received message " + m_options.ExpectedStatusCodeKeyName + " value is not an int."); } @@ -523,6 +576,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { if (statusDescription->second.GetType() != Models::AmqpValueType::String) { return IndicateError( + requestId, Models::_internal::AmqpErrorCondition::InternalError.ToString(), "Received message " + m_options.ExpectedStatusDescriptionKeyName + " value is not a string."); @@ -530,12 +584,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { description = static_cast(statusDescription->second); } - if (correlationId != m_expectedMessageId) - { - return IndicateError( - Models::_internal::AmqpErrorCondition::InternalError.ToString(), - "Received message correlation ID does not match request ID."); - } if (!m_sendCompleted) { if (m_options.EnableTrace) @@ -552,12 +600,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // 2616](https://www.rfc-editor.org/rfc/rfc2616#section-6.1.1) status codes. if ((statusCode < 200) || (statusCode > 299)) { - m_messageQueue.CompleteOperation( + m_messageQueues.at(requestId)->CompleteOperation( _internal::ManagementOperationStatus::FailedBadStatus, statusCode, messageError, message); } else { - m_messageQueue.CompleteOperation( + m_messageQueues.at(requestId)->CompleteOperation( _internal::ManagementOperationStatus::Ok, statusCode, messageError, message); } return Models::_internal::Messaging::DeliveryAccepted(); diff --git a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp index 29aa3e6a0a..cf510b4ec9 100644 --- a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp @@ -17,6 +17,7 @@ #include #include +#include #include namespace Azure { namespace Core { namespace Amqp { namespace _detail { @@ -87,15 +88,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Azure::Core::Amqp::Common::_internal::AsyncOperationQueue<_internal::ManagementOpenStatus> m_openCompleteQueue; - uint64_t m_nextMessageId{0}; - - // What is the message ID expected for the current outstanding operation? - uint64_t m_expectedMessageId{}; bool m_sendCompleted{false}; void SetState(ManagementState newState); // Reflect the error state to the OnError callback and return a delivery rejected status. Models::AmqpValue IndicateError( + std::string const& correlationId, std::string const& errorCondition, std::string const& errorDescription); @@ -106,12 +104,14 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { std::string m_managementEntityPath; Azure::Core::Credentials::AccessToken m_accessToken; - Azure::Core::Amqp::Common::_internal::AsyncOperationQueue< + using ManagementOperationQueue = Azure::Core::Amqp::Common::_internal::AsyncOperationQueue< _internal::ManagementOperationStatus, std::uint32_t, Models::_internal::AmqpError, - std::shared_ptr> - m_messageQueue; + std::shared_ptr>; + + std::recursive_mutex m_messageQueuesLock; + std::map> m_messageQueues; // Inherited via MessageSenderEvents void OnMessageSenderStateChanged( diff --git a/sdk/core/azure-core-amqp/test/ut/message_sender_receiver.cpp b/sdk/core/azure-core-amqp/test/ut/message_sender_receiver.cpp index 97f6439e78..64d1d79bff 100644 --- a/sdk/core/azure-core-amqp/test/ut/message_sender_receiver.cpp +++ b/sdk/core/azure-core-amqp/test/ut/message_sender_receiver.cpp @@ -907,7 +907,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { { GTEST_LOG_(INFO) << "Trigger message send for polling."; serviceEndpoint->ShouldSendMessage(true); - std::this_thread::sleep_for(std::chrono::milliseconds(500)); + std::this_thread::sleep_for(std::chrono::seconds(2)); GTEST_LOG_(INFO) << "Message should have been sent and processed."; auto result = receiver.TryWaitForIncomingMessage(); EXPECT_TRUE(result.first); diff --git a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp index 184eb07c4d..47e34b83d4 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -187,17 +188,23 @@ namespace Azure { namespace Messaging { namespace EventHubs { std::map m_receivers; /// @brief The AMQP Sessions used to receive messages for a given partition. - std::mutex m_sessionsLock; + std::recursive_mutex m_sessionsLock; std::map m_sessions; std::map m_connections; + // Management client used for GetProperty operations. + bool m_managementClientIsOpen{false}; + std::unique_ptr m_managementClient; + /// @brief The options used to configure the consumer client. ConsumerClientOptions m_consumerClientOptions; void EnsureConnection(std::string const& partitionId); void EnsureSession(std::string const& partitionId); + void EnsureManagementClient(const Azure::Core::Context& context = {}); Azure::Core::Amqp::_internal::Connection CreateConnection(std::string const& partitionId) const; - Azure::Core::Amqp::_internal::Session CreateSession(std::string const& partitionId); + Azure::Core::Amqp::_internal::Session CreateSession(std::string const& partitionId) const; Azure::Core::Amqp::_internal::Session GetSession(std::string const& partitionId); + std::unique_ptr const& GetManagementClient(); }; }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp index 88c4b032c2..1dc92959fb 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp @@ -92,6 +92,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { ~ProducerClient() { + if (m_managementClient && m_isManagementClientOpen) + { + m_managementClient->Close(); + } + for (auto& sender : m_senders) { sender.second.Close(); @@ -177,8 +182,10 @@ namespace Azure { namespace Messaging { namespace EventHubs { std::mutex m_sendersLock; std::map m_connections{}; std::map m_senders{}; + std::unique_ptr m_managementClient; + bool m_isManagementClientOpen{false}; - std::mutex m_sessionsLock; + std::recursive_mutex m_sessionsLock; std::map m_sessions{}; Azure::Core::Amqp::_internal::Connection CreateConnection() const; @@ -193,7 +200,10 @@ namespace Azure { namespace Messaging { namespace EventHubs { // Ensure that a message sender for the specified partition has been created. void EnsureSender(std::string const& partitionId, Azure::Core::Context const& context = {}); + void EnsureManagementClient(Azure::Core::Context const& context = {}); + Azure::Core::Amqp::_internal::MessageSender GetSender(std::string const& partitionId); Azure::Core::Amqp::_internal::Session GetSession(std::string const& partitionId); + std::unique_ptr const& GetManagementClient(); }; }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp b/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp index 63575cdfb5..d28ff12e59 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp @@ -57,6 +57,10 @@ namespace Azure { namespace Messaging { namespace EventHubs { { sender.second.Close(); } + if (m_managementClientIsOpen) + { + m_managementClient->Close(); + } while (!m_sessions.empty()) { m_sessions.erase(m_sessions.begin()); @@ -87,7 +91,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { void ConsumerClient::EnsureConnection(std::string const& partitionId) { - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); if (m_connections.find(partitionId) == m_connections.end()) { m_connections.emplace(partitionId, CreateConnection(partitionId)); @@ -95,7 +99,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { } Azure::Core::Amqp::_internal::Session ConsumerClient::CreateSession( - std::string const& partitionId) + std::string const& partitionId) const { SessionOptions sessionOptions; sessionOptions.InitialIncomingWindowSize @@ -107,17 +111,45 @@ namespace Azure { namespace Messaging { namespace EventHubs { void ConsumerClient::EnsureSession(std::string const& partitionId) { EnsureConnection(partitionId); - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); if (m_sessions.find(partitionId) == m_sessions.end()) { m_sessions.emplace(partitionId, CreateSession(partitionId)); } } + void ConsumerClient::EnsureManagementClient(Azure::Core::Context const& context) + { + EnsureSession({}); + auto session{GetSession({})}; + + std::unique_lock lock(m_sessionsLock); + if (!m_managementClient) + { + // Create a management client off the session. + // Eventhubs management APIs return a status code in the "status-code" application properties. + Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; + managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; + managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; + m_managementClient = std::make_unique( + session.CreateManagementClient(m_eventHub, managementClientOptions)); + + m_managementClient->Open(context); + m_managementClientIsOpen = true; + } + } + + std::unique_ptr const& + ConsumerClient::GetManagementClient() + { + std::unique_lock lock(m_sessionsLock); + return m_managementClient; + } + Azure::Core::Amqp::_internal::Session ConsumerClient::GetSession( std::string const& partitionId = {}) { - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); return m_sessions.at(partitionId); } @@ -143,18 +175,19 @@ namespace Azure { namespace Messaging { namespace EventHubs { Models::EventHubProperties ConsumerClient::GetEventHubProperties(Core::Context const& context) { // Since EventHub properties are not tied to a partition, we don't specify a partition ID. - EnsureSession({}); + EnsureManagementClient(context); - return _detail::EventHubsUtilities::GetEventHubsProperties(GetSession(), m_eventHub, context); + return _detail::EventHubsUtilities::GetEventHubsProperties( + GetManagementClient(), m_eventHub, context); } Models::EventHubPartitionProperties ConsumerClient::GetPartitionProperties( std::string const& partitionId, Core::Context const& context) { - EnsureSession({}); + EnsureManagementClient(context); return _detail::EventHubsUtilities::GetEventHubsPartitionProperties( - GetSession({}), m_eventHub, partitionId, context); + GetManagementClient(), m_eventHub, partitionId, context); } }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index af34e29190..8f1d961499 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -20,7 +20,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail { - constexpr bool EnableAmqpTrace = false; + constexpr bool EnableAmqpTrace = true; class EventHubsExceptionFactory { public: @@ -106,27 +106,16 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } static Models::EventHubProperties GetEventHubsProperties( - Azure::Core::Amqp::_internal::Session const& session, + std::unique_ptr const& managementClient, std::string const& eventHubName, Core::Context const& context) { - - // Create a management client off the session. - // Eventhubs management APIs return a status code in the "status-code" application properties. - Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; - managementClientOptions.EnableTrace = EnableAmqpTrace; - managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; - Azure::Core::Amqp::_internal::ManagementClient managementClient{ - session.CreateManagementClient(eventHubName, managementClientOptions)}; - - managementClient.Open(context); - // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; message.ApplicationProperties["name"] = static_cast(eventHubName); message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = managementClient.ExecuteOperation( + auto result = managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:eventhub" /* type of operation */, "" /* locales */, @@ -167,27 +156,15 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail properties.PartitionIds.push_back(static_cast(partition)); } } - managementClient.Close(context); - return properties; } static Models::EventHubPartitionProperties GetEventHubsPartitionProperties( - Azure::Core::Amqp::_internal::Session const& session, + std::unique_ptr const& managementClient, std::string const& eventHubName, std::string const& partitionId, Core::Context const& context) { - // Create a management client off the session. - // Eventhubs management APIs return a status code in the "status-code" application properties. - Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; - managementClientOptions.EnableTrace = EnableAmqpTrace; - managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; - Azure::Core::Amqp::_internal::ManagementClient managementClient{ - session.CreateManagementClient(eventHubName, managementClientOptions)}; - - managementClient.Open(context); - // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; message.ApplicationProperties["name"] @@ -195,7 +172,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["partition"] = Azure::Core::Amqp::Models::AmqpValue{partitionId}; message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = managementClient.ExecuteOperation( + auto result = managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:partition" /* type of operation */, "" /* locales */, @@ -259,8 +236,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail .count())); properties.IsEmpty = bodyMap["is_partition_empty"]; } - managementClient.Close(context); - return properties; } diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp b/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp index b95e4a12b9..9167f81bfd 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp @@ -123,7 +123,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { } void ProducerClient::EnsureConnection(std::string const& partitionId) { - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); if (m_connections.find(partitionId) == m_connections.end()) { m_connections.emplace(partitionId, CreateConnection()); @@ -136,7 +136,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { EnsureConnection(partitionId); // Ensure that a session has been created for this partition. - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); if (m_sessions.find(partitionId) == m_sessions.end()) { m_sessions.emplace(partitionId, CreateSession(partitionId)); @@ -145,7 +145,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { Azure::Core::Amqp::_internal::Session ProducerClient::GetSession(std::string const& partitionId) { - std::unique_lock lock(m_sessionsLock); + std::unique_lock lock(m_sessionsLock); return m_sessions.at(partitionId); } @@ -190,24 +190,48 @@ namespace Azure { namespace Messaging { namespace EventHubs { return m_connections.at(partitionId).CreateSession(sessionOptions); } + void ProducerClient::EnsureManagementClient(Azure::Core::Context const& context) + { + EnsureSession({}); + + std::unique_lock lock(m_sessionsLock); + if (!m_managementClient) + { + // Create a management client off the session. + // Eventhubs management APIs return a status code in the "status-code" application properties. + Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; + managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; + managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; + m_managementClient = std::make_unique( + GetSession({}).CreateManagementClient(m_eventHub, managementClientOptions)); + + m_managementClient->Open(context); + m_isManagementClientOpen = true; + } + } + + std::unique_ptr const& + ProducerClient::GetManagementClient() + { + std::unique_lock lock(m_sessionsLock); + return m_managementClient; + } + Models::EventHubProperties ProducerClient::GetEventHubProperties(Core::Context const& context) { - EnsureConnection({}); + EnsureManagementClient(context); - EnsureSession({}); - auto session{GetSession({})}; - return _detail::EventHubsUtilities::GetEventHubsProperties(session, m_eventHub, context); + return _detail::EventHubsUtilities::GetEventHubsProperties( + GetManagementClient(), m_eventHub, context); } Models::EventHubPartitionProperties ProducerClient::GetPartitionProperties( std::string const& partitionId, Core::Context const& context) { - EnsureConnection(partitionId); - EnsureSession(partitionId); - Azure::Core::Amqp::_internal::Session session{GetSession(partitionId)}; + EnsureManagementClient(context); return _detail::EventHubsUtilities::GetEventHubsPartitionProperties( - session, m_eventHub, partitionId, context); + GetManagementClient(), m_eventHub, partitionId, context); } }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp index 67b60bedb4..35fe863e00 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp @@ -60,8 +60,8 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { std::string consumerGroup = GetEnv("EVENTHUB_CONSUMER_GROUP"); std::string eventHubName = GetEnv("EVENTHUB_NAME"); - // The eventHubName parameter must match the name in the connection string.because the eventhub - // name is in the connection string. + // The eventHubName parameter must match the name in the connection string.because the + // eventhub name is in the connection string. EXPECT_ANY_THROW(Azure::Messaging::EventHubs::ConsumerClient client( connStringWithEntityPath, eventHubName, "$DefaultZ")); } @@ -155,6 +155,122 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { EXPECT_EQ(result.Name, eventHubName); EXPECT_EQ(result.PartitionId, "0"); } + + TEST_F(ConsumerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) + { + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string const connString = GetEnv("EVENTHUB_CONNECTION_STRING"); + + Azure::Messaging::EventHubs::ConsumerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + Azure::Messaging::EventHubs::ConsumerClient client(connString, eventHubName); + Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; + partitionOptions.StartPosition.Inclusive = true; + + Azure::Messaging::EventHubs::PartitionClient partitionClient + = client.CreatePartitionClient("0", partitionOptions); + + std::vector threads; + std::vector iterationsPerThread; + for (int i = 0; i < 20; i++) + { + threads.emplace_back([&client, &partitionClient, i, eventHubName, &iterationsPerThread]() { + size_t iterations = 0; + std::chrono::system_clock::duration timeout = std::chrono::seconds(3); + std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); + while ((std::chrono::system_clock::now() - start) <= timeout) + { + Azure::Messaging::EventHubs::Models::EventHubProperties result; + ASSERT_NO_THROW(result = client.GetEventHubProperties()); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_TRUE(result.PartitionIds.size() > 0); + std::this_thread::yield(); + iterations++; + } + iterationsPerThread.push_back(iterations); + }); + } + GTEST_LOG_(INFO) << "Waiting for threads to finish."; + for (auto& t : threads) + { + if (t.joinable()) + { + t.join(); + } + } + GTEST_LOG_(INFO) << "Threads finished."; + for (const auto i : iterationsPerThread) + { + GTEST_LOG_(INFO) << "Thread iterations: " << i; + } + } + + TEST_F(ConsumerClientTest, GetPartitionProperties_Multithreaded_LIVEONLY_) + { + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string const connString = GetEnv("EVENTHUB_CONNECTION_STRING"); + + Azure::Messaging::EventHubs::ConsumerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + Azure::Messaging::EventHubs::ConsumerClient client(connString, eventHubName); + Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; + partitionOptions.StartPosition.Inclusive = true; + + Azure::Messaging::EventHubs::PartitionClient partitionClient + = client.CreatePartitionClient("0", partitionOptions); + + auto ehProperties = client.GetEventHubProperties(); + std::vector threads; + std::vector iterationsPerThread; + for (const auto& partition : ehProperties.PartitionIds) + { + threads.emplace_back(std::thread([&client, partition, eventHubName, &iterationsPerThread]() { + GTEST_LOG_(INFO) << "Thread started for partition: " << partition << ".\n"; + for (int i = 0; i < 20; i++) + { + std::vector partitionThreads; + partitionThreads.emplace_back( + [&client, &partition, i, eventHubName, &iterationsPerThread]() { + size_t iterations = 0; + std::chrono::system_clock::duration timeout = std::chrono::seconds(3); + std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); + while ((std::chrono::system_clock::now() - start) <= timeout) + { + Azure::Messaging::EventHubs::Models::EventHubPartitionProperties result; + ASSERT_NO_THROW(result = client.GetPartitionProperties(partition)); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_EQ(result.PartitionId, partition); + std::this_thread::yield(); + iterations++; + } + iterationsPerThread.push_back(iterations); + }); + for (auto& t : partitionThreads) + { + if (t.joinable()) + { + t.join(); + } + } + } + GTEST_LOG_(INFO) << "Thread finished for partition: " << partition << ".\n"; + })); + } + GTEST_LOG_(INFO) << "Waiting for threads to finish."; + for (auto& t : threads) + { + if (t.joinable()) + { + t.join(); + } + } + GTEST_LOG_(INFO) << iterationsPerThread.size() << " threads finished."; + } + std::string GetRandomName(const char* baseName = "checkpoint") { std::string name = baseName; @@ -190,8 +306,8 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { EXPECT_NO_THROW(producer.Send(batch)); } - // Now receive the messages - it should take almost no time because they should have been queued - // up asynchronously. + // Now receive the messages - it should take almost no time because they should have been + // queued up asynchronously. GTEST_LOG_(INFO) << "Receive events from instance."; { Azure::Messaging::EventHubs::ConsumerClientOptions options; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp index 3dbc184526..a01147fee3 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp @@ -121,7 +121,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { producerOptions.Name = "Producer for LoadBalancerTest"; ProducerClient producerClient{connectionString, eventHubName, producerOptions}; -#if PROCESSOR_ON_TEST_THREAD +#if TRUE std::thread processEventsThread([&]() { std::set partitionsAcquired; std::vector processEventsThreads; @@ -236,46 +236,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { producerOptions.Name = "Producer for LoadBalancerTest"; ProducerClient producerClient{connectionString, eventHubName, producerOptions}; -#if PROCESSOR_ON_TEST_THREAD - std::thread processEventsThread([&]() { - std::set partitionsAcquired; - std::vector processEventsThreads; - // When we exit the process thread, cancel the context to unblock the processor. - scope_guard onExit([&context] { context.Cancel(); }); - - WaitGroup waitGroup; - for (auto const& partitionId : eventHubProperties.PartitionIds) - { - std::shared_ptr partitionClient - = processor.NextPartitionClient(context); - waitGroup.AddWaiter(); - ASSERT_EQ(partitionsAcquired.find(partitionId), partitionsAcquired.end()) - << "No previous client for " << partitionClient->PartitionId(); - processEventsThreads.push_back( - std::thread([&waitGroup, &producerClient, partitionClient, &context, this] { - scope_guard onExit([&] { waitGroup.CompleteWaiter(); }); - ProcessEventsForLoadBalancerTest(producerClient, partitionClient, context); - })); - } - // Block until all the events have been processed. - waitGroup.Wait(); - - // And wait until all the threads have completed. - for (auto& thread : processEventsThreads) - { - if (thread.joinable()) - { - thread.join(); - } - } - // Stop the processor, we're done with the test. - processor.Stop(); - }); - - processor.Run(context); - - processEventsThread.join(); -#else // Run the processor on a background thread and the test on the foreground. processor.Start(context); @@ -292,7 +252,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { } // Stop the processor, we're done with the test. processor.Stop(); -#endif } void TestPartitionAcquisition(Models::ProcessorStrategy processorStrategy) From 14ac3599eb3240cde37b923b4d2dc0a3ad7016ba Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 12:35:00 -0800 Subject: [PATCH 02/24] Added partition client properties tests, removed unused variables in consumer client tests --- .../test/ut/consumer_client_test.cpp | 10 +- .../test/ut/producer_client_test.cpp | 105 ++++++++++++++++++ 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp index 35fe863e00..a9aa30822f 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp @@ -169,14 +169,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; partitionOptions.StartPosition.Inclusive = true; - Azure::Messaging::EventHubs::PartitionClient partitionClient - = client.CreatePartitionClient("0", partitionOptions); - std::vector threads; std::vector iterationsPerThread; for (int i = 0; i < 20; i++) { - threads.emplace_back([&client, &partitionClient, i, eventHubName, &iterationsPerThread]() { + threads.emplace_back([&client, i, eventHubName, &iterationsPerThread]() { size_t iterations = 0; std::chrono::system_clock::duration timeout = std::chrono::seconds(3); std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); @@ -217,11 +214,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); Azure::Messaging::EventHubs::ConsumerClient client(connString, eventHubName); - Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; - partitionOptions.StartPosition.Inclusive = true; - - Azure::Messaging::EventHubs::PartitionClient partitionClient - = client.CreatePartitionClient("0", partitionOptions); auto ehProperties = client.GetEventHubProperties(); std::vector threads; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp index 3fd41e62f4..e652934b83 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp @@ -146,3 +146,108 @@ TEST_F(ProducerClientTest, GetPartitionProperties_LIVEONLY_) EXPECT_EQ(result.PartitionId, "0"); }()); } + + TEST_F(ProducerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) +{ + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string const connString = GetEnv("EVENTHUB_CONNECTION_STRING"); + + Azure::Messaging::EventHubs::ProducerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + Azure::Messaging::EventHubs::ProducerClient client(connString, eventHubName); + + std::vector threads; + std::vector iterationsPerThread; + for (int i = 0; i < 20; i++) + { + threads.emplace_back([&client, i, eventHubName, &iterationsPerThread]() { + size_t iterations = 0; + std::chrono::system_clock::duration timeout = std::chrono::seconds(3); + std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); + while ((std::chrono::system_clock::now() - start) <= timeout) + { + Azure::Messaging::EventHubs::Models::EventHubProperties result; + ASSERT_NO_THROW(result = client.GetEventHubProperties()); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_TRUE(result.PartitionIds.size() > 0); + std::this_thread::yield(); + iterations++; + } + iterationsPerThread.push_back(iterations); + }); + } + GTEST_LOG_(INFO) << "Waiting for threads to finish."; + for (auto& t : threads) + { + if (t.joinable()) + { + t.join(); + } + } + GTEST_LOG_(INFO) << "Threads finished."; + for (const auto i : iterationsPerThread) + { + GTEST_LOG_(INFO) << "Thread iterations: " << i; + } +} + +TEST_F(ProducerClientTest, GetPartitionProperties_Multithreaded_LIVEONLY_) +{ + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string const connString = GetEnv("EVENTHUB_CONNECTION_STRING"); + + Azure::Messaging::EventHubs::ProducerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + Azure::Messaging::EventHubs::ProducerClient client(connString, eventHubName); + + auto ehProperties = client.GetEventHubProperties(); + std::vector threads; + std::vector iterationsPerThread; + for (const auto& partition : ehProperties.PartitionIds) + { + threads.emplace_back(std::thread([&client, partition, eventHubName, &iterationsPerThread]() { + GTEST_LOG_(INFO) << "Thread started for partition: " << partition << ".\n"; + for (int i = 0; i < 20; i++) + { + std::vector partitionThreads; + partitionThreads.emplace_back( + [&client, &partition, i, eventHubName, &iterationsPerThread]() { + size_t iterations = 0; + std::chrono::system_clock::duration timeout = std::chrono::seconds(3); + std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); + while ((std::chrono::system_clock::now() - start) <= timeout) + { + Azure::Messaging::EventHubs::Models::EventHubPartitionProperties result; + ASSERT_NO_THROW(result = client.GetPartitionProperties(partition)); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_EQ(result.PartitionId, partition); + std::this_thread::yield(); + iterations++; + } + iterationsPerThread.push_back(iterations); + }); + for (auto& t : partitionThreads) + { + if (t.joinable()) + { + t.join(); + } + } + } + GTEST_LOG_(INFO) << "Thread finished for partition: " << partition << ".\n"; + })); + } + GTEST_LOG_(INFO) << "Waiting for threads to finish."; + for (auto& t : threads) + { + if (t.joinable()) + { + t.join(); + } + } + GTEST_LOG_(INFO) << iterationsPerThread.size() << " threads finished."; +} From 72edbb8016e0ac162bb7fd3682570e9277d79a43 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 14:39:51 -0800 Subject: [PATCH 03/24] Restructured properties APIs in eventhubs to simplify producer client and consumer client implementations --- .../azure-core-amqp/src/amqp/management.cpp | 256 ++++++++++-------- .../src/amqp/private/management_impl.hpp | 2 +- .../messaging/eventhubs/consumer_client.hpp | 13 +- .../messaging/eventhubs/producer_client.hpp | 17 +- .../src/consumer_client.cpp | 55 +--- .../src/private/eventhubs_utilities.hpp | 119 +++++--- .../src/producer_client.cpp | 40 +-- .../test/ut/producer_client_test.cpp | 2 +- 8 files changed, 262 insertions(+), 242 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index db8cece810..ac6249d852 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -75,76 +75,95 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::ManagementOpenStatus ManagementClientImpl::Open(Context const& context) { - /** Authentication needs to happen *before* the links are created. - * - * Note that we ONLY enable authentication if we know we're talking to the management node. - * Other nodes require their own authentication. - */ - if (m_options.ManagementNodeName == "$management") - { - m_accessToken = m_session->GetConnection()->AuthenticateAudience( - m_session, m_managementEntityPath + "/" + m_options.ManagementNodeName, context); - } - { - _internal::MessageSenderOptions messageSenderOptions; - messageSenderOptions.EnableTrace = m_options.EnableTrace; - messageSenderOptions.MessageSource = m_options.ManagementNodeName; - messageSenderOptions.Name = m_options.ManagementNodeName + "-sender"; - messageSenderOptions.AuthenticationRequired = false; - - m_messageSender = std::make_shared( - m_session, m_options.ManagementNodeName, messageSenderOptions, this); - } - { - _internal::MessageReceiverOptions messageReceiverOptions; - messageReceiverOptions.EnableTrace = m_options.EnableTrace; - messageReceiverOptions.MessageTarget = m_options.ManagementNodeName; - messageReceiverOptions.Name = m_options.ManagementNodeName + "-receiver"; - messageReceiverOptions.AuthenticationRequired = false; - messageReceiverOptions.SettleMode = _internal::ReceiverSettleMode::First; - - m_messageReceiver = std::make_shared( - m_session, m_options.ManagementNodeName, messageReceiverOptions, this); - } - - // Now open the message sender and receiver. - SetState(ManagementState::Opening); try { - m_messageSender->Open(context); - m_messageSenderOpen = true; - m_messageReceiver->Open(context); - m_messageReceiverOpen = true; - } - catch (std::runtime_error const& e) - { - Log::Stream(Logger::Level::Warning) - << "Exception thrown opening message sender and receiver." << e.what(); - return _internal::ManagementOpenStatus::Error; - } + /** Authentication needs to happen *before* the links are created. + * + * Note that we ONLY enable authentication if we know we're talking to the management node. + * Other nodes require their own authentication. + */ + if (m_options.ManagementNodeName == "$management") + { + m_accessToken = m_session->GetConnection()->AuthenticateAudience( + m_session, m_managementEntityPath + "/" + m_options.ManagementNodeName, context); + } + { + _internal::MessageSenderOptions messageSenderOptions; + messageSenderOptions.EnableTrace = m_options.EnableTrace; + messageSenderOptions.MessageSource = m_options.ManagementNodeName; + messageSenderOptions.Name = m_options.ManagementNodeName + "-sender"; + messageSenderOptions.AuthenticationRequired = false; + + m_messageSender = std::make_shared( + m_session, m_options.ManagementNodeName, messageSenderOptions, this); + } + { + _internal::MessageReceiverOptions messageReceiverOptions; + messageReceiverOptions.EnableTrace = m_options.EnableTrace; + messageReceiverOptions.MessageTarget = m_options.ManagementNodeName; + messageReceiverOptions.Name = m_options.ManagementNodeName + "-receiver"; + messageReceiverOptions.AuthenticationRequired = false; + messageReceiverOptions.SettleMode = _internal::ReceiverSettleMode::First; + + m_messageReceiver = std::make_shared( + m_session, m_options.ManagementNodeName, messageReceiverOptions, this); + } - // And finally, wait for the message sender and receiver to finish opening before we return. - auto result = m_openCompleteQueue.WaitForResult(context); - if (result) + // Now open the message sender and receiver. + SetState(ManagementState::Opening); + try + { + m_messageSender->Open(context); + m_messageSenderOpen = true; + m_messageReceiver->Open(context); + m_messageReceiverOpen = true; + } + catch (std::runtime_error const& e) + { + Log::Stream(Logger::Level::Warning) + << "Exception thrown opening message sender and receiver." << e.what(); + return _internal::ManagementOpenStatus::Error; + } + + // And finally, wait for the message sender and receiver to finish opening before we return. + auto result = m_openCompleteQueue.WaitForResult(context); + if (result) + { + // If the message sender or receiver failed to open, we need to close them + _internal::ManagementOpenStatus rv = std::get<0>(*result); + if (rv != _internal::ManagementOpenStatus::Ok) + { + Log::Stream(Logger::Level::Warning) << "Management operation failed to open."; + m_messageSender->Close(context); + m_messageSenderOpen = false; + m_messageReceiver->Close(context); + m_messageReceiverOpen = false; + } + else + { + m_isOpen = true; + } + return rv; + } + // If result is null, then it means that the context was cancelled. + return _internal::ManagementOpenStatus::Cancelled; + } + catch (...) { - // If the message sender or receiver failed to open, we need to close them - _internal::ManagementOpenStatus rv = std::get<0>(*result); - if (rv != _internal::ManagementOpenStatus::Ok) + Log::Stream(Logger::Level::Warning) << "Exception thrown during management open."; + // If an exception is thrown, ensure that the message sender and receiver are closed. + if (m_messageSenderOpen) { - Log::Stream(Logger::Level::Warning) << "Management operation failed to open."; - m_messageSender->Close(context); + m_messageSender->Close({}); m_messageSenderOpen = false; - m_messageReceiver->Close(context); - m_messageReceiverOpen = false; } - else + if (m_messageReceiverOpen) { - m_isOpen = true; + m_messageReceiver->Close({}); + m_messageReceiverOpen = false; } - return rv; + throw; } - // If result is null, then it means that the context was cancelled. - return _internal::ManagementOpenStatus::Cancelled; } _internal::ManagementOperationResult ManagementClientImpl::ExecuteOperation( @@ -154,68 +173,87 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Models::AmqpMessage messageToSend, Context const& context) { - // If the connection is authenticated, include the token in the message. - if (!m_accessToken.Token.empty()) - { - messageToSend.ApplicationProperties["security_token"] - = Models::AmqpValue{m_accessToken.Token}; - } - messageToSend.ApplicationProperties.emplace("operation", operationToPerform); - messageToSend.ApplicationProperties.emplace("type", typeOfOperation); - if (!locales.empty()) + try { - messageToSend.ApplicationProperties.emplace("locales", locales); - } + // If the connection is authenticated, include the token in the message. + if (!m_accessToken.Token.empty()) + { + messageToSend.ApplicationProperties["security_token"] + = Models::AmqpValue{m_accessToken.Token}; + } + messageToSend.ApplicationProperties.emplace("operation", operationToPerform); + messageToSend.ApplicationProperties.emplace("type", typeOfOperation); + if (!locales.empty()) + { + messageToSend.ApplicationProperties.emplace("locales", locales); + } - // Set the message ID and remember it for later. - auto requestId = Azure::Core::Uuid::CreateUuid().ToString(); + // Set the message ID and remember it for later. + auto requestId = Azure::Core::Uuid::CreateUuid().ToString(); - messageToSend.Properties.MessageId - = static_cast(requestId); - { - std::unique_lock lock(m_messageQueuesLock); + messageToSend.Properties.MessageId + = static_cast(requestId); + { + std::unique_lock lock(m_messageQueuesLock); - Log::Stream(Logger::Level::Verbose) - << "ManagementClient::ExecuteOperation: " << requestId << ". Create Queue for request."; - m_messageQueues.emplace(requestId, std::make_unique()); - m_sendCompleted = false; - } + Log::Stream(Logger::Level::Verbose) + << "ManagementClient::ExecuteOperation: " << requestId << ". Create Queue for request."; + m_messageQueues.emplace(requestId, std::make_unique()); + m_sendCompleted = false; + } - auto sendResult = m_messageSender->Send(messageToSend, context); - if (std::get<0>(sendResult) != _internal::MessageSendStatus::Ok) - { - _internal::ManagementOperationResult rv; - rv.Status = _internal::ManagementOperationStatus::Error; - rv.StatusCode = 500; - rv.Error = std::get<1>(sendResult); - rv.Message = nullptr; + auto sendResult = m_messageSender->Send(messageToSend, context); + if (std::get<0>(sendResult) != _internal::MessageSendStatus::Ok) { - std::unique_lock lock(m_messageQueuesLock); - // Remove the queue from the map, we don't need it anymore. - m_messageQueues.erase(requestId); + _internal::ManagementOperationResult rv; + rv.Status = _internal::ManagementOperationStatus::Error; + rv.StatusCode = 500; + rv.Error = std::get<1>(sendResult); + rv.Message = nullptr; + { + std::unique_lock lock(m_messageQueuesLock); + // Remove the queue from the map, we don't need it anymore. + m_messageQueues.erase(requestId); + } + return rv; } - return rv; - } - auto result = m_messageQueues.at(requestId)->WaitForResult(context); - if (result) - { - _internal::ManagementOperationResult rv; - rv.Status = std::get<0>(*result); - rv.StatusCode = std::get<1>(*result); - rv.Error = std::get<2>(*result); - rv.Message = std::get<3>(*result); + auto result = m_messageQueues.at(requestId)->WaitForResult(context); + if (result) + { + _internal::ManagementOperationResult rv; + rv.Status = std::get<0>(*result); + rv.StatusCode = std::get<1>(*result); + rv.Error = std::get<2>(*result); + rv.Message = std::get<3>(*result); + { + std::unique_lock lock(m_messageQueuesLock); + // Remove the queue from the map, we don't need it anymore. + m_messageQueues.erase(requestId); + } + return rv; + } + else { - std::unique_lock lock(m_messageQueuesLock); - // Remove the queue from the map, we don't need it anymore. - m_messageQueues.erase(requestId); + throw Azure::Core::OperationCancelledException("Management operation cancelled."); } - return rv; } - else + catch (...) { - throw Azure::Core::OperationCancelledException("Management operation cancelled."); + Log::Stream(Logger::Level::Error) << "ManagementClient::ExecuteOperation: Exception thrown. " + "Closing message sender and receiver."; + if (m_messageSenderOpen) + { + m_messageSender->Close({}); + m_messageSenderOpen = false; + } + if (m_messageReceiverOpen) + { + m_messageReceiver->Close({}); + m_messageReceiverOpen = false; + } + throw; } } diff --git a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp index cf510b4ec9..ce3f05cce0 100644 --- a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp @@ -16,8 +16,8 @@ #include #include -#include #include +#include #include namespace Azure { namespace Core { namespace Amqp { namespace _detail { diff --git a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp index 47e34b83d4..81b706308e 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/consumer_client.hpp @@ -10,13 +10,15 @@ #include #include -#include #include #include #include #include #include namespace Azure { namespace Messaging { namespace EventHubs { + namespace _detail { + class EventHubsPropertiesClient; + } /// @brief The default consumer group name. constexpr const char* DefaultConsumerGroup = "$Default"; @@ -192,19 +194,18 @@ namespace Azure { namespace Messaging { namespace EventHubs { std::map m_sessions; std::map m_connections; - // Management client used for GetProperty operations. - bool m_managementClientIsOpen{false}; - std::unique_ptr m_managementClient; + // Client used for GetProperty operations. + std::mutex m_propertiesClientLock; + std::shared_ptr<_detail::EventHubsPropertiesClient> m_propertiesClient; /// @brief The options used to configure the consumer client. ConsumerClientOptions m_consumerClientOptions; void EnsureConnection(std::string const& partitionId); void EnsureSession(std::string const& partitionId); - void EnsureManagementClient(const Azure::Core::Context& context = {}); Azure::Core::Amqp::_internal::Connection CreateConnection(std::string const& partitionId) const; Azure::Core::Amqp::_internal::Session CreateSession(std::string const& partitionId) const; Azure::Core::Amqp::_internal::Session GetSession(std::string const& partitionId); - std::unique_ptr const& GetManagementClient(); + std::shared_ptr<_detail::EventHubsPropertiesClient> GetPropertiesClient(); }; }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp index 1dc92959fb..b683904afe 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/producer_client.hpp @@ -8,7 +8,6 @@ #include "models/management_models.hpp" #include -#include #include #include #include @@ -17,6 +16,9 @@ #include namespace Azure { namespace Messaging { namespace EventHubs { + namespace _detail { + class EventHubsPropertiesClient; + } // namespace _detail /**@brief Contains options for the ProducerClient creation */ @@ -92,11 +94,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { ~ProducerClient() { - if (m_managementClient && m_isManagementClientOpen) - { - m_managementClient->Close(); - } - for (auto& sender : m_senders) { sender.second.Close(); @@ -182,12 +179,13 @@ namespace Azure { namespace Messaging { namespace EventHubs { std::mutex m_sendersLock; std::map m_connections{}; std::map m_senders{}; - std::unique_ptr m_managementClient; - bool m_isManagementClientOpen{false}; std::recursive_mutex m_sessionsLock; std::map m_sessions{}; + std::mutex m_propertiesClientLock; + std::shared_ptr<_detail::EventHubsPropertiesClient> m_propertiesClient; + Azure::Core::Amqp::_internal::Connection CreateConnection() const; Azure::Core::Amqp::_internal::Session CreateSession(std::string const& partitionId); @@ -200,10 +198,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { // Ensure that a message sender for the specified partition has been created. void EnsureSender(std::string const& partitionId, Azure::Core::Context const& context = {}); - void EnsureManagementClient(Azure::Core::Context const& context = {}); + std::shared_ptr<_detail::EventHubsPropertiesClient> GetPropertiesClient(); Azure::Core::Amqp::_internal::MessageSender GetSender(std::string const& partitionId); Azure::Core::Amqp::_internal::Session GetSession(std::string const& partitionId); - std::unique_ptr const& GetManagementClient(); }; }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp b/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp index d28ff12e59..d72a377839 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/consumer_client.cpp @@ -57,10 +57,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { { sender.second.Close(); } - if (m_managementClientIsOpen) - { - m_managementClient->Close(); - } while (!m_sessions.empty()) { m_sessions.erase(m_sessions.begin()); @@ -118,34 +114,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { } } - void ConsumerClient::EnsureManagementClient(Azure::Core::Context const& context) - { - EnsureSession({}); - auto session{GetSession({})}; - - std::unique_lock lock(m_sessionsLock); - if (!m_managementClient) - { - // Create a management client off the session. - // Eventhubs management APIs return a status code in the "status-code" application properties. - Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; - managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; - managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; - m_managementClient = std::make_unique( - session.CreateManagementClient(m_eventHub, managementClientOptions)); - - m_managementClient->Open(context); - m_managementClientIsOpen = true; - } - } - - std::unique_ptr const& - ConsumerClient::GetManagementClient() - { - std::unique_lock lock(m_sessionsLock); - return m_managementClient; - } - Azure::Core::Amqp::_internal::Session ConsumerClient::GetSession( std::string const& partitionId = {}) { @@ -153,6 +121,18 @@ namespace Azure { namespace Messaging { namespace EventHubs { return m_sessions.at(partitionId); } + std::shared_ptr<_detail::EventHubsPropertiesClient> ConsumerClient::GetPropertiesClient() + { + std::lock_guard lock(m_propertiesClientLock); + EnsureConnection({}); + if (!m_propertiesClient) + { + m_propertiesClient + = std::make_shared<_detail::EventHubsPropertiesClient>(m_connections.at(""), m_eventHub); + } + return m_propertiesClient; + } + PartitionClient ConsumerClient::CreatePartitionClient( std::string const& partitionId, PartitionClientOptions const& options, @@ -174,20 +154,13 @@ namespace Azure { namespace Messaging { namespace EventHubs { Models::EventHubProperties ConsumerClient::GetEventHubProperties(Core::Context const& context) { - // Since EventHub properties are not tied to a partition, we don't specify a partition ID. - EnsureManagementClient(context); - - return _detail::EventHubsUtilities::GetEventHubsProperties( - GetManagementClient(), m_eventHub, context); + return GetPropertiesClient()->GetEventHubsProperties(m_eventHub, context); } Models::EventHubPartitionProperties ConsumerClient::GetPartitionProperties( std::string const& partitionId, Core::Context const& context) { - EnsureManagementClient(context); - - return _detail::EventHubsUtilities::GetEventHubsPartitionProperties( - GetManagementClient(), m_eventHub, partitionId, context); + return GetPropertiesClient()->GetEventHubsPartitionProperties(m_eventHub, partitionId, context); } }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index 8f1d961499..4748c03822 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -10,6 +10,7 @@ #include "azure/messaging/eventhubs/partition_client.hpp" #include "package_version.hpp" +#include #include #include #include @@ -83,39 +84,33 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail PartitionClientFactory() = delete; }; - class EventHubsUtilities { - + class EventHubsPropertiesClient { public: - template static void SetUserAgent(T& options, std::string const& applicationId) - { - constexpr const char* packageName = "cpp-azure-messaging-eventhubs-cpp"; + EventHubsPropertiesClient( + const Azure::Core::Amqp::_internal::Connection& connection, + std::string eventHubName) + : m_session{connection.CreateSession()}, m_eventHub{eventHubName} {}; - options.Properties.emplace("Product", +packageName); - options.Properties.emplace("Version", PackageVersion::ToString()); -#if defined(AZ_PLATFORM_WINDOWS) - options.Properties.emplace("Platform", "Windows"); -#elif defined(AZ_PLATFORM_LINUX) - options.Properties.emplace("Platform", "Linux"); -#elif defined(AZ_PLATFORM_MAC) - options.Properties.emplace("Platform", "Mac"); -#endif - options.Properties.emplace( - "User-Agent", - Azure::Core::Http::_detail::UserAgentGenerator::GenerateUserAgent( - packageName, PackageVersion::ToString(), applicationId)); + ~EventHubsPropertiesClient() + { + if (m_managementClientIsOpen) + { + m_managementClient->Close(); + } } - static Models::EventHubProperties GetEventHubsProperties( - std::unique_ptr const& managementClient, + Models::EventHubProperties GetEventHubsProperties( std::string const& eventHubName, Core::Context const& context) { + EnsureManagementClient(context); + // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; message.ApplicationProperties["name"] = static_cast(eventHubName); message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = managementClient->ExecuteOperation( + auto result = m_managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:eventhub" /* type of operation */, "" /* locales */, @@ -159,12 +154,13 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail return properties; } - static Models::EventHubPartitionProperties GetEventHubsPartitionProperties( - std::unique_ptr const& managementClient, + Models::EventHubPartitionProperties GetEventHubsPartitionProperties( std::string const& eventHubName, std::string const& partitionId, Core::Context const& context) { + EnsureManagementClient(context); + // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; message.ApplicationProperties["name"] @@ -172,7 +168,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["partition"] = Azure::Core::Amqp::Models::AmqpValue{partitionId}; message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = managementClient->ExecuteOperation( + auto result = m_managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:partition" /* type of operation */, "" /* locales */, @@ -210,25 +206,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail properties.LastEnqueuedOffset = std::strtoull( static_cast(bodyMap["last_enqueued_offset"]).c_str(), nullptr, 10); - Azure::Core::Diagnostics::_internal::Log::Stream( - Azure::Core::Diagnostics::Logger::Level::Informational) - << "last enqueued time utc: " << bodyMap["last_enqueued_time_utc"]; - Azure::Core::Diagnostics::_internal::Log::Stream( - Azure::Core::Diagnostics::Logger::Level::Informational) - << "last enqueued time utc: " - << static_cast( - bodyMap["last_enqueued_time_utc"].AsTimestamp()) - .count() - << " ms"; - Azure::Core::Diagnostics::_internal::Log::Stream( - Azure::Core::Diagnostics::Logger::Level::Informational) - << "last enqueued time utc: " - << std::chrono::duration_cast( - static_cast( - bodyMap["last_enqueued_time_utc"].AsTimestamp())) - .count() - << " s"; - properties.LastEnqueuedTimeUtc = Azure::DateTime(std::chrono::system_clock::from_time_t( std::chrono::duration_cast( static_cast( @@ -239,6 +216,62 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail return properties; } + private: + void EnsureManagementClient(Azure::Core::Context const& context) + { + + std::unique_lock lock(m_managementClientMutex); + if (!m_managementClient) + { + // Create a management client off the session. + // Eventhubs management APIs return a status code in the "status-code" application + // properties. + Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; + managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; + managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; + m_managementClient = std::make_unique( + m_session.CreateManagementClient(m_eventHub, managementClientOptions)); + + m_managementClient->Open(context); + m_managementClientIsOpen = true; + } + } + + std::unique_ptr const& GetManagementClient() + { + std::unique_lock lock(m_managementClientMutex); + return m_managementClient; + } + + std::mutex m_managementClientMutex; + Azure::Core::Amqp::_internal::Session m_session; + std::unique_ptr m_managementClient; + bool m_managementClientIsOpen{false}; + std::string m_eventHub; + }; + + class EventHubsUtilities { + + public: + template static void SetUserAgent(T& options, std::string const& applicationId) + { + constexpr const char* packageName = "cpp-azure-messaging-eventhubs-cpp"; + + options.Properties.emplace("Product", +packageName); + options.Properties.emplace("Version", PackageVersion::ToString()); +#if defined(AZ_PLATFORM_WINDOWS) + options.Properties.emplace("Platform", "Windows"); +#elif defined(AZ_PLATFORM_LINUX) + options.Properties.emplace("Platform", "Linux"); +#elif defined(AZ_PLATFORM_MAC) + options.Properties.emplace("Platform", "Mac"); +#endif + options.Properties.emplace( + "User-Agent", + Azure::Core::Http::_detail::UserAgentGenerator::GenerateUserAgent( + packageName, PackageVersion::ToString(), applicationId)); + } + static void LogRawBuffer(std::ostream& os, std::vector const& buffer); ~EventHubsUtilities() = delete; }; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp b/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp index 9167f81bfd..e6224d03f2 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp @@ -189,49 +189,27 @@ namespace Azure { namespace Messaging { namespace EventHubs { sessionOptions.InitialOutgoingWindowSize = std::numeric_limits::max(); return m_connections.at(partitionId).CreateSession(sessionOptions); } - - void ProducerClient::EnsureManagementClient(Azure::Core::Context const& context) + std::shared_ptr<_detail::EventHubsPropertiesClient> ProducerClient::GetPropertiesClient() { - EnsureSession({}); - - std::unique_lock lock(m_sessionsLock); - if (!m_managementClient) + std::lock_guard lock(m_propertiesClientLock); + EnsureConnection({}); + if (!m_propertiesClient) { - // Create a management client off the session. - // Eventhubs management APIs return a status code in the "status-code" application properties. - Azure::Core::Amqp::_internal::ManagementClientOptions managementClientOptions; - managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; - managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; - m_managementClient = std::make_unique( - GetSession({}).CreateManagementClient(m_eventHub, managementClientOptions)); - - m_managementClient->Open(context); - m_isManagementClientOpen = true; + m_propertiesClient + = std::make_shared<_detail::EventHubsPropertiesClient>(m_connections.at(""), m_eventHub); } - } - - std::unique_ptr const& - ProducerClient::GetManagementClient() - { - std::unique_lock lock(m_sessionsLock); - return m_managementClient; + return m_propertiesClient; } Models::EventHubProperties ProducerClient::GetEventHubProperties(Core::Context const& context) { - EnsureManagementClient(context); - - return _detail::EventHubsUtilities::GetEventHubsProperties( - GetManagementClient(), m_eventHub, context); + return GetPropertiesClient()->GetEventHubsProperties(m_eventHub, context); } Models::EventHubPartitionProperties ProducerClient::GetPartitionProperties( std::string const& partitionId, Core::Context const& context) { - EnsureManagementClient(context); - - return _detail::EventHubsUtilities::GetEventHubsPartitionProperties( - GetManagementClient(), m_eventHub, partitionId, context); + return GetPropertiesClient()->GetEventHubsPartitionProperties(m_eventHub, partitionId, context); } }}} // namespace Azure::Messaging::EventHubs diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp index e652934b83..9f3479b4a8 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp @@ -147,7 +147,7 @@ TEST_F(ProducerClientTest, GetPartitionProperties_LIVEONLY_) }()); } - TEST_F(ProducerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) +TEST_F(ProducerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) { std::string eventHubName{GetEnv("EVENTHUB_NAME")}; std::string const connString = GetEnv("EVENTHUB_CONNECTION_STRING"); From 40dfecbf109e990a48af9ba7cdabd29d5d1ecc20 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 15:11:10 -0800 Subject: [PATCH 04/24] Clang fix --- .../azure-messaging-eventhubs/test/ut/consumer_client_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp index a9aa30822f..c8aa2b7f5b 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp @@ -226,7 +226,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { { std::vector partitionThreads; partitionThreads.emplace_back( - [&client, &partition, i, eventHubName, &iterationsPerThread]() { + [&client, &partition, eventHubName, &iterationsPerThread]() { size_t iterations = 0; std::chrono::system_clock::duration timeout = std::chrono::seconds(3); std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); From 1b7f2a6fedeace7e0fafb775073f72af1f7996f4 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 15:55:18 -0800 Subject: [PATCH 05/24] Removed AMQP code which logged an incoming management message --- sdk/core/azure-core-amqp/src/amqp/management.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index ac6249d852..7c45be7530 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -548,10 +548,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::MessageReceiver const&, std::shared_ptr const& message) { - if (m_options.EnableTrace) - { - Log::Stream(Logger::Level::Informational) << "Received message: " << *message; - } if (!message->Properties.CorrelationId.HasValue()) { return IndicateError( From d1a41e7231720348467578814826bdf2982e4cb5 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 2 Feb 2024 16:08:22 -0800 Subject: [PATCH 06/24] Removed unused lambda capture fields. --- .../test/ut/consumer_client_test.cpp | 2 +- .../test/ut/producer_client_test.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp index c8aa2b7f5b..484f804a3e 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp @@ -173,7 +173,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { std::vector iterationsPerThread; for (int i = 0; i < 20; i++) { - threads.emplace_back([&client, i, eventHubName, &iterationsPerThread]() { + threads.emplace_back([&client, eventHubName, &iterationsPerThread]() { size_t iterations = 0; std::chrono::system_clock::duration timeout = std::chrono::seconds(3); std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp index 9f3479b4a8..4b0a219de7 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp @@ -162,7 +162,7 @@ TEST_F(ProducerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) std::vector iterationsPerThread; for (int i = 0; i < 20; i++) { - threads.emplace_back([&client, i, eventHubName, &iterationsPerThread]() { + threads.emplace_back([&client, eventHubName, &iterationsPerThread]() { size_t iterations = 0; std::chrono::system_clock::duration timeout = std::chrono::seconds(3); std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); @@ -215,7 +215,7 @@ TEST_F(ProducerClientTest, GetPartitionProperties_Multithreaded_LIVEONLY_) { std::vector partitionThreads; partitionThreads.emplace_back( - [&client, &partition, i, eventHubName, &iterationsPerThread]() { + [&client, &partition, eventHubName, &iterationsPerThread]() { size_t iterations = 0; std::chrono::system_clock::duration timeout = std::chrono::seconds(3); std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); From 84a54295aa58c54964bb622235f243318ecb1d84 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 5 Feb 2024 10:50:34 -0800 Subject: [PATCH 07/24] Fixed test crash in LinkAttachDetach AMQP test --- sdk/core/azure-core-amqp/src/amqp/link.cpp | 16 +++------------- .../vendor/azure-uamqp-c/src/session.c | 5 ++++- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/link.cpp b/sdk/core/azure-core-amqp/src/amqp/link.cpp index ebe4cb51d1..8dd9ae7a84 100644 --- a/sdk/core/azure-core-amqp/src/amqp/link.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/link.cpp @@ -209,7 +209,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Azure::Core::_internal::AzureNoReturnPath( "Destroying link while link detach subscription is still active."); } - + auto lock{m_session->GetConnection()->Lock()}; if (m_link) { link_destroy(m_link); @@ -512,19 +512,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { { { auto lock{m_session->GetConnection()->Lock()}; - if (m_eventHandler) + if (link_attach(m_link, OnTransferReceivedFn, OnLinkStateChangedFn, OnLinkFlowOnFn, this)) { - if (link_attach(m_link, OnTransferReceivedFn, OnLinkStateChangedFn, OnLinkFlowOnFn, this)) - { - throw std::runtime_error("Could not set attach properties."); - } - } - else - { - if (link_attach(m_link, nullptr, nullptr, nullptr, this)) - { - throw std::runtime_error("Could not set attach properties."); - } + throw std::runtime_error("Could not set attach properties."); } } // Mark the connection as async so that we can use the async APIs. diff --git a/sdk/core/azure-core-amqp/vendor/azure-uamqp-c/src/session.c b/sdk/core/azure-core-amqp/vendor/azure-uamqp-c/src/session.c index b6556e4e3a..ee7726ec23 100644 --- a/sdk/core/azure-core-amqp/vendor/azure-uamqp-c/src/session.c +++ b/sdk/core/azure-core-amqp/vendor/azure-uamqp-c/src/session.c @@ -616,7 +616,10 @@ static void on_frame_received(void* context, AMQP_VALUE performative, uint32_t p if (link_endpoint->link_endpoint_state != LINK_ENDPOINT_STATE_DETACHING) { link_endpoint->link_endpoint_state = LINK_ENDPOINT_STATE_DETACHING; - link_endpoint->frame_received_callback(link_endpoint->callback_context, performative, payload_size, payload_bytes); + if (link_endpoint->frame_received_callback) + { + link_endpoint->frame_received_callback(link_endpoint->callback_context, performative, payload_size, payload_bytes); + } } else { From 8a4c96e4ee9956eabeb84465ab7ca956545ab335 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 5 Feb 2024 15:19:34 -0800 Subject: [PATCH 08/24] Added test cases for management authn failures --- .../src/amqp/claim_based_security.cpp | 2 +- .../azure-core-amqp/src/amqp/connection.cpp | 2 +- .../test/ut/management_tests.cpp | 41 +++++++++++++ .../test/ut/mock_amqp_server.hpp | 4 +- .../test/ut/consumer_client_test.cpp | 58 +++++++++++++++++++ 5 files changed, 103 insertions(+), 4 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp index eb1cc05df1..62ac646ccb 100644 --- a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp @@ -103,7 +103,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { context); if (result.Status != ManagementOperationStatus::Ok) { - throw std::runtime_error( + throw Azure::Core::Credentials::AuthenticationException( "Could not authenticate to client. Error Status: " + std::to_string(result.StatusCode) + " condition: " + result.Error.Condition.ToString() + " reason: " + result.Error.Description); diff --git a/sdk/core/azure-core-amqp/src/amqp/connection.cpp b/sdk/core/azure-core-amqp/src/amqp/connection.cpp index b7d8ecd730..07c1620e55 100644 --- a/sdk/core/azure-core-amqp/src/amqp/connection.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/connection.cpp @@ -690,7 +690,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { m_tokenStore.emplace(audienceUrl, accessToken); return accessToken; } - catch (std::runtime_error const&) + catch (...) { // Ensure that the claims based security object is closed before we leave this scope. claimsBasedSecurity->Close(context); diff --git a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp index 922c02c9fa..f505fff010 100644 --- a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp @@ -196,6 +196,47 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { mockServer.StopListening(); } + TEST_F(TestManagement, ManagementOpenCloseAuthenticatedFail) + { + MessageTests::AmqpServerMock mockServer; + MessageTests::MockServiceEndpointOptions managementEndpointOptions; + managementEndpointOptions.EnableTrace = true; + auto endpoint = std::make_shared(managementEndpointOptions); + mockServer.AddServiceEndpoint(endpoint); + + auto sasCredential = std::make_shared( + "Endpoint=amqp://localhost:" + std::to_string(mockServer.GetPort()) + + "/;SharedAccessKeyName=MyTestKey;SharedAccessKey=abcdabcd;EntityPath=testLocation"); + + ConnectionOptions connectionOptions; + connectionOptions.Port = mockServer.GetPort(); + connectionOptions.EnableTrace = true; + Connection connection("localhost", sasCredential, connectionOptions); + + Session session{connection.CreateSession({})}; + ManagementClientOptions options; + options.EnableTrace = 1; + ManagementClient management(session.CreateManagementClient("Test", options)); + + // Force an authentication error. + mockServer.ForceCbsError(true); + + mockServer.StartListening(); + + try + { + EXPECT_THROW(management.Open(), Azure::Core::Credentials::AuthenticationException); + + management.Close(); + } + catch (std::exception const& e) + { + GTEST_LOG_(INFO) << "Caught exception: " << e.what(); + } + mockServer.StopListening(); + + } + TEST_F(TestManagement, ManagementOpenCloseError) { MessageTests::AmqpServerMock mockServer; diff --git a/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp b/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp index 6ead71ed81..c7389ad98a 100644 --- a/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp +++ b/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp @@ -57,8 +57,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { public Azure::Core::Amqp::_internal::MessageSenderEvents { public: MockServiceEndpoint(std::string const& name, MockServiceEndpointOptions const& options) - : m_listenerContext{options.ListenerContext}, - m_enableTrace{options.EnableTrace}, m_name{name} + : m_listenerContext{options.ListenerContext}, m_enableTrace{options.EnableTrace}, + m_name{name} { } diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp index 484f804a3e..f7817c5bfc 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/consumer_client_test.cpp @@ -156,6 +156,64 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { EXPECT_EQ(result.PartitionId, "0"); } + TEST_F(ConsumerClientTest, GetPartitionPropertiesClientSecret_LIVEONLY_) + { + auto credentials + { +#if 0 + std::make_shared( + GetEnv("EVENTHUBS_TENANT_ID"), + GetEnv("EVENTHUBS_CLIENT_ID"), + GetEnv("EVENTHUBS_CLIENT_SECRET")) +#else + std::make_shared() +#endif + }; + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string hostName{GetEnv("EVENTHUBS_HOST")}; + std::string consumerGroup{GetEnv("EVENTHUB_CONSUMER_GROUP")}; + + Azure::Messaging::EventHubs::ConsumerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + + Azure::Messaging::EventHubs::ConsumerClient client( + hostName, eventHubName, credentials, consumerGroup); + Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; + partitionOptions.StartPosition.Inclusive = true; + + Azure::Messaging::EventHubs::PartitionClient partitionClient + = client.CreatePartitionClient("0", partitionOptions); + + auto result = client.GetPartitionProperties("0"); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_EQ(result.PartitionId, "0"); + } + + TEST_F(ConsumerClientTest, GetPartitionPropertiesAuthError_LIVEONLY_) + { + auto credentials{ + std::make_shared("abc", "def", "ghi")}; + std::string eventHubName{GetEnv("EVENTHUB_NAME")}; + std::string hostName{GetEnv("EVENTHUBS_HOST")}; + std::string consumerGroup{GetEnv("EVENTHUB_CONSUMER_GROUP")}; + + Azure::Messaging::EventHubs::ConsumerClientOptions options; + options.ApplicationID = testing::UnitTest::GetInstance()->current_test_info()->name(); + + options.Name = testing::UnitTest::GetInstance()->current_test_case()->name(); + + Azure::Messaging::EventHubs::ConsumerClient client( + hostName, eventHubName, credentials, consumerGroup); + Azure::Messaging::EventHubs::PartitionClientOptions partitionOptions; + partitionOptions.StartPosition.Inclusive = true; + + EXPECT_THROW( + client.CreatePartitionClient("0", partitionOptions), + Azure::Core::Credentials::AuthenticationException); + } + TEST_F(ConsumerClientTest, GetEventHubProperties_Multithreaded_LIVEONLY_) { std::string eventHubName{GetEnv("EVENTHUB_NAME")}; From 2ac800f1434ba9a49f0d7b661bee754b64e2130c Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 5 Feb 2024 15:39:21 -0800 Subject: [PATCH 09/24] clang-format --- .../test/ut/management_tests.cpp | 5 ++- .../test/ut/mock_amqp_server.hpp | 4 +-- .../test/ut/producer_client_test.cpp | 31 +++++++++---------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp index f505fff010..65c5d6c5bf 100644 --- a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp @@ -231,10 +231,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { } catch (std::exception const& e) { - GTEST_LOG_(INFO) << "Caught exception: " << e.what(); - } + GTEST_LOG_(INFO) << "Caught exception: " << e.what(); + } mockServer.StopListening(); - } TEST_F(TestManagement, ManagementOpenCloseError) diff --git a/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp b/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp index c7389ad98a..6ead71ed81 100644 --- a/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp +++ b/sdk/core/azure-core-amqp/test/ut/mock_amqp_server.hpp @@ -57,8 +57,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { public Azure::Core::Amqp::_internal::MessageSenderEvents { public: MockServiceEndpoint(std::string const& name, MockServiceEndpointOptions const& options) - : m_listenerContext{options.ListenerContext}, m_enableTrace{options.EnableTrace}, - m_name{name} + : m_listenerContext{options.ListenerContext}, + m_enableTrace{options.EnableTrace}, m_name{name} { } diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp index 4b0a219de7..9a44b7b7c4 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp @@ -214,22 +214,21 @@ TEST_F(ProducerClientTest, GetPartitionProperties_Multithreaded_LIVEONLY_) for (int i = 0; i < 20; i++) { std::vector partitionThreads; - partitionThreads.emplace_back( - [&client, &partition, eventHubName, &iterationsPerThread]() { - size_t iterations = 0; - std::chrono::system_clock::duration timeout = std::chrono::seconds(3); - std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); - while ((std::chrono::system_clock::now() - start) <= timeout) - { - Azure::Messaging::EventHubs::Models::EventHubPartitionProperties result; - ASSERT_NO_THROW(result = client.GetPartitionProperties(partition)); - EXPECT_EQ(result.Name, eventHubName); - EXPECT_EQ(result.PartitionId, partition); - std::this_thread::yield(); - iterations++; - } - iterationsPerThread.push_back(iterations); - }); + partitionThreads.emplace_back([&client, &partition, eventHubName, &iterationsPerThread]() { + size_t iterations = 0; + std::chrono::system_clock::duration timeout = std::chrono::seconds(3); + std::chrono::system_clock::time_point start = std::chrono::system_clock::now(); + while ((std::chrono::system_clock::now() - start) <= timeout) + { + Azure::Messaging::EventHubs::Models::EventHubPartitionProperties result; + ASSERT_NO_THROW(result = client.GetPartitionProperties(partition)); + EXPECT_EQ(result.Name, eventHubName); + EXPECT_EQ(result.PartitionId, partition); + std::this_thread::yield(); + iterations++; + } + iterationsPerThread.push_back(iterations); + }); for (auto& t : partitionThreads) { if (t.joinable()) From 99cc8848f52f6014e7a9685b1f02126fa84af666 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 6 Feb 2024 10:01:22 -0800 Subject: [PATCH 10/24] Don't emit body contents in AmqpMessage insertion operator --- .../inc/azure/core/amqp/models/amqp_value.hpp | 10 +++ .../src/models/amqp_message.cpp | 38 ++++++--- .../azure-core-amqp/src/models/amqp_value.cpp | 84 +++++++++++++++++++ .../test/ut/amqp_message_tests.cpp | 5 +- 4 files changed, 123 insertions(+), 14 deletions(-) diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp index 0b0a441e23..99a49f2690 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp @@ -76,6 +76,16 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { Unknown, }; + /** + * @brief ostream insertion operator for AmqpValueType. + * + * @param os - stream to insert to. + * @param value - value to insert. + * + * @returns the input ostream. + */ + std::ostream& operator<<(std::ostream& os, AmqpValueType const value); + class AmqpArray; class AmqpMap; class AmqpList; diff --git a/sdk/core/azure-core-amqp/src/models/amqp_message.cpp b/sdk/core/azure-core-amqp/src/models/amqp_message.cpp index aab27cd654..f07ab7c344 100644 --- a/sdk/core/azure-core-amqp/src/models/amqp_message.cpp +++ b/sdk/core/azure-core-amqp/src/models/amqp_message.cpp @@ -749,13 +749,14 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { std::ostream& operator<<(std::ostream& os, AmqpMessage const& message) { - os << "Message: " << std::endl; + os << "Message: <" << std::endl; + if (message.MessageFormat != AmqpDefaultMessageFormatValue) { os << " Message Format: " << message.MessageFormat << std::endl; } - os << " Header " << message.Header << std::endl; - os << " Properties: " << message.Properties << std::endl; + os << " " << message.Header << std::endl; + os << " " << message.Properties; { if (!message.ApplicationProperties.empty()) @@ -796,38 +797,49 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { } } - os << std::endl << " Body: [" << std::endl; + os << std::endl << " Body: ["; switch (message.BodyType) { case MessageBodyType::Invalid: - os << " Invalid"; + os << "Invalid"; break; case MessageBodyType::None: - os << " None"; + os << "None"; break; case MessageBodyType::Data: { - os << " AMQP Data: ["; + os << "AmqpBinaryData: ["; auto const& bodyBinary = message.GetBodyAsBinary(); uint8_t i = 0; for (auto const& val : bodyBinary) { - os << "Data: " << val << std::endl; + os << val.size() << " bytes"; if (i < bodyBinary.size() - 1) { os << ", "; } i += 1; } - os << " ]"; + os << "]"; } break; case MessageBodyType::Sequence: { - os << " AMQP Sequence: ["; + os << "AmqpSequence: ["; auto const& bodySequence = message.GetBodyAsAmqpList(); uint8_t i = 0; for (auto const& val : bodySequence) { - os << "Sequence: " << val << std::endl; + os << "{Sequence: "; + uint8_t j = 0; + for (auto const& seqVal : val) + { + os << seqVal.GetType(); + if (j < val.size() - 1) + { + os << ", "; + } + j += 1; + } + os << "}"; if (i < bodySequence.size() - 1) { os << ", "; @@ -838,10 +850,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { } break; case MessageBodyType::Value: - os << " AmqpValue: " << message.GetBodyAsAmqpValue(); + os << "AmqpValue, type=" << message.GetBodyAsAmqpValue().GetType(); break; } - os << std::endl << " ]"; + os << std::endl << ">"; return os; } diff --git a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp index 15962ec650..0923220877 100644 --- a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp +++ b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp @@ -146,6 +146,90 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { } } // namespace _detail + std::ostream& operator<<(std::ostream& os, AmqpValueType const value) + { + + switch (value) + { + case AmqpValueType::Invalid: + os << "Invalid"; + break; + case AmqpValueType::Null: + os << "Null"; + break; + case AmqpValueType::Bool: + os << "Bool"; + break; + case AmqpValueType::Ubyte: + os << "Ubyte"; + break; + case AmqpValueType::Ushort: + os << "Ushort"; + break; + case AmqpValueType::Uint: + os << "Uint"; + break; + case AmqpValueType::Ulong: + os << "Ulong"; + break; + case AmqpValueType::Byte: + os << "Byte"; + break; + case AmqpValueType::Short: + os << "Short"; + break; + case AmqpValueType::Int: + os << "Int"; + break; + case AmqpValueType::Long: + os << "Long"; + break; + case AmqpValueType::Float: + os << "Float"; + break; + case AmqpValueType::Double: + os << "Double"; + break; + case AmqpValueType::Char: + os << "Char"; + break; + case AmqpValueType::Timestamp: + os << "Timestamp"; + break; + case AmqpValueType::Uuid: + os << "Uuid"; + break; + case AmqpValueType::Binary: + os << "Binary"; + break; + case AmqpValueType::String: + os << "String"; + break; + case AmqpValueType::Symbol: + os << "Symbol"; + break; + case AmqpValueType::List: + os << "List"; + break; + case AmqpValueType::Map: + os << "Map"; + break; + case AmqpValueType::Array: + os << "Array"; + break; + case AmqpValueType::Described: + os << "Described"; + break; + case AmqpValueType::Composite: + os << "Composite"; + break; + case AmqpValueType::Unknown: + os << "Unknown"; + break; + } + + return os; + } AmqpValue::~AmqpValue() {} AmqpValue::AmqpValue(bool bool_value) diff --git a/sdk/core/azure-core-amqp/test/ut/amqp_message_tests.cpp b/sdk/core/azure-core-amqp/test/ut/amqp_message_tests.cpp index 1e7c0809f6..397c58061e 100644 --- a/sdk/core/azure-core-amqp/test/ut/amqp_message_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/amqp_message_tests.cpp @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -//#include "../src/models/private/message_impl.hpp" +// #include "../src/models/private/message_impl.hpp" #include "azure/core/amqp/models/amqp_message.hpp" #include @@ -183,6 +183,9 @@ TEST_F(TestMessage, TestBodyAmqpData) EXPECT_EQ(message2->BodyType, MessageBodyType::Data); GTEST_LOG_(INFO) << message; + + message.SetBody({AmqpBinaryData{1, 3, 5, 7, 9, 10}, AmqpBinaryData{2, 4, 6, 8}}); + GTEST_LOG_(INFO) << message; } class MessageSerialization : public testing::Test { From 908992615b85252ed3596e0b03234d2250ba945a Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 6 Feb 2024 10:19:08 -0800 Subject: [PATCH 11/24] Don't take numeric value parameters by value to ostream insertion operators --- .../inc/azure/core/amqp/internal/amqp_settle_mode.hpp | 4 ++-- .../inc/azure/core/amqp/internal/claims_based_security.hpp | 4 ++-- .../azure-core-amqp/inc/azure/core/amqp/internal/link.hpp | 2 +- .../inc/azure/core/amqp/internal/message_receiver.hpp | 2 +- .../inc/azure/core/amqp/internal/message_sender.hpp | 2 +- sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp | 4 ++-- sdk/core/azure-core-amqp/src/amqp/link.cpp | 2 +- sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp | 6 +++--- sdk/core/azure-core-amqp/src/amqp/message_sender.cpp | 4 ++-- 9 files changed, 15 insertions(+), 15 deletions(-) diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp index ef56b9067e..7396c31ad0 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp @@ -14,13 +14,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Mixed, }; - std::ostream& operator<<(std::ostream& os, SenderSettleMode const& mode); + std::ostream& operator<<(std::ostream& os, SenderSettleMode const mode); enum class ReceiverSettleMode { First, Second, }; - std::ostream& operator<<(std::ostream& os, ReceiverSettleMode const& mode); + std::ostream& operator<<(std::ostream& os, ReceiverSettleMode const mode); }}}} // namespace Azure::Core::Amqp::_internal diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp index 82c2a395f0..598e730676 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp @@ -18,7 +18,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Failed, InstanceClosed, }; - std::ostream& operator<<(std::ostream& os, CbsOperationResult const& operationResult); + std::ostream& operator<<(std::ostream& os, CbsOperationResult const operationResult); enum class CbsOpenResult { @@ -27,7 +27,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Error, Cancelled, }; - std::ostream& operator<<(std::ostream& os, CbsOpenResult const& operationResult); + std::ostream& operator<<(std::ostream& os, CbsOpenResult const operationResult); enum class CbsTokenType { diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp index 7f4141e8d7..95a6987577 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp @@ -47,7 +47,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Error, }; - std::ostream& operator<<(std::ostream& stream, LinkState const& linkState); + std::ostream& operator<<(std::ostream& stream, LinkState const linkState); enum class LinkTransferResult { diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp index f47c2b3e91..645197f976 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp @@ -33,7 +33,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Closing, Error, }; - std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const& state); + std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const state); class MessageReceiver; diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp index 82989c89a2..e3bdc827a2 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp @@ -40,7 +40,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Closing, Error, }; - std::ostream& operator<<(std::ostream& stream, MessageSenderState const& state); + std::ostream& operator<<(std::ostream& stream, MessageSenderState const state); class MessageSender; class MessageSenderEvents { diff --git a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp index 62ac646ccb..4e2c1137f8 100644 --- a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp @@ -137,7 +137,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return std::make_tuple(cbsResult, result.StatusCode, result.Error.Description); } } - std::ostream& operator<<(std::ostream& os, CbsOperationResult const& operationResult) + std::ostream& operator<<(std::ostream& os, CbsOperationResult const operationResult) { switch (operationResult) { @@ -163,7 +163,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return os; } - std::ostream& operator<<(std::ostream& os, CbsOpenResult const& openResult) + std::ostream& operator<<(std::ostream& os, CbsOpenResult const openResult) { switch (openResult) { diff --git a/sdk/core/azure-core-amqp/src/amqp/link.cpp b/sdk/core/azure-core-amqp/src/amqp/link.cpp index 8dd9ae7a84..7c714f9108 100644 --- a/sdk/core/azure-core-amqp/src/amqp/link.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/link.cpp @@ -126,7 +126,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } #endif - std::ostream& operator<<(std::ostream& os, LinkState const& linkState) + std::ostream& operator<<(std::ostream& os, LinkState const linkState) { switch (linkState) { diff --git a/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp b/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp index e24e97dbfb..bff0dee102 100644 --- a/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp @@ -41,7 +41,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { namespace Azure { namespace Core { namespace Amqp { namespace _internal { - std::ostream& operator<<(std::ostream& stream, ReceiverSettleMode const& settleMode) + std::ostream& operator<<(std::ostream& stream, ReceiverSettleMode const settleMode) { switch (settleMode) { @@ -109,7 +109,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { } std::string MessageReceiver::GetLinkName() const { return m_impl->GetLinkName(); } - std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const& state) + std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const state) { switch (state) { @@ -399,7 +399,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { "MESSAGE_RECEIVER_STATE_ERROR", }; - std::ostream& operator<<(std::ostream& stream, MESSAGE_RECEIVER_STATE const& state) + std::ostream& operator<<(std::ostream& stream, MESSAGE_RECEIVER_STATE const state) { if (state < sizeof(MESSAGE_RECEIVER_STATEStrings) / sizeof(MESSAGE_RECEIVER_STATEStrings[0])) { diff --git a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp index de4d8226d5..4739cf0480 100644 --- a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp @@ -37,7 +37,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { }}}} // namespace Azure::Core::Amqp::_detail namespace Azure { namespace Core { namespace Amqp { namespace _internal { - std::ostream& operator<<(std::ostream& stream, SenderSettleMode const& settleMode) + std::ostream& operator<<(std::ostream& stream, SenderSettleMode const settleMode) { switch (settleMode) { @@ -66,7 +66,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { std::uint64_t MessageSender::GetMaxMessageSize() const { return m_impl->GetMaxMessageSize(); } std::string MessageSender::GetLinkName() const { return m_impl->GetLinkName(); } MessageSender::~MessageSender() noexcept {} - std::ostream& operator<<(std::ostream& stream, _internal::MessageSenderState const& state) + std::ostream& operator<<(std::ostream& stream, _internal::MessageSenderState const state) { switch (state) { From 847296aae7e0920ff5045e4f9ff5029be1535109 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 6 Feb 2024 10:19:24 -0800 Subject: [PATCH 12/24] clang-format --- .../azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp index 99a49f2690..055277290d 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp @@ -78,10 +78,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { /** * @brief ostream insertion operator for AmqpValueType. - * + * * @param os - stream to insert to. * @param value - value to insert. - * + * * @returns the input ostream. */ std::ostream& operator<<(std::ostream& os, AmqpValueType const value); From b31ccd990da949440ca488dd7f9bd368c1328b10 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 6 Feb 2024 13:26:16 -0800 Subject: [PATCH 13/24] Updated changelog to reflect changes in this PR --- sdk/core/azure-core-amqp/CHANGELOG.md | 11 +++++++++++ sdk/core/azure-core-amqp/src/amqp/management.cpp | 2 +- sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md | 3 +++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index 17642cec9d..ffcfd1194b 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -6,10 +6,21 @@ ### Breaking Changes +- `ClaimBasedSecurity::PutToken` throws `Azure::Core::Credentials::AuthenticationException` instead of `std::runtime_error`. + ### Bugs Fixed +- Fixed [#5284](https://github.com/Azure/azure-sdk-for-cpp/issues/5284). +- Fixed [#5297](https://github.com/Azure/azure-sdk-for-cpp/issues/5297). Enabled multiple simultaneous `ExecuteOperation` calls. +- Fixed crash when Link Detach message is received while link is being destroyed. + ### Other Changes +- `std::ostream` inserter for message body no longer prints the body of the message. +- Tidied up the output of the `AmqpMessage` `std::ostream` inserter. +- Added several `std::ostream` inserters. +- Pass numeric values to `std::ostream` inserters by value not by reference. + ## 1.0.0-beta.7 (2024-02-02) ### Features Added diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 7c45be7530..26e7961157 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -533,7 +533,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // Ensure nobody else is messing with the message queues right now. std::unique_lock lock(m_messageQueuesLock); - // Remove the queue from the map, we don't need it anymore. + // If the correlation ID is found locally, complete the operation with an error. if (m_messageQueues.find(correlationId) != m_messageQueues.end()) { // Complete any outstanding receives with an error. diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 8058f95543..171e5004b2 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -8,8 +8,11 @@ ### Bugs Fixed +- Fixed [#5297](https://github.com/Azure/azure-sdk-for-cpp/issues/5297). The actual fix for this was to use a single management client per connection. + ### Other Changes + ## 1.0.0-beta.6 (2024-02-06) ### Breaking Changes From 64cdeb16a28c97fd0329a4f8234b5d2d4bcf7bce Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 6 Feb 2024 16:47:59 -0800 Subject: [PATCH 14/24] Updated eventhubs dependency to match reality --- sdk/eventhubs/azure-messaging-eventhubs/vcpkg/vcpkg.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/vcpkg/vcpkg.json b/sdk/eventhubs/azure-messaging-eventhubs/vcpkg/vcpkg.json index 44289a29cb..ca9cb484fc 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/vcpkg/vcpkg.json +++ b/sdk/eventhubs/azure-messaging-eventhubs/vcpkg/vcpkg.json @@ -18,7 +18,7 @@ { "name": "azure-core-amqp-cpp", "default-features": false, - "version>=": "1.0.0-beta.5" + "version>=": "1.0.0-beta.7" }, { "name": "vcpkg-cmake", From 1307c98425a3888ff5806c5f399da81d6c0a6da4 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 08:40:04 -0800 Subject: [PATCH 15/24] Pull request feedback --- sdk/core/azure-core-amqp/CHANGELOG.md | 6 +-- .../core/amqp/internal/amqp_settle_mode.hpp | 4 +- .../amqp/internal/claims_based_security.hpp | 4 +- .../azure/core/amqp/internal/connection.hpp | 2 +- .../inc/azure/core/amqp/internal/link.hpp | 2 +- .../core/amqp/internal/message_receiver.hpp | 2 +- .../core/amqp/internal/message_sender.hpp | 2 +- .../inc/azure/core/amqp/models/amqp_value.hpp | 2 +- .../src/amqp/claim_based_security.cpp | 4 +- sdk/core/azure-core-amqp/src/amqp/link.cpp | 2 +- .../azure-core-amqp/src/amqp/management.cpp | 12 ++++++ .../src/amqp/message_receiver.cpp | 6 +-- .../src/amqp/message_sender.cpp | 4 +- .../src/amqp/private/management_impl.hpp | 1 + .../azure-core-amqp/src/models/amqp_value.cpp | 4 +- .../src/models/private/value_impl.hpp | 2 +- .../azure-messaging-eventhubs/CHANGELOG.md | 1 - .../test/ut/processor_test.cpp | 38 ------------------- 18 files changed, 36 insertions(+), 62 deletions(-) diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index ffcfd1194b..662e16c3ac 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -6,12 +6,12 @@ ### Breaking Changes -- `ClaimBasedSecurity::PutToken` throws `Azure::Core::Credentials::AuthenticationException` instead of `std::runtime_error`. +- Claims Based Security authentication now throws `Azure::Core::Credentials::AuthenticationException` instead of `std::runtime_error`. ### Bugs Fixed -- Fixed [#5284](https://github.com/Azure/azure-sdk-for-cpp/issues/5284). -- Fixed [#5297](https://github.com/Azure/azure-sdk-for-cpp/issues/5297). Enabled multiple simultaneous `ExecuteOperation` calls. +- [#5284](https://github.com/Azure/azure-sdk-for-cpp/issues/5284): [azure-identity][azure-messaging-eventhubs] Impossible to catch exception resulting in SIGABRT signal. +- [#5297](https://github.com/Azure/azure-sdk-for-cpp/issues/5297): Enabled multiple simultaneous `ExecuteOperation` calls. - Fixed crash when Link Detach message is received while link is being destroyed. ### Other Changes diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp index 7396c31ad0..99ece9d1c7 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/amqp_settle_mode.hpp @@ -14,13 +14,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Mixed, }; - std::ostream& operator<<(std::ostream& os, SenderSettleMode const mode); + std::ostream& operator<<(std::ostream& os, SenderSettleMode mode); enum class ReceiverSettleMode { First, Second, }; - std::ostream& operator<<(std::ostream& os, ReceiverSettleMode const mode); + std::ostream& operator<<(std::ostream& os, ReceiverSettleMode mode); }}}} // namespace Azure::Core::Amqp::_internal diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp index 598e730676..c97626ff77 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp @@ -18,7 +18,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Failed, InstanceClosed, }; - std::ostream& operator<<(std::ostream& os, CbsOperationResult const operationResult); + std::ostream& operator<<(std::ostream& os, CbsOperationResult operationResult); enum class CbsOpenResult { @@ -27,7 +27,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Error, Cancelled, }; - std::ostream& operator<<(std::ostream& os, CbsOpenResult const operationResult); + std::ostream& operator<<(std::ostream& os, CbsOpenResult operationResult); enum class CbsTokenType { diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection.hpp index 1e3cf99f7e..9df7df1a07 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection.hpp @@ -158,7 +158,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Error, }; - std::ostream& operator<<(std::ostream& stream, ConnectionState const value); + std::ostream& operator<<(std::ostream& stream, ConnectionState value); class Connection; diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp index 95a6987577..6d93bb18e8 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/link.hpp @@ -47,7 +47,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Error, }; - std::ostream& operator<<(std::ostream& stream, LinkState const linkState); + std::ostream& operator<<(std::ostream& stream, LinkState linkState); enum class LinkTransferResult { diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp index 645197f976..8e17ed2258 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp @@ -33,7 +33,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Closing, Error, }; - std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const state); + std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState state); class MessageReceiver; diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp index e3bdc827a2..957f6cfaab 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp @@ -40,7 +40,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Closing, Error, }; - std::ostream& operator<<(std::ostream& stream, MessageSenderState const state); + std::ostream& operator<<(std::ostream& stream, MessageSenderState state); class MessageSender; class MessageSenderEvents { diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp index 055277290d..3a3f1f6684 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/models/amqp_value.hpp @@ -84,7 +84,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { * * @returns the input ostream. */ - std::ostream& operator<<(std::ostream& os, AmqpValueType const value); + std::ostream& operator<<(std::ostream& os, AmqpValueType value); class AmqpArray; class AmqpMap; diff --git a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp index 4e2c1137f8..1291a668f1 100644 --- a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp @@ -137,7 +137,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return std::make_tuple(cbsResult, result.StatusCode, result.Error.Description); } } - std::ostream& operator<<(std::ostream& os, CbsOperationResult const operationResult) + std::ostream& operator<<(std::ostream& os, CbsOperationResult operationResult) { switch (operationResult) { @@ -163,7 +163,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return os; } - std::ostream& operator<<(std::ostream& os, CbsOpenResult const openResult) + std::ostream& operator<<(std::ostream& os, CbsOpenResult openResult) { switch (openResult) { diff --git a/sdk/core/azure-core-amqp/src/amqp/link.cpp b/sdk/core/azure-core-amqp/src/amqp/link.cpp index 7c714f9108..f5eedf2369 100644 --- a/sdk/core/azure-core-amqp/src/amqp/link.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/link.cpp @@ -126,7 +126,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } #endif - std::ostream& operator<<(std::ostream& os, LinkState const linkState) + std::ostream& operator<<(std::ostream& os, LinkState linkState) { switch (linkState) { diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 26e7961157..cc47330a33 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -75,6 +75,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::ManagementOpenStatus ManagementClientImpl::Open(Context const& context) { + std::unique_lock lock(m_openCloseLock); + if (m_isOpen) + { + throw std::runtime_error("Management object is already open."); + } + try { /** Authentication needs to happen *before* the links are created. @@ -261,7 +267,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { void ManagementClientImpl::Close(Context const& context) { + std::unique_lock lock(m_openCloseLock); Log::Stream(Logger::Level::Verbose) << "ManagementClient::Close" << std::endl; + if (!m_isOpen) + { + throw std::runtime_error("Management object is not open."); + } + SetState(ManagementState::Closing); if (m_messageSender && m_messageSenderOpen) { diff --git a/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp b/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp index bff0dee102..fbc32715b1 100644 --- a/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/message_receiver.cpp @@ -41,7 +41,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { namespace Azure { namespace Core { namespace Amqp { namespace _internal { - std::ostream& operator<<(std::ostream& stream, ReceiverSettleMode const settleMode) + std::ostream& operator<<(std::ostream& stream, ReceiverSettleMode settleMode) { switch (settleMode) { @@ -109,7 +109,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { } std::string MessageReceiver::GetLinkName() const { return m_impl->GetLinkName(); } - std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState const state) + std::ostream& operator<<(std::ostream& stream, _internal::MessageReceiverState state) { switch (state) { @@ -399,7 +399,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { "MESSAGE_RECEIVER_STATE_ERROR", }; - std::ostream& operator<<(std::ostream& stream, MESSAGE_RECEIVER_STATE const state) + std::ostream& operator<<(std::ostream& stream, MESSAGE_RECEIVER_STATE state) { if (state < sizeof(MESSAGE_RECEIVER_STATEStrings) / sizeof(MESSAGE_RECEIVER_STATEStrings[0])) { diff --git a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp index 4739cf0480..2fde9fbb39 100644 --- a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp @@ -37,7 +37,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { }}}} // namespace Azure::Core::Amqp::_detail namespace Azure { namespace Core { namespace Amqp { namespace _internal { - std::ostream& operator<<(std::ostream& stream, SenderSettleMode const settleMode) + std::ostream& operator<<(std::ostream& stream, SenderSettleMode settleMode) { switch (settleMode) { @@ -66,7 +66,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { std::uint64_t MessageSender::GetMaxMessageSize() const { return m_impl->GetMaxMessageSize(); } std::string MessageSender::GetLinkName() const { return m_impl->GetLinkName(); } MessageSender::~MessageSender() noexcept {} - std::ostream& operator<<(std::ostream& stream, _internal::MessageSenderState const state) + std::ostream& operator<<(std::ostream& stream, _internal::MessageSenderState state) { switch (state) { diff --git a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp index ce3f05cce0..08479b9f3e 100644 --- a/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/amqp/private/management_impl.hpp @@ -82,6 +82,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { std::shared_ptr m_messageSender; std::shared_ptr m_messageReceiver; ManagementState m_state = ManagementState::Idle; + std::mutex m_openCloseLock; bool m_isOpen{false}; bool m_messageSenderOpen{false}; bool m_messageReceiverOpen{false}; diff --git a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp index 0923220877..ab6cd65189 100644 --- a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp +++ b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp @@ -46,7 +46,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { namespace Azure { namespace Core { namespace Amqp { namespace Models { namespace _detail { - std::ostream& operator<<(std::ostream& os, AMQP_TYPE const value) + std::ostream& operator<<(std::ostream& os, AMQP_TYPE value) { switch (value) { @@ -146,7 +146,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { } } // namespace _detail - std::ostream& operator<<(std::ostream& os, AmqpValueType const value) + std::ostream& operator<<(std::ostream& os, AmqpValueType value) { switch (value) diff --git a/sdk/core/azure-core-amqp/src/models/private/value_impl.hpp b/sdk/core/azure-core-amqp/src/models/private/value_impl.hpp index b081326086..4c01283f51 100644 --- a/sdk/core/azure-core-amqp/src/models/private/value_impl.hpp +++ b/sdk/core/azure-core-amqp/src/models/private/value_impl.hpp @@ -58,7 +58,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { namespace private: UniqueAmqpValueHandle m_value; }; - std::ostream& operator<<(std::ostream& os, AMQP_TYPE const value); + std::ostream& operator<<(std::ostream& os, AMQP_TYPE value); std::ostream& operator<<(std::ostream& os, AMQP_VALUE const value); }}}}} // namespace Azure::Core::Amqp::Models::_detail diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 171e5004b2..bd10a80f34 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -12,7 +12,6 @@ ### Other Changes - ## 1.0.0-beta.6 (2024-02-06) ### Breaking Changes diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp index a01147fee3..103ef36e3b 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/processor_test.cpp @@ -121,7 +121,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { producerOptions.Name = "Producer for LoadBalancerTest"; ProducerClient producerClient{connectionString, eventHubName, producerOptions}; -#if TRUE std::thread processEventsThread([&]() { std::set partitionsAcquired; std::vector processEventsThreads; @@ -160,43 +159,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { processor.Run(context); processEventsThread.join(); -#else - // Run the processor on a background thread and the test on the foreground. - processor.Start(context); - - std::set partitionsAcquired; - std::vector processEventsThreads; - // When we exit the process thread, cancel the context to unblock the processor. - // scope_guard onExit([&context] { context.Cancel(); }); - - WaitGroup waitGroup; - for (auto const& partitionId : eventHubProperties.PartitionIds) - { - std::shared_ptr partitionClient - = processor.NextPartitionClient(context); - waitGroup.AddWaiter(); - ASSERT_EQ(partitionsAcquired.find(partitionId), partitionsAcquired.end()) - << "No previous client for " << partitionClient->PartitionId(); - processEventsThreads.push_back( - std::thread([&waitGroup, &producerClient, partitionClient, &context, this] { - scope_guard onExit([&] { waitGroup.CompleteWaiter(); }); - ProcessEventsForLoadBalancerTest(producerClient, partitionClient, context); - })); - } - // Block until all the events have been processed. - waitGroup.Wait(); - - // And wait until all the threads have completed. - for (auto& thread : processEventsThreads) - { - if (thread.joinable()) - { - thread.join(); - } - } - // Stop the processor, we're done with the test. - processor.Stop(); -#endif } void TestWithLoadBalancerSingleThreaded(Models::ProcessorStrategy processorStrategy) From 5964ef91641d25e4eadd3c5d1150b6fb0ca00190 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 09:19:31 -0800 Subject: [PATCH 16/24] Fixed test crashes in management tests --- sdk/core/azure-core-amqp/src/amqp/management.cpp | 8 +++++++- sdk/core/azure-core-amqp/test/ut/management_tests.cpp | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index cc47330a33..76613974e3 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -151,7 +151,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } return rv; } - // If result is null, then it means that the context was cancelled. + + // If result is null, then it means that the context was cancelled. Close the things we opened earlier (if any) + // and return the error. + m_messageSender->Close({}); + m_messageSenderOpen = false; + m_messageReceiver->Close({}); + m_messageReceiverOpen = false; return _internal::ManagementOpenStatus::Cancelled; } catch (...) diff --git a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp index 65c5d6c5bf..cce46a65c0 100644 --- a/sdk/core/azure-core-amqp/test/ut/management_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/management_tests.cpp @@ -258,7 +258,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { context.Cancel(); EXPECT_EQ(management.Open(context), ManagementOpenStatus::Cancelled); - management.Close(); + EXPECT_THROW(management.Close(), std::runtime_error); mockServer.StopListening(); } From 51e432b2eb3fc2cb482cc04ec135f00b7e27ccd0 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 09:43:04 -0800 Subject: [PATCH 17/24] clang-format --- sdk/core/azure-core-amqp/src/amqp/management.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 76613974e3..4bc144dc84 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -152,8 +152,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return rv; } - // If result is null, then it means that the context was cancelled. Close the things we opened earlier (if any) - // and return the error. + // If result is null, then it means that the context was cancelled. Close the things we opened + // earlier (if any) and return the error. m_messageSender->Close({}); m_messageSenderOpen = false; m_messageReceiver->Close({}); From db550b91cc249e5f22cb910ba862cf16e5198551 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 12:09:40 -0800 Subject: [PATCH 18/24] Improved code coverage --- sdk/core/azure-core-amqp/CHANGELOG.md | 8 ++- .../amqp/internal/claims_based_security.hpp | 1 + .../azure/core/amqp/internal/management.hpp | 1 + .../core/amqp/internal/message_sender.hpp | 2 + .../src/amqp/claim_based_security.cpp | 25 +++---- .../azure-core-amqp/src/amqp/connection.cpp | 4 +- .../azure-core-amqp/src/amqp/management.cpp | 22 +++++- .../src/amqp/message_sender.cpp | 63 ++++++++++++---- .../test/ut/claim_based_security_tests.cpp | 72 ++++++++++++++++++- 9 files changed, 162 insertions(+), 36 deletions(-) diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index 662e16c3ac..5511a90a47 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -6,12 +6,14 @@ ### Breaking Changes -- Claims Based Security authentication now throws `Azure::Core::Credentials::AuthenticationException` instead of `std::runtime_error`. +- Claims Based Security authentication now longer throws a `std::runtime_error`, and instead follows the pattern of the rest of the AMQP library and returns an error. +- Authentication now throws `Azure::Core::Credentials::AuthenticationException` instead of `std::runtime_error`. +- Added `Cancelled` status to `CbsOperationResult` and `ManagementOperationStatus`. ### Bugs Fixed -- [#5284](https://github.com/Azure/azure-sdk-for-cpp/issues/5284): [azure-identity][azure-messaging-eventhubs] Impossible to catch exception resulting in SIGABRT signal. -- [#5297](https://github.com/Azure/azure-sdk-for-cpp/issues/5297): Enabled multiple simultaneous `ExecuteOperation` calls. +- [[#5284]](https://github.com/Azure/azure-sdk-for-cpp/issues/5284) [azure-identity][azure-messaging-eventhubs] Impossible to catch exception resulting in SIGABRT signal. +- [[#5297]](https://github.com/Azure/azure-sdk-for-cpp/issues/5297): Enabled multiple simultaneous `ExecuteOperation` calls. - Fixed crash when Link Detach message is received while link is being destroyed. ### Other Changes diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp index c97626ff77..dcb82ee6bf 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp @@ -17,6 +17,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Error, Failed, InstanceClosed, + Cancelled, }; std::ostream& operator<<(std::ostream& os, CbsOperationResult operationResult); diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/management.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/management.hpp index 8b2a1f5160..29ece47e2b 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/management.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/management.hpp @@ -26,6 +26,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Error, FailedBadStatus, InstanceClosed, + Cancelled, }; enum class ManagementOpenStatus diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp index 957f6cfaab..c1ac56feee 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_sender.hpp @@ -31,6 +31,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { Timeout, Cancelled, }; + std::ostream& operator<<(std::ostream& stream, MessageSendStatus status); + enum class MessageSenderState { Invalid, diff --git a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp index 1291a668f1..425d89e168 100644 --- a/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/claim_based_security.cpp @@ -102,13 +102,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { message, context); if (result.Status != ManagementOperationStatus::Ok) - { - throw Azure::Core::Credentials::AuthenticationException( - "Could not authenticate to client. Error Status: " + std::to_string(result.StatusCode) - + " condition: " + result.Error.Condition.ToString() - + " reason: " + result.Error.Description); - } - else { CbsOperationResult cbsResult; switch (result.Status) @@ -128,14 +121,21 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { case ManagementOperationStatus::InstanceClosed: cbsResult = CbsOperationResult::InstanceClosed; break; + case ManagementOperationStatus::Cancelled: + cbsResult = CbsOperationResult::Cancelled; + break; default: throw std::runtime_error("Unknown management operation status."); } Log::Stream(Logger::Level::Informational) << "CBS PutToken result: " << cbsResult << " status code: " << result.StatusCode - << " Error: " << result.Error.Description << "."; + << " Error: " << result.Error << "."; return std::make_tuple(cbsResult, result.StatusCode, result.Error.Description); } + else + { + return std::make_tuple(CbsOperationResult::Ok, result.StatusCode, result.Error.Description); + } } std::ostream& operator<<(std::ostream& os, CbsOperationResult operationResult) { @@ -156,9 +156,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { case CbsOperationResult::InstanceClosed: os << "InstanceClosed"; break; - default: - os << "Unknown CbsOperationResult." - << static_cast::type>(operationResult); + case CbsOperationResult::Cancelled: + os << "Cancelled"; + break; } return os; } @@ -179,9 +179,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { case CbsOpenResult::Cancelled: os << "Cancelled"; break; - default: - os << "Unknown CbsOpenResult." - << static_cast::type>(openResult); } return os; } diff --git a/sdk/core/azure-core-amqp/src/amqp/connection.cpp b/sdk/core/azure-core-amqp/src/amqp/connection.cpp index 07c1620e55..409ac59bcf 100644 --- a/sdk/core/azure-core-amqp/src/amqp/connection.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/connection.cpp @@ -677,7 +677,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { context); if (std::get<0>(result) != CbsOperationResult::Ok) { - throw std::runtime_error("Could not put Claims Based Security token."); + throw Azure::Core::Credentials::AuthenticationException( + "Could not authenticate client. Error Status: " + + std::to_string(std::get<1>(result)) + " reason: " + std::get<2>(result)); } Log::Stream(Logger::Level::Verbose) << "Close CBS object"; claimsBasedSecurity->Close(context); diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 4bc144dc84..469a5bda68 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -217,10 +217,28 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { auto sendResult = m_messageSender->Send(messageToSend, context); if (std::get<0>(sendResult) != _internal::MessageSendStatus::Ok) { + auto sendStatus = std::get<0>(sendResult); + const auto& sendError = std::get<1>(sendResult); + Log::Stream(Logger::Level::Error) + << "ManagementClient::ExecuteOperation, send failed" << sendStatus; _internal::ManagementOperationResult rv; - rv.Status = _internal::ManagementOperationStatus::Error; + switch (sendStatus) + { + case _internal::MessageSendStatus::Error: + rv.Status = _internal::ManagementOperationStatus::Error; + break; + case _internal::MessageSendStatus::Cancelled: + rv.Status = _internal::ManagementOperationStatus::Cancelled; + break; + case _internal::MessageSendStatus::Invalid: + rv.Status = _internal::ManagementOperationStatus::Invalid; + break; + case _internal::MessageSendStatus::Timeout: + rv.Status = _internal::ManagementOperationStatus::Error; + break; + } rv.StatusCode = 500; - rv.Error = std::get<1>(sendResult); + rv.Error = sendError; rv.Message = nullptr; { std::unique_lock lock(m_messageQueuesLock); diff --git a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp index 2fde9fbb39..f5a53a27d5 100644 --- a/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/message_sender.cpp @@ -91,6 +91,30 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { } return stream; } + + std::ostream& operator<<(std::ostream& stream, _internal::MessageSendStatus status) + { + switch (status) + { + case _internal::MessageSendStatus::Invalid: + stream << "Invalid"; + break; + case _internal::MessageSendStatus::Cancelled: + stream << "Cancelled"; + break; + case _internal::MessageSendStatus::Error: + stream << "Error"; + break; + case _internal::MessageSendStatus::Ok: + stream << "Ok"; + break; + case _internal::MessageSendStatus::Timeout: + stream << "Timeout"; + break; + } + return stream; + } + }}}} // namespace Azure::Core::Amqp::_internal namespace Azure { namespace Core { namespace Amqp { namespace _detail { @@ -461,20 +485,26 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Azure::Core::Amqp::_internal::MessageSender::MessageSendCompleteCallback onSendComplete, Context const& context) { - auto operation(std::make_unique>>(onSendComplete)); - auto result = messagesender_send_async( - m_messageSender.get(), - Models::_detail::AmqpMessageFactory::ToUamqp(message).get(), - std::remove_pointer::type::OnOperationFn, - operation.release(), - 0 /*timeout*/); - if (result == nullptr) + // If the context is canceled, don't queue the operation. + // Note that normally this would be handled via uAMQP's async operation cancellation, but if the + // remote node sends an incoming frame, the async operation completion handler will be called + // twice, which results in a double free of the underlying operation. + if (!context.IsCancelled()) { - throw std::runtime_error("Could not send message"); + auto operation(std::make_unique>>(onSendComplete)); + auto result = messagesender_send_async( + m_messageSender.get(), + Models::_detail::AmqpMessageFactory::ToUamqp(message).get(), + std::remove_pointer::type::OnOperationFn, + operation.release(), + 0 /*timeout*/); + if (result == nullptr) + { + throw std::runtime_error("Could not send message"); + } } - (void)context; } std::tuple<_internal::MessageSendStatus, Models::_internal::AmqpError> MessageSenderImpl::Send( @@ -538,7 +568,14 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { { return std::move(*result); } - throw std::runtime_error("Error sending message"); + else + { + Models::_internal::AmqpError error{ + Models::_internal::AmqpErrorCondition::OperationCancelled, + "Message send operation cancelled.", + {}}; + return std::make_tuple(_internal::MessageSendStatus::Cancelled, error); + } } std::string MessageSenderImpl::GetLinkName() const { return m_link->GetName(); } diff --git a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp index c31de73fe7..e7de12a0f4 100644 --- a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp @@ -75,6 +75,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { EXPECT_EQ(CbsOpenResult::Error, cbs.Open()); } } + TEST_F(TestCbs, CbsOpen) { MessageTests::AmqpServerMock mockServer; @@ -103,6 +104,33 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { } mockServer.StopListening(); } + + TEST_F(TestCbs, CbsCancelledOpen) + { + MessageTests::AmqpServerMock mockServer; + + mockServer.EnableTrace(false); + + ConnectionOptions options; + options.Port = mockServer.GetPort(); + options.EnableTrace = true; + options.ContainerId = testing::UnitTest::GetInstance()->current_test_info()->test_case_name(); + Connection connection("localhost", nullptr, options); + Session session{connection.CreateSession()}; + + mockServer.StartListening(); + + { + GTEST_LOG_(INFO) << "Create CBS object."; + ClaimsBasedSecurity cbs(session); + Azure::Core::Context openContext; + openContext.Cancel(); + CbsOpenResult openResult = cbs.Open(openContext); + EXPECT_EQ(CbsOpenResult::Cancelled, openResult); + } + mockServer.StopListening(); + } + #endif // !defined(AZ_PLATFORM_MAC) #if !defined(AZ_PLATFORM_MAC) @@ -157,9 +185,46 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { GTEST_LOG_(INFO) << "Open Completed."; mockServer.ForceCbsError(true); - EXPECT_ANY_THROW( - auto putResult = cbs.PutToken( - Azure::Core::Amqp::_detail::CbsTokenType::Sas, "of one", "stringizedToken");); + auto putResult = cbs.PutToken( + Azure::Core::Amqp::_detail::CbsTokenType::Sas, "of one", "stringizedToken"); + EXPECT_EQ(CbsOperationResult::Failed, std::get<0>(putResult)); + cbs.Close(); + } + + mockServer.StopListening(); + } + } + + TEST_F(TestCbs, CbsOpenAndPutCancelled) + { + { + MessageTests::AmqpServerMock mockServer; + + ConnectionOptions options; + options.Port = mockServer.GetPort(); + options.EnableTrace = true; + Connection connection("localhost", nullptr, options); + Session session{connection.CreateSession()}; + + mockServer.StartListening(); + + { + ClaimsBasedSecurity cbs(session); + + EXPECT_EQ(CbsOpenResult::Ok, cbs.Open()); + GTEST_LOG_(INFO) << "Open Completed."; + + Azure::Core::Context putContext; + putContext.Cancel(); + + // mockServer.ForceCbsError(true); + EXPECT_EQ( + CbsOperationResult::Cancelled, + std::get<0>(cbs.PutToken( + Azure::Core::Amqp::_detail::CbsTokenType::Sas, + "of one", + "stringizedToken", + putContext))); cbs.Close(); } @@ -167,5 +232,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { mockServer.StopListening(); } } + #endif // !defined(AZ_PLATFORM_MAC) }}}} // namespace Azure::Core::Amqp::Tests From 198af855cd8a722083f5996678579b0fb453ebbd Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 12:20:36 -0800 Subject: [PATCH 19/24] compiler didnt notice an impossible branch --- sdk/core/azure-core-amqp/src/amqp/management.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sdk/core/azure-core-amqp/src/amqp/management.cpp b/sdk/core/azure-core-amqp/src/amqp/management.cpp index 469a5bda68..2b1f4fa5cb 100644 --- a/sdk/core/azure-core-amqp/src/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/management.cpp @@ -236,6 +236,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { case _internal::MessageSendStatus::Timeout: rv.Status = _internal::ManagementOperationStatus::Error; break; + case _internal::MessageSendStatus::Ok: + AZURE_ASSERT_MSG(false, "MessageSendStatus::Ok is not a failure status."); + break; } rv.StatusCode = 500; rv.Error = sendError; From 60318a38bcbd3333eef85abff5b67ab646c79c80 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 12:41:39 -0800 Subject: [PATCH 20/24] clang-format --- sdk/core/azure-core-amqp/src/amqp/connection.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/connection.cpp b/sdk/core/azure-core-amqp/src/amqp/connection.cpp index 409ac59bcf..2b0088e030 100644 --- a/sdk/core/azure-core-amqp/src/amqp/connection.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/connection.cpp @@ -678,8 +678,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { if (std::get<0>(result) != CbsOperationResult::Ok) { throw Azure::Core::Credentials::AuthenticationException( - "Could not authenticate client. Error Status: " - + std::to_string(std::get<1>(result)) + " reason: " + std::get<2>(result)); + "Could not authenticate client. Error Status: " + std::to_string(std::get<1>(result)) + + " reason: " + std::get<2>(result)); } Log::Stream(Logger::Level::Verbose) << "Close CBS object"; claimsBasedSecurity->Close(context); From a373ae57de6003dc85c4e4fb169712cf26598baf Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 13:34:47 -0800 Subject: [PATCH 21/24] Better code coverage --- .../test/ut/amqp_value_tests.cpp | 30 +++++++++++++++++++ .../test/ut/claim_based_security_tests.cpp | 2 ++ .../test/ut_uamqp/uamqp_value_tests.cpp | 27 +++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp index ab06627c28..9815594dae 100644 --- a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp @@ -31,10 +31,13 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value; EXPECT_EQ(AmqpValueType::Null, value.GetType()); } + { AmqpValue value{true}; EXPECT_EQ(AmqpValueType::Bool, value.GetType()); EXPECT_TRUE(value); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + } { AmqpValue value{false}; @@ -43,6 +46,7 @@ TEST_F(TestValues, SimpleCreate) EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); EXPECT_ANY_THROW((void)static_cast(value)); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); } { EXPECT_LT(AmqpValue(false), AmqpValue(true)); @@ -50,6 +54,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{}; EXPECT_TRUE(value.IsNull()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); } { @@ -57,6 +62,7 @@ TEST_F(TestValues, SimpleCreate) EXPECT_EQ(AmqpValueType::Byte, value.GetType()); EXPECT_EQ(-17, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_LT(AmqpValue{static_cast(-18)}, value); EXPECT_ANY_THROW((void)static_cast(value)); EXPECT_ANY_THROW((void)static_cast(value)); @@ -75,6 +81,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(255)}; EXPECT_EQ(AmqpValueType::Ubyte, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(255, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -84,6 +91,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{'D'}; EXPECT_EQ(AmqpValueType::Byte, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(static_cast(68), static_cast(value)); EXPECT_TRUE(AmqpValue() < value); char ch{value}; @@ -95,6 +103,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(65535)}; EXPECT_EQ(AmqpValueType::Ushort, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(65535, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -103,6 +112,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(32767)}; EXPECT_EQ(AmqpValueType::Short, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(32767, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -112,6 +122,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(32); EXPECT_EQ(AmqpValueType::Int, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(32, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -120,6 +131,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(32u); EXPECT_EQ(AmqpValueType::Uint, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(32u, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -129,6 +141,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(static_cast(32ll)); EXPECT_EQ(AmqpValueType::Long, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(32ll, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -137,6 +150,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(static_cast(39ull)); EXPECT_EQ(AmqpValueType::Ulong, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(39ull, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -146,6 +160,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0f); EXPECT_EQ(AmqpValueType::Float, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(39.0f, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -154,6 +169,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0); EXPECT_EQ(AmqpValueType::Double, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(39.0, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -162,6 +178,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); double d{value}; EXPECT_EQ(39.0, d); EXPECT_TRUE(AmqpValue() < value); @@ -171,6 +188,7 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value(std::string("Fred")); std::string fredP(value); EXPECT_EQ(AmqpValueType::String, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(std::string("Fred"), fredP); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -180,6 +198,7 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value("Fred"); std::string fredP(value); EXPECT_EQ(AmqpValueType::String, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(std::string("Fred"), fredP); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -189,6 +208,7 @@ TEST_F(TestValues, SimpleCreate) Azure::Core::Uuid uuid = Azure::Core::Uuid::CreateUuid(); AmqpValue value(uuid); EXPECT_EQ(AmqpValueType::Uuid, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(uuid.ToString(), static_cast(value).ToString()); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -215,6 +235,7 @@ TEST_F(TestValues, TestBinary) binaryData.push_back('a'); binaryData.push_back(3); AmqpValue value{binaryData.AsAmqpValue()}; + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_FALSE(value < binaryData.AsAmqpValue()); @@ -251,6 +272,7 @@ TEST_F(TestValues, TestList) EXPECT_EQ(AmqpValue('a'), list1[3]); AmqpValue value{list1.AsAmqpValue()}; + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); const AmqpList list2(value); EXPECT_FALSE(value < list1.AsAmqpValue()); @@ -310,6 +332,7 @@ TEST_F(TestValues, TestMap) // Now round-trip the map through an AMQP value and confirm that the values persist. AmqpValue valueOfMap = map1.AsAmqpValue(); + GTEST_LOG_(INFO) << "Value Type: " << valueOfMap.GetType(); AmqpMap map2(valueOfMap); EXPECT_FALSE(valueOfMap < map1.AsAmqpValue()); @@ -327,6 +350,7 @@ TEST_F(TestValues, TestArray) AmqpValue value = array1.AsAmqpValue(); EXPECT_EQ(AmqpValueType::Array, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); const AmqpArray array2 = value.AsArray(); EXPECT_EQ(5, array2.size()); @@ -351,6 +375,7 @@ TEST_F(TestValues, TestChar) { { AmqpValue value{U'\U0001f34c'}; + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); EXPECT_EQ(U'\U0001f34c', static_cast(value)); EXPECT_EQ(AmqpValueType::Char, value.GetType()); EXPECT_FALSE(static_cast(value) < U'\U0001f34c'); @@ -369,6 +394,7 @@ TEST_F(TestValues, TestTimestamp) AmqpTimestamp value{timeNow}; EXPECT_EQ(static_cast(value), timeNow); AmqpValue av{value.AsAmqpValue()}; + GTEST_LOG_(INFO) << "Value Type: " << av.GetType(); AmqpTimestamp ts2{av.AsTimestamp()}; EXPECT_EQ(timeNow, static_cast(ts2)); @@ -387,6 +413,8 @@ TEST_F(TestValues, TestSymbol) EXPECT_EQ(value, "timeNow"); EXPECT_FALSE(value < AmqpSymbol("timeNow")); GTEST_LOG_(INFO) << "Symbol value: " << value; + AmqpValue av{value.AsAmqpValue()}; + GTEST_LOG_(INFO) << "Value Type: " << av.GetType(); } { AmqpValue boolValue{false}; @@ -425,6 +453,7 @@ TEST_F(TestValues, TestCompositeValue) { AmqpComposite compositeVal(static_cast(116ull), {25, 25.0f}); AmqpValue value = compositeVal.AsAmqpValue(); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); AmqpComposite testVal(value.AsComposite()); EXPECT_EQ(compositeVal.size(), testVal.size()); @@ -447,6 +476,7 @@ TEST_F(TestValues, TestDescribed) AmqpValue value = described1.AsAmqpValue(); EXPECT_EQ(AmqpValueType::Described, value.GetType()); + GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); AmqpDescribed described2 = value.AsDescribed(); EXPECT_EQ(AmqpValueType::Described, value.GetType()); diff --git a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp index e7de12a0f4..3664193c86 100644 --- a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp @@ -47,6 +47,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Invalid; GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Failed; GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::InstanceClosed; + GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Cancelled; + GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Ok; GTEST_LOG_(INFO) << "CbsOperations" << static_cast(32768); } { diff --git a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp index c61a99bce5..cff0abc142 100644 --- a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp @@ -24,6 +24,11 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value; } + GTEST_LOG_(INFO) << AMQP_TYPE_INVALID; + GTEST_LOG_(INFO) << AMQP_TYPE_UNKNOWN; + GTEST_LOG_(INFO) << static_cast(nullptr); + + { _detail::UniqueAmqpValueHandle handle{amqpvalue_create_null()}; @@ -215,4 +220,26 @@ TEST_F(TestValue, SimpleCreate) GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); } + + { + AmqpValue descriptor(235ll); + AmqpValue descriptorValue("Value"); + _detail::UniqueAmqpValueHandle handle{amqpvalue_create_described( + _detail::AmqpValueFactory::ToUamqp(descriptor), _detail::AmqpValueFactory::ToUamqp(descriptorValue))}; + + AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; + GTEST_LOG_(INFO) << "Handle Type: " + << amqpvalue_get_type(amqpvalue_get_inplace_descriptor(handle.get())) + << " Value: " << static_cast(handle.get()); + } + { + AmqpValue descriptor(235ll); + _detail::UniqueAmqpValueHandle handle{ + amqpvalue_create_composite(_detail::AmqpValueFactory::ToUamqp(descriptor), 21)}; + + AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; + GTEST_LOG_(INFO) << "Handle Type: " + << amqpvalue_get_type(amqpvalue_get_inplace_descriptor(handle.get())) + << " Value: " << static_cast(handle.get()); + } } From 29a9c479d0e772e4cded94353569cebdc57f3490 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 14:54:38 -0800 Subject: [PATCH 22/24] clang fixes --- sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp | 1 - .../azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp index 9815594dae..2fce6785e4 100644 --- a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp @@ -37,7 +37,6 @@ TEST_F(TestValues, SimpleCreate) EXPECT_EQ(AmqpValueType::Bool, value.GetType()); EXPECT_TRUE(value); GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); - } { AmqpValue value{false}; diff --git a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp index cff0abc142..bb26ed0a19 100644 --- a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp @@ -28,7 +28,6 @@ TEST_F(TestValue, SimpleCreate) GTEST_LOG_(INFO) << AMQP_TYPE_UNKNOWN; GTEST_LOG_(INFO) << static_cast(nullptr); - { _detail::UniqueAmqpValueHandle handle{amqpvalue_create_null()}; @@ -222,10 +221,11 @@ TEST_F(TestValue, SimpleCreate) } { - AmqpValue descriptor(235ll); + AmqpValue descriptor(static_cast(237ll)); AmqpValue descriptorValue("Value"); _detail::UniqueAmqpValueHandle handle{amqpvalue_create_described( - _detail::AmqpValueFactory::ToUamqp(descriptor), _detail::AmqpValueFactory::ToUamqp(descriptorValue))}; + _detail::AmqpValueFactory::ToUamqp(descriptor), + _detail::AmqpValueFactory::ToUamqp(descriptorValue))}; AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " @@ -233,7 +233,7 @@ TEST_F(TestValue, SimpleCreate) << " Value: " << static_cast(handle.get()); } { - AmqpValue descriptor(235ll); + AmqpValue descriptor(static_cast(235ll)); _detail::UniqueAmqpValueHandle handle{ amqpvalue_create_composite(_detail::AmqpValueFactory::ToUamqp(descriptor), 21)}; From 5209f1f5b6115355121f59ec799b25597018be30 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Thu, 8 Feb 2024 16:16:49 -0800 Subject: [PATCH 23/24] amqpvalue_create_described does not clone its inputs --- sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp index bb26ed0a19..c8d3fb23b5 100644 --- a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp @@ -224,8 +224,8 @@ TEST_F(TestValue, SimpleCreate) AmqpValue descriptor(static_cast(237ll)); AmqpValue descriptorValue("Value"); _detail::UniqueAmqpValueHandle handle{amqpvalue_create_described( - _detail::AmqpValueFactory::ToUamqp(descriptor), - _detail::AmqpValueFactory::ToUamqp(descriptorValue))}; + amqpvalue_clone(_detail::AmqpValueFactory::ToUamqp(descriptor)), + amqpvalue_clone(_detail::AmqpValueFactory::ToUamqp(descriptorValue)))}; AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " From 395e1c3e2088d9ba032ff8a1060af5c2951044e6 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 9 Feb 2024 12:14:32 -0800 Subject: [PATCH 24/24] Added value based tests for enumeration stream inserters --- .../azure-core-amqp/src/amqp/connection.cpp | 28 +++---- .../src/amqp/private/connection_impl.hpp | 2 + .../azure-core-amqp/src/models/amqp_value.cpp | 50 ++++++------ .../test/ut/amqp_value_tests.cpp | 64 ++++++++------- .../test/ut/claim_based_security_tests.cpp | 29 ++++--- .../test/ut_uamqp/uamqp_insertion_tests.cpp | 79 +++++++++++++------ .../test/ut_uamqp/uamqp_value_tests.cpp | 32 ++++++++ 7 files changed, 184 insertions(+), 100 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/amqp/connection.cpp b/sdk/core/azure-core-amqp/src/amqp/connection.cpp index 2b0088e030..1111f2d55a 100644 --- a/sdk/core/azure-core-amqp/src/amqp/connection.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/connection.cpp @@ -340,22 +340,22 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { {CONNECTION_STATE_ERROR, "CONNECTION_STATE_ERROR"}, }; - std::ostream& operator<<(std::ostream& os, CONNECTION_STATE state) + } // namespace + + std::ostream& operator<<(std::ostream& os, CONNECTION_STATE state) + { + auto val{UamqpConnectionStateToStringMap.find(state)}; + if (val == UamqpConnectionStateToStringMap.end()) { - auto val{UamqpConnectionStateToStringMap.find(state)}; - if (val == UamqpConnectionStateToStringMap.end()) - { - os << "Unknown connection state: " - << static_cast::type>(state); - } - else - { - os << val->second << "(" << static_cast::type>(state) - << ")"; - } - return os; + os << "Unknown connection state: " + << static_cast::type>(state); } - } // namespace + else + { + os << val->second; + } + return os; + } _internal::ConnectionState ConnectionStateFromCONNECTION_STATE(CONNECTION_STATE state) { diff --git a/sdk/core/azure-core-amqp/src/amqp/private/connection_impl.hpp b/sdk/core/azure-core-amqp/src/amqp/private/connection_impl.hpp index 56fe61ea5d..7a512a614f 100644 --- a/sdk/core/azure-core-amqp/src/amqp/private/connection_impl.hpp +++ b/sdk/core/azure-core-amqp/src/amqp/private/connection_impl.hpp @@ -35,6 +35,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { namespace Azure { namespace Core { namespace Amqp { namespace _detail { using UniqueAmqpConnection = UniqueHandle; + std::ostream& operator<<(std::ostream& os, CONNECTION_STATE state); + class ClaimsBasedSecurity; class ConnectionFactory final { diff --git a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp index ab6cd65189..17637c919b 100644 --- a/sdk/core/azure-core-amqp/src/models/amqp_value.cpp +++ b/sdk/core/azure-core-amqp/src/models/amqp_value.cpp @@ -51,79 +51,79 @@ namespace Azure { namespace Core { namespace Amqp { namespace Models { switch (value) { case AMQP_TYPE_INVALID: - os << "INVALID"; + os << "AMQP_TYPE_INVALID"; break; case AMQP_TYPE_NULL: - os << "NULL"; + os << "AMQP_TYPE_NULL"; break; case AMQP_TYPE_BOOL: - os << "BOOL"; + os << "AMQP_TYPE_BOOL"; break; case AMQP_TYPE_UBYTE: - os << "UBYTE"; + os << "AMQP_TYPE_UBYTE"; break; case AMQP_TYPE_USHORT: - os << "USHORT"; + os << "AMQP_TYPE_USHORT"; break; case AMQP_TYPE_UINT: - os << "UINT"; + os << "AMQP_TYPE_UINT"; break; case AMQP_TYPE_ULONG: - os << "ULONG"; + os << "AMQP_TYPE_ULONG"; break; case AMQP_TYPE_BYTE: - os << "BYTE"; + os << "AMQP_TYPE_BYTE"; break; case AMQP_TYPE_SHORT: - os << "SHORT"; + os << "AMQP_TYPE_SHORT"; break; case AMQP_TYPE_INT: - os << "INT"; + os << "AMQP_TYPE_INT"; break; case AMQP_TYPE_LONG: - os << "LONG"; + os << "AMQP_TYPE_LONG"; break; case AMQP_TYPE_FLOAT: - os << "FLOAT"; + os << "AMQP_TYPE_FLOAT"; break; case AMQP_TYPE_DOUBLE: - os << "DOUBLE"; + os << "AMQP_TYPE_DOUBLE"; break; case AMQP_TYPE_CHAR: - os << "CHAR"; + os << "AMQP_TYPE_CHAR"; break; case AMQP_TYPE_TIMESTAMP: - os << "TIMESTAMP"; + os << "AMQP_TYPE_TIMESTAMP"; break; case AMQP_TYPE_UUID: - os << "UUID"; + os << "AMQP_TYPE_UUID"; break; case AMQP_TYPE_BINARY: - os << "BINARY"; + os << "AMQP_TYPE_BINARY"; break; case AMQP_TYPE_STRING: - os << "STRING"; + os << "AMQP_TYPE_STRING"; break; case AMQP_TYPE_SYMBOL: - os << "SYMBOL"; + os << "AMQP_TYPE_SYMBOL"; break; case AMQP_TYPE_LIST: - os << "LIST"; + os << "AMQP_TYPE_LIST"; break; case AMQP_TYPE_MAP: - os << "MAP"; + os << "AMQP_TYPE_MAP"; break; case AMQP_TYPE_ARRAY: - os << "ARRAY"; + os << "AMQP_TYPE_ARRAY"; break; case AMQP_TYPE_DESCRIBED: - os << "DESCRIBED"; + os << "AMQP_TYPE_DESCRIBED"; break; case AMQP_TYPE_COMPOSITE: - os << "COMPOSITE"; + os << "AMQP_TYPE_COMPOSITE"; break; case AMQP_TYPE_UNKNOWN: - os << "UNKNOWN"; + os << "AMQP_TYPE_UNKNOWN"; break; } return os; diff --git a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp index 2fce6785e4..2d8271deaa 100644 --- a/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/amqp_value_tests.cpp @@ -25,6 +25,13 @@ class TestValues : public testing::Test { void TearDown() override {} }; +#define TEST_OSTREAM_INSERTER(VALUE, EXPECTED) \ + { \ + std::stringstream ss; \ + ss << VALUE.GetType(); \ + EXPECT_EQ(EXPECTED, ss.str()); \ + } + TEST_F(TestValues, SimpleCreate) { { @@ -36,7 +43,7 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value{true}; EXPECT_EQ(AmqpValueType::Bool, value.GetType()); EXPECT_TRUE(value); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Bool"); } { AmqpValue value{false}; @@ -45,7 +52,7 @@ TEST_F(TestValues, SimpleCreate) EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); EXPECT_ANY_THROW((void)static_cast(value)); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Bool"); } { EXPECT_LT(AmqpValue(false), AmqpValue(true)); @@ -53,7 +60,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{}; EXPECT_TRUE(value.IsNull()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Null"); } { @@ -61,7 +68,7 @@ TEST_F(TestValues, SimpleCreate) EXPECT_EQ(AmqpValueType::Byte, value.GetType()); EXPECT_EQ(-17, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Byte"); EXPECT_LT(AmqpValue{static_cast(-18)}, value); EXPECT_ANY_THROW((void)static_cast(value)); EXPECT_ANY_THROW((void)static_cast(value)); @@ -80,7 +87,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(255)}; EXPECT_EQ(AmqpValueType::Ubyte, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Ubyte"); EXPECT_EQ(255, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -90,7 +97,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{'D'}; EXPECT_EQ(AmqpValueType::Byte, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Byte"); EXPECT_EQ(static_cast(68), static_cast(value)); EXPECT_TRUE(AmqpValue() < value); char ch{value}; @@ -102,7 +109,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(65535)}; EXPECT_EQ(AmqpValueType::Ushort, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Ushort"); EXPECT_EQ(65535, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -111,7 +118,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value{static_cast(32767)}; EXPECT_EQ(AmqpValueType::Short, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Short"); EXPECT_EQ(32767, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -121,7 +128,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(32); EXPECT_EQ(AmqpValueType::Int, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Int"); EXPECT_EQ(32, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -130,7 +137,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(32u); EXPECT_EQ(AmqpValueType::Uint, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Uint"); EXPECT_EQ(32u, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -140,7 +147,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(static_cast(32ll)); EXPECT_EQ(AmqpValueType::Long, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Long"); EXPECT_EQ(32ll, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -149,7 +156,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(static_cast(39ull)); EXPECT_EQ(AmqpValueType::Ulong, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Ulong"); EXPECT_EQ(39ull, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -159,7 +166,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0f); EXPECT_EQ(AmqpValueType::Float, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Float"); EXPECT_EQ(39.0f, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -168,7 +175,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0); EXPECT_EQ(AmqpValueType::Double, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Double"); EXPECT_EQ(39.0, static_cast(value)); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -177,7 +184,7 @@ TEST_F(TestValues, SimpleCreate) { AmqpValue value(39.0); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Double"); double d{value}; EXPECT_EQ(39.0, d); EXPECT_TRUE(AmqpValue() < value); @@ -187,7 +194,7 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value(std::string("Fred")); std::string fredP(value); EXPECT_EQ(AmqpValueType::String, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "String"); EXPECT_EQ(std::string("Fred"), fredP); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -197,7 +204,7 @@ TEST_F(TestValues, SimpleCreate) AmqpValue value("Fred"); std::string fredP(value); EXPECT_EQ(AmqpValueType::String, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "String"); EXPECT_EQ(std::string("Fred"), fredP); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -207,7 +214,7 @@ TEST_F(TestValues, SimpleCreate) Azure::Core::Uuid uuid = Azure::Core::Uuid::CreateUuid(); AmqpValue value(uuid); EXPECT_EQ(AmqpValueType::Uuid, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Uuid"); EXPECT_EQ(uuid.ToString(), static_cast(value).ToString()); EXPECT_TRUE(AmqpValue() < value); EXPECT_ANY_THROW((void)static_cast(value)); @@ -234,7 +241,8 @@ TEST_F(TestValues, TestBinary) binaryData.push_back('a'); binaryData.push_back(3); AmqpValue value{binaryData.AsAmqpValue()}; - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + EXPECT_EQ(AmqpValueType::Binary, value.GetType()); + TEST_OSTREAM_INSERTER(value, "Binary"); EXPECT_FALSE(value < binaryData.AsAmqpValue()); @@ -271,7 +279,7 @@ TEST_F(TestValues, TestList) EXPECT_EQ(AmqpValue('a'), list1[3]); AmqpValue value{list1.AsAmqpValue()}; - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "List"); const AmqpList list2(value); EXPECT_FALSE(value < list1.AsAmqpValue()); @@ -293,6 +301,7 @@ TEST_F(TestValues, TestList) test.push_back(desc.AsAmqpValue()); EXPECT_EQ(1, test.size()); EXPECT_EQ(AmqpValueType::Described, test[0].GetType()); + TEST_OSTREAM_INSERTER(test[0], "Described"); AmqpList list2{test}; EXPECT_EQ(1, list2.size()); @@ -331,7 +340,7 @@ TEST_F(TestValues, TestMap) // Now round-trip the map through an AMQP value and confirm that the values persist. AmqpValue valueOfMap = map1.AsAmqpValue(); - GTEST_LOG_(INFO) << "Value Type: " << valueOfMap.GetType(); + TEST_OSTREAM_INSERTER(valueOfMap, "Map"); AmqpMap map2(valueOfMap); EXPECT_FALSE(valueOfMap < map1.AsAmqpValue()); @@ -349,7 +358,7 @@ TEST_F(TestValues, TestArray) AmqpValue value = array1.AsAmqpValue(); EXPECT_EQ(AmqpValueType::Array, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Array"); const AmqpArray array2 = value.AsArray(); EXPECT_EQ(5, array2.size()); @@ -374,7 +383,7 @@ TEST_F(TestValues, TestChar) { { AmqpValue value{U'\U0001f34c'}; - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Char"); EXPECT_EQ(U'\U0001f34c', static_cast(value)); EXPECT_EQ(AmqpValueType::Char, value.GetType()); EXPECT_FALSE(static_cast(value) < U'\U0001f34c'); @@ -393,7 +402,7 @@ TEST_F(TestValues, TestTimestamp) AmqpTimestamp value{timeNow}; EXPECT_EQ(static_cast(value), timeNow); AmqpValue av{value.AsAmqpValue()}; - GTEST_LOG_(INFO) << "Value Type: " << av.GetType(); + TEST_OSTREAM_INSERTER(av, "Timestamp"); AmqpTimestamp ts2{av.AsTimestamp()}; EXPECT_EQ(timeNow, static_cast(ts2)); @@ -411,9 +420,8 @@ TEST_F(TestValues, TestSymbol) AmqpSymbol value("timeNow"); EXPECT_EQ(value, "timeNow"); EXPECT_FALSE(value < AmqpSymbol("timeNow")); - GTEST_LOG_(INFO) << "Symbol value: " << value; AmqpValue av{value.AsAmqpValue()}; - GTEST_LOG_(INFO) << "Value Type: " << av.GetType(); + TEST_OSTREAM_INSERTER(av, "Symbol"); } { AmqpValue boolValue{false}; @@ -452,7 +460,7 @@ TEST_F(TestValues, TestCompositeValue) { AmqpComposite compositeVal(static_cast(116ull), {25, 25.0f}); AmqpValue value = compositeVal.AsAmqpValue(); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Composite"); AmqpComposite testVal(value.AsComposite()); EXPECT_EQ(compositeVal.size(), testVal.size()); @@ -475,7 +483,7 @@ TEST_F(TestValues, TestDescribed) AmqpValue value = described1.AsAmqpValue(); EXPECT_EQ(AmqpValueType::Described, value.GetType()); - GTEST_LOG_(INFO) << "Value Type: " << value.GetType(); + TEST_OSTREAM_INSERTER(value, "Described"); AmqpDescribed described2 = value.AsDescribed(); EXPECT_EQ(AmqpValueType::Described, value.GetType()); diff --git a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp index 3664193c86..5631c74694 100644 --- a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp @@ -24,6 +24,14 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { using namespace Azure::Core::Amqp::_detail; #if !defined(AZ_PLATFORM_MAC) + +#define TEST_OSTREAM_INSERTER(ENUMERATION, ENUMERATOR) \ + { \ + std::stringstream ss; \ + ss << ENUMERATION::ENUMERATOR; \ + EXPECT_EQ(#ENUMERATOR, ss.str()); \ + } + TEST_F(TestCbs, SimpleCbs) { @@ -43,19 +51,20 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { } { - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Error; - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Invalid; - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Failed; - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::InstanceClosed; - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Cancelled; - GTEST_LOG_(INFO) << "CbsOperations" << CbsOperationResult::Ok; + TEST_OSTREAM_INSERTER(CbsOperationResult, Failed); + TEST_OSTREAM_INSERTER(CbsOperationResult, Ok); + TEST_OSTREAM_INSERTER(CbsOperationResult, Cancelled); + TEST_OSTREAM_INSERTER(CbsOperationResult, InstanceClosed); + TEST_OSTREAM_INSERTER(CbsOperationResult, Invalid); + TEST_OSTREAM_INSERTER(CbsOperationResult, Error); GTEST_LOG_(INFO) << "CbsOperations" << static_cast(32768); } { - GTEST_LOG_(INFO) << "CbsOpens" << CbsOpenResult::Cancelled; - GTEST_LOG_(INFO) << "CbsOpens" << CbsOpenResult::Error; - GTEST_LOG_(INFO) << "CbsOpens" << CbsOpenResult::Ok; - GTEST_LOG_(INFO) << "CbsOpens" << CbsOpenResult::Invalid; + TEST_OSTREAM_INSERTER(CbsOpenResult, Ok); + TEST_OSTREAM_INSERTER(CbsOpenResult, Cancelled); + TEST_OSTREAM_INSERTER(CbsOpenResult, Invalid); + TEST_OSTREAM_INSERTER(CbsOpenResult, Error); + GTEST_LOG_(INFO) << "CbsOpens" << static_cast(32768); } } diff --git a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_insertion_tests.cpp b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_insertion_tests.cpp index 5d11138465..5dd41837a7 100644 --- a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_insertion_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_insertion_tests.cpp @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#include "../../src/amqp/private/connection_impl.hpp" + #include #include @@ -11,35 +13,66 @@ #include -class TestUAMQPInsertions : public testing::Test { -protected: - void SetUp() override {} - void TearDown() override {} -}; +namespace Azure { namespace Core { namespace Amqp { namespace _detail { namespace Tests { + + class TestUAMQPInsertions : public testing::Test { + protected: + void SetUp() override {} + void TearDown() override {} + }; -TEST_F(TestUAMQPInsertions, TestInsertions) -{ +#define TEST_OSTREAM_INSERTER(ENUMERATION, ENUMERATOR) \ + { \ + std::stringstream ss; \ + ss << ENUMERATION::ENUMERATOR; \ + EXPECT_EQ(#ENUMERATOR, ss.str()); \ + } + TEST_F(TestUAMQPInsertions, TestInsertions) { - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::Start; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::HeaderReceived; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::HeaderSent; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::HeaderExchanged; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::OpenPipe; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::OcPipe; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::OpenReceived; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::OpenSent; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::ClosePipe; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::CloseReceived; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::CloseSent; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::Discarding; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::End; - GTEST_LOG_(INFO) << Azure::Core::Amqp::_internal::ConnectionState::Error; + { + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, Start); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, HeaderReceived); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, HeaderSent); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, HeaderExchanged); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, OpenPipe); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, OcPipe); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, OpenReceived); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, OpenSent); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, ClosePipe); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, CloseReceived); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, CloseSent); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, Discarding); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, End); + TEST_OSTREAM_INSERTER(Azure::Core::Amqp::_internal::ConnectionState, Error); + } + +#define TEST_C_INSERTER(ENUMERATOR) \ + { \ + std::stringstream ss; \ + ss << ENUMERATOR; \ + EXPECT_EQ(#ENUMERATOR, ss.str()); \ } + { + TEST_C_INSERTER(CONNECTION_STATE_START); + TEST_C_INSERTER(CONNECTION_STATE_HDR_RCVD); + TEST_C_INSERTER(CONNECTION_STATE_HDR_SENT); + TEST_C_INSERTER(CONNECTION_STATE_HDR_EXCH); + TEST_C_INSERTER(CONNECTION_STATE_OPEN_PIPE); + TEST_C_INSERTER(CONNECTION_STATE_OC_PIPE); + TEST_C_INSERTER(CONNECTION_STATE_OPEN_RCVD); + TEST_C_INSERTER(CONNECTION_STATE_OPEN_SENT); + TEST_C_INSERTER(CONNECTION_STATE_CLOSE_PIPE); + TEST_C_INSERTER(CONNECTION_STATE_OPENED); + TEST_C_INSERTER(CONNECTION_STATE_CLOSE_RCVD); + TEST_C_INSERTER(CONNECTION_STATE_CLOSE_SENT); + TEST_C_INSERTER(CONNECTION_STATE_DISCARDING); + TEST_C_INSERTER(CONNECTION_STATE_END); + TEST_C_INSERTER(CONNECTION_STATE_ERROR); + } - { CONNECTION_STATE state; GTEST_LOG_(INFO) << CONNECTION_STATE_ERROR; state = static_cast(3257); GTEST_LOG_(INFO) << state; } -} +}}}}} // namespace Azure::Core::Amqp::_detail::Tests diff --git a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp index c8d3fb23b5..0a7c2f2a3d 100644 --- a/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut_uamqp/uamqp_value_tests.cpp @@ -18,6 +18,13 @@ class TestValue : public testing::Test { void TearDown() override {} }; +#define TEST_C_INSERTER(VALUE, ENUMERATOR) \ + { \ + std::stringstream ss; \ + ss << amqpvalue_get_type(VALUE); \ + EXPECT_EQ(#ENUMERATOR, ss.str()); \ + } + TEST_F(TestValue, SimpleCreate) { { @@ -34,6 +41,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_NULL); } { @@ -42,6 +50,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_BYTE); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_boolean, bool, bool_value); @@ -51,6 +60,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_BOOL); } { _detail::UniqueAmqpValueHandle handle{amqpvalue_create_boolean(false)}; @@ -58,6 +68,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_BOOL); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_ubyte, unsigned char, ubyte_value); @@ -67,6 +78,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_UBYTE); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_ushort, uint16_t, ushort_value); @@ -76,6 +88,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_USHORT); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_uint, uint32_t, uint_value); { @@ -84,6 +97,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_UINT); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_ulong, uint64_t, ulong_value); { @@ -92,6 +106,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_ULONG); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_byte, char, byte_value); { @@ -100,6 +115,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_BYTE); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_short, int16_t, short_value); @@ -109,6 +125,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_SHORT); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_int, int32_t, int_value); { @@ -117,6 +134,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_INT); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_long, int64_t, long_value); { @@ -125,6 +143,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_LONG); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_float, float, float_value); { @@ -133,6 +152,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_FLOAT); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_double, double, double_value); { @@ -141,6 +161,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_DOUBLE); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_char, uint32_t, char_value); { @@ -149,6 +170,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_CHAR); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_timestamp, int64_t, timestamp_value); { @@ -157,6 +179,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_TIMESTAMP); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_uuid, uuid, uuid_value); { @@ -167,6 +190,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_UUID); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_binary, amqp_binary, binary_value); { @@ -178,6 +202,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_BINARY); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_string, const char*, string_value); { @@ -186,6 +211,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_STRING); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_symbol, const char*, symbol_value); { @@ -194,6 +220,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_SYMBOL); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_list); { @@ -202,6 +229,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_LIST); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_map); { @@ -210,6 +238,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_MAP); } // MOCKABLE_FUNCTION(, AMQP_VALUE, amqpvalue_create_array); { @@ -218,6 +247,7 @@ TEST_F(TestValue, SimpleCreate) AmqpValue value{_detail::AmqpValueFactory::FromUamqp(handle)}; GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(handle.get()) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_ARRAY); } { @@ -231,6 +261,7 @@ TEST_F(TestValue, SimpleCreate) GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(amqpvalue_get_inplace_descriptor(handle.get())) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_DESCRIBED); } { AmqpValue descriptor(static_cast(235ll)); @@ -241,5 +272,6 @@ TEST_F(TestValue, SimpleCreate) GTEST_LOG_(INFO) << "Handle Type: " << amqpvalue_get_type(amqpvalue_get_inplace_descriptor(handle.get())) << " Value: " << static_cast(handle.get()); + TEST_C_INSERTER(handle.get(), AMQP_TYPE_COMPOSITE); } }