Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed builders from eventhubs; removed impl Into<String> from eventhubs and AMQP; Fixed time conversion issue with 1-1-0001 #1892

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

LarryOsterman
Copy link
Member

@LarryOsterman LarryOsterman commented Nov 4, 2024

A lot of churn, basically boiling down to 4 changes, related to recent API guideline changes:

  1. Removed most builders from Eventhubs API surface (EventData builder is still there because EventData is a fairly complicated structure).
  2. Removed most impl Into references from both EventHubs and AMQP.
  3. Fixed a time conversion with the 1-1-0001 sentinel value EventHubs uses to represent times that aren't available yet.
  4. Added the ability to specify a ServiceBus token to the AMQP layer to allow C++ to continue to use Connection Strings.

CoPilot summary

This pull request includes several changes to the sdk/core/azure_core_amqp module, focusing on improving type consistency and handling optional values. The most important changes involve updating method signatures to use concrete types instead of generics and adding support for an optional token type in authorization methods.

Type Consistency Improvements:

  • Updated method signatures in AmqpClaimsBasedSecurityApis, AmqpConnectionApis, AmqpManagementApis, and AmqpSenderApis to use concrete String types instead of generics like impl Into<String>. (sdk/core/azure_core_amqp/src/cbs.rs, sdk/core/azure_core_amqp/src/connection.rs, sdk/core/azure_core_amqp/src/fe2o3/cbs.rs, sdk/core/azure_core_amqp/src/fe2o3/connection.rs, sdk/core/azure_core_amqp/src/fe2o3/management.rs, sdk/core/azure_core_amqp/src/fe2o3/sender.rs, sdk/core/azure_core_amqp/src/management.rs) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]

Optional Token Type Support:

  • Added an optional token_type parameter to the authorize_path method in AmqpClaimsBasedSecurityApis to allow specifying the type of token used for authorization. If not supplied, "jwt" is assumed. (sdk/core/azure_core_amqp/src/cbs.rs, sdk/core/azure_core_amqp/src/fe2o3/cbs.rs) [1] [2]

Code Simplification:

  • Simplified the conversion logic in fe2o3_amqp_types by using direct String values and handling optional values more explicitly. (sdk/core/azure_core_amqp/src/fe2o3/messaging/message_fields.rs, sdk/core/azure_core_amqp/src/fe2o3/messaging/message_source.rs, sdk/core/azure_core_amqp/src/fe2o3/value.rs) [1] [2] [3] [4] [5] [6] [7] [8] [9]

Comment Updates:

  • Updated comments to reflect the changes, including the addition of sastoken to the list of spell-checked words. (sdk/core/azure_core_amqp/src/cbs.rs)

These changes collectively enhance the codebase by making it more consistent and easier to understand, while also adding flexibility for handling different token types in authorization scenarios.

sdk/core/azure_core_amqp/src/fe2o3/cbs.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/value.rs Show resolved Hide resolved
sdk/core/azure_core_amqp/src/value.rs Outdated Show resolved Hide resolved
sdk/eventhubs/azure_messaging_eventhubs/README.md Outdated Show resolved Hide resolved
sdk/eventhubs/azure_messaging_eventhubs/src/common/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better - almost there - but a couple outstanding questions and requested changes.

sdk/core/azure_core_amqp/src/connection.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/session.rs Show resolved Hide resolved
sdk/eventhubs/azure_messaging_eventhubs/src/common/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just some thoughts about symmetrically implementing PartialEq you could probably avoid.

sdk/core/azure_core_amqp/src/value.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/value.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants