Skip to content

Commit

Permalink
Fixing a bug in management client that is sending atom xml elements o…
Browse files Browse the repository at this point in the history 
…ut of order (Azure#16488)

* Fixing a bug in management client that is sending atom xml elements in enity description
out of order in an update call. Because of this bug, deserializer on the service ignores
these out of order elements, and persists elements with default values in the righr order
resulting in these out of order elements getting duplicated in the resource xml.
This fix removes duplicate elements only if they are present from the xml, and sends
xml elements in the correct order in update call.

* Removing the part that is added to remove duplicate elements, as I realize it is not required.

* Correcting the order of IsAnonymousAccessbile and AuthorizationRules in QueueDescription.
  • Loading branch information
@yvgopal
yvgopal authored Nov 3, 2020
1 parent a6c7ec4 commit 5db2963
Show file tree
Hide file tree
Showing 7 changed files with 226 additions and 50 deletions.
23 changes: 22 additions & 1 deletion sdk/servicebus/Microsoft.Azure.ServiceBus/src/Management/QueueDescription.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.Azure.ServiceBus.Management
{
using System;
using System.Collections.Generic;
using System.Xml.Linq;
using Microsoft.Azure.ServiceBus.Primitives;

/// <summary>
Expand All @@ -14,6 +15,7 @@ public class QueueDescription : IEquatable<QueueDescription>
{
internal TimeSpan duplicateDetectionHistoryTimeWindow = TimeSpan.FromMinutes(1);
internal string path;
internal bool? internalSupportOrdering = null;
TimeSpan lockDuration = TimeSpan.FromSeconds(60);
TimeSpan defaultMessageTimeToLive = TimeSpan.MaxValue;
TimeSpan autoDeleteOnIdle = TimeSpan.MaxValue;
Expand Down Expand Up @@ -273,11 +275,27 @@ public string UserMetadata
}
}

internal bool IsAnonymousAccessible { get; set; } = false;

internal bool SupportOrdering
{
get
{
return this.internalSupportOrdering ?? !this.EnablePartitioning;
}
set
{
this.internalSupportOrdering = value;
}
}

internal bool EnableExpress { get; set; } = false;

/// <summary>
/// List of properties that were retrieved using GetQueue but are not understood by this version of client is stored here.
/// The list will be sent back when an already retrieved QueueDescription will be used in UpdateQueue call.
/// </summary>
internal List<object> UnknownProperties { get; set; }
internal List<XElement> UnknownProperties { get; set; }

public override int GetHashCode()
{
Expand Down Expand Up @@ -308,6 +326,9 @@ public bool Equals(QueueDescription otherDescription)
&& this.RequiresDuplicateDetection.Equals(other.RequiresDuplicateDetection)
&& this.RequiresSession.Equals(other.RequiresSession)
&& this.Status.Equals(other.Status)
&& this.internalSupportOrdering.Equals(other.SupportOrdering)
&& this.EnableExpress == other.EnableExpress
&& this.IsAnonymousAccessible == other.IsAnonymousAccessible
&& string.Equals(this.userMetadata, other.userMetadata, StringComparison.OrdinalIgnoreCase)
&& (this.AuthorizationRules != null && other.AuthorizationRules != null
|| this.AuthorizationRules == null && other.AuthorizationRules == null)
Expand Down
57 changes: 35 additions & 22 deletions sdk/servicebus/Microsoft.Azure.ServiceBus/src/Management/QueueDescriptionExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ internal static class QueueDescriptionExtensions
{
public static XDocument Serialize(this QueueDescription description)
{
var queueDescriptionElements = new List<object>()
var queueDescriptionElements = new List<XElement>()
{
new XElement(XName.Get("LockDuration", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.LockDuration)),
new XElement(XName.Get("MaxSizeInMegabytes", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.MaxSizeInMB)),
Expand All @@ -25,15 +25,20 @@ public static XDocument Serialize(this QueueDescription description)
: null,
new XElement(XName.Get("MaxDeliveryCount", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.MaxDeliveryCount)),
new XElement(XName.Get("EnableBatchedOperations", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.EnableBatchedOperations)),
new XElement(XName.Get("IsAnonymousAccessible", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.IsAnonymousAccessible)),
description.AuthorizationRules?.Serialize(),
new XElement(XName.Get("Status", ManagementClientConstants.ServiceBusNamespace), description.Status.ToString()),
description.ForwardTo != null ? new XElement(XName.Get("ForwardTo", ManagementClientConstants.ServiceBusNamespace), description.ForwardTo) : null,
description.UserMetadata != null ? new XElement(XName.Get("UserMetadata", ManagementClientConstants.ServiceBusNamespace), description.UserMetadata) : null,
description.internalSupportOrdering.HasValue ? new XElement(XName.Get("SupportOrdering", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.internalSupportOrdering.Value)) : null,
description.AutoDeleteOnIdle != TimeSpan.MaxValue ? new XElement(XName.Get("AutoDeleteOnIdle", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.AutoDeleteOnIdle)) : null,
new XElement(XName.Get("EnablePartitioning", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.EnablePartitioning)),
description.ForwardDeadLetteredMessagesTo != null ? new XElement(XName.Get("ForwardDeadLetteredMessagesTo", ManagementClientConstants.ServiceBusNamespace), description.ForwardDeadLetteredMessagesTo) : null
description.ForwardDeadLetteredMessagesTo != null ? new XElement(XName.Get("ForwardDeadLetteredMessagesTo", ManagementClientConstants.ServiceBusNamespace), description.ForwardDeadLetteredMessagesTo) : null,
new XElement(XName.Get("EnableExpress", ManagementClientConstants.ServiceBusNamespace), XmlConvert.ToString(description.EnableExpress))
};

// Insert unknown properties in the exact order they were in the received xml.
// Expectation is that servicebus will add any new elements only at the bottom of the xml tree.
if (description.UnknownProperties != null)
{
queueDescriptionElements.AddRange(description.UnknownProperties);
Expand Down Expand Up @@ -71,8 +76,6 @@ public static QueueDescription ParseFromContent(string xml)
private static QueueDescription ParseFromEntryElement(XElement xEntry)
{
var name = xEntry.Element(XName.Get("title", ManagementClientConstants.AtomNamespace)).Value;
var qd = new QueueDescription(name);

var qdXml = xEntry.Element(XName.Get("content", ManagementClientConstants.AtomNamespace))?
.Element(XName.Get("QueueDescription", ManagementClientConstants.ServiceBusNamespace));

Expand All @@ -81,10 +84,14 @@ private static QueueDescription ParseFromEntryElement(XElement xEntry)
throw new MessagingEntityNotFoundException("Queue was not found");
}

var qd = new QueueDescription(name);
foreach (var element in qdXml.Elements())
{
switch (element.Name.LocalName)
{
case "LockDuration":
qd.LockDuration = XmlConvert.ToTimeSpan(element.Value);
break;
case "MaxSizeInMegabytes":
qd.MaxSizeInMB = Int64.Parse(element.Value);
break;
Expand All @@ -94,50 +101,56 @@ private static QueueDescription ParseFromEntryElement(XElement xEntry)
case "RequiresSession":
qd.RequiresSession = Boolean.Parse(element.Value);
break;
case "DefaultMessageTimeToLive":
qd.DefaultMessageTimeToLive = XmlConvert.ToTimeSpan(element.Value);
break;
case "DeadLetteringOnMessageExpiration":
qd.EnableDeadLetteringOnMessageExpiration = Boolean.Parse(element.Value);
break;
case "DuplicateDetectionHistoryTimeWindow":
qd.duplicateDetectionHistoryTimeWindow = XmlConvert.ToTimeSpan(element.Value);
break;
case "LockDuration":
qd.LockDuration = XmlConvert.ToTimeSpan(element.Value);
break;
case "DefaultMessageTimeToLive":
qd.DefaultMessageTimeToLive = XmlConvert.ToTimeSpan(element.Value);
break;
case "MaxDeliveryCount":
qd.MaxDeliveryCount = Int32.Parse(element.Value);
break;
case "EnableBatchedOperations":
qd.EnableBatchedOperations = Boolean.Parse(element.Value);
break;
case "Status":
qd.Status = (EntityStatus)Enum.Parse(typeof(EntityStatus), element.Value);
break;
case "AutoDeleteOnIdle":
qd.AutoDeleteOnIdle = XmlConvert.ToTimeSpan(element.Value);
case "IsAnonymousAccessible":
qd.IsAnonymousAccessible = Boolean.Parse(element.Value);
break;
case "EnablePartitioning":
qd.EnablePartitioning = bool.Parse(element.Value);
case "AuthorizationRules":
qd.AuthorizationRules = AuthorizationRules.ParseFromXElement(element);
break;
case "UserMetadata":
qd.UserMetadata = element.Value;
case "Status":
qd.Status = (EntityStatus)Enum.Parse(typeof(EntityStatus), element.Value);
break;
case "ForwardTo":
if (!string.IsNullOrWhiteSpace(element.Value))
{
qd.ForwardTo = element.Value;
}
break;
case "UserMetadata":
qd.UserMetadata = element.Value;
break;
case "SupportOrdering":
qd.SupportOrdering = Boolean.Parse(element.Value);
break;
case "AutoDeleteOnIdle":
qd.AutoDeleteOnIdle = XmlConvert.ToTimeSpan(element.Value);
break;
case "EnablePartitioning":
qd.EnablePartitioning = bool.Parse(element.Value);
break;
case "ForwardDeadLetteredMessagesTo":
if (!string.IsNullOrWhiteSpace(element.Value))
{
qd.ForwardDeadLetteredMessagesTo = element.Value;
}
break;
case "AuthorizationRules":
qd.AuthorizationRules = AuthorizationRules.ParseFromXElement(element);
case "EnableExpress":
qd.EnableExpress = bool.Parse(element.Value);
break;
case "AccessedAt":
case "CreatedAt":
Expand All @@ -152,7 +165,7 @@ private static QueueDescription ParseFromEntryElement(XElement xEntry)
// For unknown properties, keep them as-is for forward proof.
if (qd.UnknownProperties == null)
{
qd.UnknownProperties = new List<object>();
qd.UnknownProperties = new List<XElement>();
}

qd.UnknownProperties.Add(element);
Expand Down
29 changes: 15 additions & 14 deletions ...servicebus/Microsoft.Azure.ServiceBus/src/Management/SubscriptionDescriptionExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,54 +105,55 @@ private static SubscriptionDescription ParseFromEntryElement(string topicName, X
{
switch (element.Name.LocalName)
{
case "LockDuration":
subscriptionDesc.LockDuration = XmlConvert.ToTimeSpan(element.Value);
break;
case "RequiresSession":
subscriptionDesc.RequiresSession = bool.Parse(element.Value);
break;
case "DefaultMessageTimeToLive":
subscriptionDesc.DefaultMessageTimeToLive = XmlConvert.ToTimeSpan(element.Value);
break;
case "DeadLetteringOnMessageExpiration":
subscriptionDesc.EnableDeadLetteringOnMessageExpiration = bool.Parse(element.Value);
break;
case "DeadLetteringOnFilterEvaluationExceptions":
subscriptionDesc.EnableDeadLetteringOnFilterEvaluationExceptions = bool.Parse(element.Value);
break;
case "LockDuration":
subscriptionDesc.LockDuration = XmlConvert.ToTimeSpan(element.Value);
break;
case "DefaultMessageTimeToLive":
subscriptionDesc.DefaultMessageTimeToLive = XmlConvert.ToTimeSpan(element.Value);
break;
case "MaxDeliveryCount":
subscriptionDesc.MaxDeliveryCount = int.Parse(element.Value);
break;
case "Status":
subscriptionDesc.Status = (EntityStatus)Enum.Parse(typeof(EntityStatus), element.Value);
break;
case "EnableBatchedOperations":
subscriptionDesc.EnableBatchedOperations = bool.Parse(element.Value);
break;
case "UserMetadata":
subscriptionDesc.UserMetadata = element.Value;
break;
case "AutoDeleteOnIdle":
subscriptionDesc.AutoDeleteOnIdle = XmlConvert.ToTimeSpan(element.Value);
case "Status":
subscriptionDesc.Status = (EntityStatus)Enum.Parse(typeof(EntityStatus), element.Value);
break;
case "ForwardTo":
if (!string.IsNullOrWhiteSpace(element.Value))
{
subscriptionDesc.ForwardTo = element.Value;
}
break;
case "UserMetadata":
subscriptionDesc.UserMetadata = element.Value;
break;
case "ForwardDeadLetteredMessagesTo":
if (!string.IsNullOrWhiteSpace(element.Value))
{
subscriptionDesc.ForwardDeadLetteredMessagesTo = element.Value;
}
break;
case "AutoDeleteOnIdle":
subscriptionDesc.AutoDeleteOnIdle = XmlConvert.ToTimeSpan(element.Value);
break;
case "AccessedAt":
case "CreatedAt":
case "MessageCount":
case "SizeInBytes":
case "UpdatedAt":
case "CountDetails":
case "DefaultRuleDescription":
// Ignore known properties
// Do nothing
break;
Expand Down
15 changes: 15 additions & 0 deletions sdk/servicebus/Microsoft.Azure.ServiceBus/src/Management/TopicDescription.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,16 @@ public string UserMetadata
}
}

internal bool IsAnonymousAccessible { get; set; } = false;

internal bool FilteringMessagesBeforePublishing { get; set; } = false;

internal string ForwardTo { get; set; } = null;

internal bool EnableExpress { get; set; } = false;

internal bool EnableSubscriptionPartitioning { get; set; } = false;

/// <summary>
/// List of properties that were retrieved using GetTopic but are not understood by this version of client is stored here.
/// The list will be sent back when an already retrieved TopicDescription will be used in UpdateTopic call.
Expand Down Expand Up @@ -202,6 +212,11 @@ public bool Equals(TopicDescription otherDescription)
&& this.RequiresDuplicateDetection.Equals(other.RequiresDuplicateDetection)
&& this.Status.Equals(other.Status)
&& string.Equals(this.userMetadata, other.userMetadata, StringComparison.OrdinalIgnoreCase)
&& string.Equals(this.ForwardTo, other.ForwardTo, StringComparison.OrdinalIgnoreCase)
&& this.EnableExpress == other.EnableExpress
&& this.IsAnonymousAccessible == other.IsAnonymousAccessible
&& this.FilteringMessagesBeforePublishing == other.FilteringMessagesBeforePublishing
&& this.EnableSubscriptionPartitioning == other.EnableSubscriptionPartitioning
&& (this.AuthorizationRules != null && other.AuthorizationRules != null
|| this.AuthorizationRules == null && other.AuthorizationRules == null)
&& (this.AuthorizationRules == null || this.AuthorizationRules.Equals(other.AuthorizationRules)))
Expand Down
Loading

0 comments on commit 5db2963

Please sign in to comment.