From dad244455b7c2ce46d5ed29fa0c7755ac47836e8 Mon Sep 17 00:00:00 2001 From: m-nash Date: Tue, 11 May 2021 14:54:45 -0700 Subject: [PATCH 1/3] updates to subresource type clean up extensions meant for Operation now --- ...perationExtensions.cs => OperationExtensions.cs} | 2 +- ...mResponseExtensions.cs => ResponseExtensions.cs} | 2 +- .../src/Resources/SubResource.cs | 13 ++++++------- ...tion.cs => SubResourceWritable.Serialization.cs} | 6 +++--- ...ubResourceReadOnly.cs => SubResourceWritable.cs} | 13 ++++++++++--- 5 files changed, 21 insertions(+), 15 deletions(-) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/{ArmOperationExtensions.cs => OperationExtensions.cs} (98%) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/{ArmResponseExtensions.cs => ResponseExtensions.cs} (93%) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/{SubResourceReadOnly.Serialization.cs => SubResourceWritable.Serialization.cs} (88%) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/{SubResourceReadOnly.cs => SubResourceWritable.cs} (61%) diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmOperationExtensions.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/OperationExtensions.cs similarity index 98% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmOperationExtensions.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/OperationExtensions.cs index fa570ea0710c..24af0e2bb87d 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmOperationExtensions.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/OperationExtensions.cs @@ -11,7 +11,7 @@ namespace Azure.ResourceManager.Core /// /// Extension methods for Operation class that apply to management use cases. /// - public static class ArmOperationExtensions + public static class OperationExtensions { /// /// Waits for the completion of the long running operations. diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmResponseExtensions.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ResponseExtensions.cs similarity index 93% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmResponseExtensions.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ResponseExtensions.cs index 7bcb9109fb0a..1de63e0891a5 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ArmResponseExtensions.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Extensions/ResponseExtensions.cs @@ -6,7 +6,7 @@ namespace Azure.ResourceManager.Core /// /// Extension methods for Response class that apply to management use cases. /// - public static class ArmResponseExtensions + public static class ResponseExtensions { /// /// Gets the correlation id from x-ms-correlation-id. diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs index cc82aa4c662c..d3161537833f 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs @@ -10,15 +10,17 @@ namespace Azure.ResourceManager.Core public partial class SubResource { /// - /// Initializes an empty instance of . + /// Initializes an empty instance of for mocking. /// [InitializationConstructor] - protected SubResource() { } + protected SubResource() + { + } /// Initializes a new instance of SubResource. /// ARM resource Id. [SerializationConstructor] - protected internal SubResource(string id) + internal SubResource(string id) { Id = id; } @@ -27,9 +29,6 @@ protected internal SubResource(string id) /// ARM resource identifier. /// /// - public virtual ResourceIdentifier Id { get; set; } + public virtual ResourceIdentifier Id { get; } } } - -// Todo: we want to make the default one (SubResource) to represent read-only data, -// that is the pattern we used in Resource and TrackedResource diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.Serialization.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.Serialization.cs similarity index 88% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.Serialization.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.Serialization.cs index 73da313642c8..f7f088dcf84d 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.Serialization.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.Serialization.cs @@ -10,7 +10,7 @@ namespace Azure.ResourceManager.Core /// /// A class representing a sub-resource that contains only the ID. /// - public partial class SubResourceReadOnly : IUtf8JsonSerializable + public partial class SubResourceWritable : IUtf8JsonSerializable { /// /// Serialize the input SubResourceReadOnly object. @@ -32,7 +32,7 @@ void IUtf8JsonSerializable.Write(Utf8JsonWriter writer) /// /// The JSON element to be deserialized. /// Deserialized SubResourceReadOnly object. - internal static SubResourceReadOnly DeserializeSubResourceReadOnly(JsonElement element) + internal static SubResourceWritable DeserializeSubResourceReadOnly(JsonElement element) { Optional id = default; foreach (var property in element.EnumerateObject()) @@ -43,7 +43,7 @@ internal static SubResourceReadOnly DeserializeSubResourceReadOnly(JsonElement e continue; } } - return new SubResourceReadOnly(id.Value); + return new SubResourceWritable(id.Value); } } } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs similarity index 61% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs index d5df3191eea7..ef810d7120f0 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceReadOnly.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs @@ -7,11 +7,18 @@ namespace Azure.ResourceManager.Core /// A class representing a sub-resource that contains only the read-only ID. /// [ReferenceType] - public partial class SubResourceReadOnly + public partial class SubResourceWritable { + /// Initializes a new instance of SubResourceReadOnly. + [InitializationConstructor] + public SubResourceWritable() + { + } + /// Initializes a new instance of SubResourceReadOnly. /// ARM resource Id. - protected internal SubResourceReadOnly(string id) + [SerializationConstructor] + internal SubResourceWritable(string id) { Id = id; } @@ -20,6 +27,6 @@ protected internal SubResourceReadOnly(string id) /// ARM resource identifier (read-only). /// /// - public virtual ResourceIdentifier Id { get; protected set; } + public virtual ResourceIdentifier Id { get; set; } } } From 5ac6f01be06d681c6c2d4731e82c99df9caefc11 Mon Sep 17 00:00:00 2001 From: m-nash Date: Wed, 12 May 2021 11:00:30 -0700 Subject: [PATCH 2/3] need protected internal serialization constructor for reference types, added unit tests to validate for future --- ... => InitializationConstructorAttribute.cs} | 4 +- .../src/Resources/Resource.cs | 2 +- ...s => SerializationConstructorAttribute.cs} | 4 +- .../src/Resources/SubResource.cs | 4 +- .../src/Resources/SubResourceWritable.cs | 4 +- .../src/Resources/TrackedResource.cs | 2 +- .../tests/Unit/ReferenceTypeTests.cs | 47 +++++++++++++++++++ 7 files changed, 57 insertions(+), 10 deletions(-) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/{InitializationConstructor.cs => InitializationConstructorAttribute.cs} (80%) rename sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/{SerializationConstructor.cs => SerializationConstructorAttribute.cs} (80%) create mode 100644 sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructor.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructorAttribute.cs similarity index 80% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructor.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructorAttribute.cs index 44ddb331ff80..b5f3b625ab00 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructor.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/InitializationConstructorAttribute.cs @@ -9,12 +9,12 @@ namespace Azure.ResourceManager.Core /// An attribute class indicating the constructor to use for initialization. /// [AttributeUsage(AttributeTargets.Constructor)] - public class InitializationConstructor : Attribute + public class InitializationConstructorAttribute : Attribute { /// /// Instatiate a new InitializationConstructor attribute. /// - public InitializationConstructor() + public InitializationConstructorAttribute() { } } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/Resource.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/Resource.cs index 29ebb2d3740d..befebf7f9a56 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/Resource.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/Resource.cs @@ -25,7 +25,7 @@ protected Resource() { } /// The name of the resource. /// The of the resource. [SerializationConstructor] - protected Resource(TIdentifier id, string name, ResourceType type) + protected internal Resource(TIdentifier id, string name, ResourceType type) { Id = id; Name = name; diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructor.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructorAttribute.cs similarity index 80% rename from sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructor.cs rename to sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructorAttribute.cs index e124b806da6c..4d62b8a047ad 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructor.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SerializationConstructorAttribute.cs @@ -9,12 +9,12 @@ namespace Azure.ResourceManager.Core /// An attribute class indicating the constructor to use for serialization. /// [AttributeUsage(AttributeTargets.Constructor)] - public class SerializationConstructor : Attribute + public class SerializationConstructorAttribute : Attribute { /// /// Instatiate a new SerializationConstructor attribute. /// - public SerializationConstructor() + public SerializationConstructorAttribute() { } } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs index d3161537833f..6b7d2343a872 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs @@ -20,13 +20,13 @@ protected SubResource() /// Initializes a new instance of SubResource. /// ARM resource Id. [SerializationConstructor] - internal SubResource(string id) + protected internal SubResource(string id) { Id = id; } /// - /// ARM resource identifier. + /// Gets the ARM resource identifier. /// /// public virtual ResourceIdentifier Id { get; } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs index ef810d7120f0..4061074adb48 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResourceWritable.cs @@ -18,13 +18,13 @@ public SubResourceWritable() /// Initializes a new instance of SubResourceReadOnly. /// ARM resource Id. [SerializationConstructor] - internal SubResourceWritable(string id) + protected internal SubResourceWritable(string id) { Id = id; } /// - /// ARM resource identifier (read-only). + /// Gets or sets the ARM resource identifier. /// /// public virtual ResourceIdentifier Id { get; set; } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/TrackedResource.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/TrackedResource.cs index b17703ffad8c..24fe9a1c8149 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/TrackedResource.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/TrackedResource.cs @@ -40,7 +40,7 @@ protected TrackedResource(LocationData location) /// The tags for the resource. /// The location of the resource. [SerializationConstructor] - protected TrackedResource(TIdentifier id, string name, ResourceType type, LocationData location, IDictionary tags) + protected internal TrackedResource(TIdentifier id, string name, ResourceType type, LocationData location, IDictionary tags) : base(id, name, type) { Tags = tags ?? new Dictionary(StringComparer.InvariantCultureIgnoreCase); diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs new file mode 100644 index 000000000000..6310667695fa --- /dev/null +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs @@ -0,0 +1,47 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using NUnit.Framework; + +namespace Azure.ResourceManager.Core.Tests +{ + public class ReferenceTypeTests + { + private static readonly Type ReferenceAttribute = typeof(ReferenceTypeAttribute); + private static readonly Type SerializationConstructor = typeof(SerializationConstructorAttribute); + private static readonly Type InitializationConstructor = typeof(InitializationConstructorAttribute); + private static readonly IEnumerable AssemblyTypes = typeof(ArmClient).Assembly.GetTypes(); + + [Test] + public void ValidateSerializationConstructor() + { + foreach (var refType in AssemblyTypes.Where(t => HasAttribute(t.GetCustomAttributes(false), ReferenceAttribute))) + { + var serializationCtor = refType.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) + .Where(c => HasAttribute(c.GetCustomAttributes(false), SerializationConstructor)).FirstOrDefault(); + Assert.IsNotNull(serializationCtor); + Assert.IsTrue(serializationCtor.IsFamilyOrAssembly); + Assert.IsFalse(serializationCtor.IsPublic); + } + } + + [Test] + public void ValidateInitializationConstructor() + { + foreach (var refType in AssemblyTypes.Where(t => HasAttribute(t.GetCustomAttributes(false), ReferenceAttribute))) + { + var initializationCtor = refType.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) + .Where(c => HasAttribute(c.GetCustomAttributes(false), InitializationConstructor)).FirstOrDefault(); + Assert.IsNotNull(initializationCtor); + Assert.IsTrue(initializationCtor.IsFamily || initializationCtor.IsPublic); + Assert.IsFalse(initializationCtor.IsAssembly); + } + } + + public bool HasAttribute(IEnumerable list, Type attributeType) + { + return list.FirstOrDefault(a => a.GetType() == attributeType) is not null; + } + } +} From 4dbd0f462766bec71626ea75d176c42cae509085 Mon Sep 17 00:00:00 2001 From: m-nash Date: Wed, 12 May 2021 11:07:57 -0700 Subject: [PATCH 3/3] updates after merge --- .../src/Resources/SubResource.cs | 4 ++-- .../tests/Unit/ReferenceTypeTests.cs | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs index 7bb7944f2a1d..49a2b46b604b 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/src/Resources/SubResource.cs @@ -13,14 +13,14 @@ public partial class SubResource /// Initializes an empty instance of for mocking. /// [InitializationConstructor] - protected SubResource() + public SubResource() { } /// Initializes a new instance of SubResource. /// ARM resource Id. [SerializationConstructor] - internal SubResource(string id) + protected internal SubResource(string id) { Id = id; } diff --git a/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs b/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs index 6310667695fa..7f35a405e80a 100644 --- a/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs +++ b/sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ReferenceTypeTests.cs @@ -21,8 +21,8 @@ public void ValidateSerializationConstructor() var serializationCtor = refType.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) .Where(c => HasAttribute(c.GetCustomAttributes(false), SerializationConstructor)).FirstOrDefault(); Assert.IsNotNull(serializationCtor); - Assert.IsTrue(serializationCtor.IsFamilyOrAssembly); - Assert.IsFalse(serializationCtor.IsPublic); + Assert.IsTrue(serializationCtor.IsFamilyOrAssembly, $"Serialization ctor for {refType.Name} should be protected internal"); + Assert.IsFalse(serializationCtor.IsPublic, $"Serialization ctor for {refType.Name} should not be public"); } } @@ -34,8 +34,9 @@ public void ValidateInitializationConstructor() var initializationCtor = refType.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) .Where(c => HasAttribute(c.GetCustomAttributes(false), InitializationConstructor)).FirstOrDefault(); Assert.IsNotNull(initializationCtor); - Assert.IsTrue(initializationCtor.IsFamily || initializationCtor.IsPublic); - Assert.IsFalse(initializationCtor.IsAssembly); + Assert.IsTrue(refType.IsAbstract == initializationCtor.IsFamily, $"If {refType.Name} is abstract then its initialization ctor should be protected"); + Assert.IsTrue(refType.IsAbstract != initializationCtor.IsPublic, $"If {refType.Name} is abstract then its initialization ctor should be public"); + Assert.IsFalse(initializationCtor.IsAssembly, $"Initialization ctor for {refType.Name} should not be internal"); } }