Fix Todo comment (derived classes in registration) (bagetter#64)

* - fix TODO comment by using polymorphism - adjust tests * - adjust test to include `JsonPropertyName` - cleanup * - fix TODO comment by using polymorphism - adjust tests * - adjust test to include `JsonPropertyName` - cleanup * - replace tuple - fix formatting
teknologi-umum · Feb 8, 2024 · 23f136a · 23f136a
1 parent 69b8fdb
commit 23f136a
Show file tree

Hide file tree

Showing 6 changed files with 122 additions and 158 deletions.
diff --git a/src/BaGetter.Core/Metadata/BaGetRegistrationIndexPageItem.cs b/src/BaGetter.Core/Metadata/BaGetRegistrationIndexPageItem.cs
@@ -5,29 +5,16 @@ namespace BaGetter.Core
 {
     /// <summary>
     /// BaGetter's extensions to a registration index page.
-    /// Extends <see cref="RegistrationIndexPageItem"/>.
     /// </summary>
-    /// <remarks>
-    /// TODO: After this project is updated to .NET 5, make <see cref="BaGetRegistrationIndexPageItem"/>
-    /// extend <see cref="RegistrationIndexPageItem"/> and remove identical properties.
-    /// Properties that are modified should be marked with the "new" modified.
-    /// See: https://github.com/dotnet/runtime/pull/32107
-    /// </remarks>
-    public class BaGetRegistrationIndexPageItem
+    /// <remarks>Extends <see cref="RegistrationIndexPageItem"/>.</remarks>
+    public class BaGetRegistrationIndexPageItem : RegistrationIndexPageItem
     {
-#region Original properties from RegistrationIndexPageItem.
-        [JsonPropertyName("@id")]
-        public string RegistrationLeafUrl { get; set; }
-
-        [JsonPropertyName("packageContent")]
-        public string PackageContentUrl { get; set; }
-#endregion
-
         /// <summary>
         /// The catalog entry containing the package metadata.
-        /// This was modified to use BaGetter's extended package metadata model.
         /// </summary>
+        /// <remarks>This was modified to use BaGetter's extended package metadata model.</remarks>
         [JsonPropertyName("catalogEntry")]
-        public BaGetterPackageMetadata PackageMetadata { get; set; }
+        [JsonPropertyOrder(int.MaxValue)]
+        public new BaGetterPackageMetadata PackageMetadata { get; set; }
     }
 }
diff --git a/src/BaGetter.Core/Metadata/BaGetterPackageMetadata.cs b/src/BaGetter.Core/Metadata/BaGetterPackageMetadata.cs
@@ -5,9 +5,11 @@
 namespace BaGetter.Core
 {
     /// <summary>
-    /// BaGetter's extensions to the package metadata model. These additions
-    /// are not part of the official protocol.
+    /// BaGetter's extensions to the package metadata model.
     /// </summary>
+    /// <remarks><para>Extends <see cref="PackageMetadata"/>.</para>
+    /// These additions are not part of the official protocol.
+    /// </remarks>
     public class BaGetterPackageMetadata : PackageMetadata
     {
         [JsonPropertyName("downloads")]

diff --git a/src/BaGetter.Core/Metadata/BaGetterRegistrationIndexPage.cs b/src/BaGetter.Core/Metadata/BaGetterRegistrationIndexPage.cs
@@ -6,34 +6,17 @@ namespace BaGetter.Core
 {
     /// <summary>
     /// BaGetter's extensions to a registration index page.
-    /// Extends <see cref="RegistrationIndexPage"/>.
     /// </summary>
-    /// <remarks>
-    /// TODO: After this project is updated to .NET 5, make <see cref="BaGetterRegistrationIndexPage"/>
-    /// extend <see cref="RegistrationIndexPage"/> and remove identical properties.
-    /// Properties that are modified should be marked with the "new" modified.
-    /// See: https://github.com/dotnet/runtime/pull/32107
-    /// </remarks>
-    public class BaGetterRegistrationIndexPage
+    /// <remarks>Extends <see cref="RegistrationIndexPage"/>.</remarks>
+    public class BaGetterRegistrationIndexPage : RegistrationIndexPage
     {
-#region Original properties from RegistrationIndexPage.
-        [JsonPropertyName("@id")]
-        public string RegistrationPageUrl { get; set; }
-
-        [JsonPropertyName("count")]
-        public int Count { get; set; }
-
-        [JsonPropertyName("lower")]
-        public string Lower { get; set; }
-
-        [JsonPropertyName("upper")]
-        public string Upper { get; set; }
-#endregion
-
         /// <summary>
-        /// This was modified to use BaGetter's extended registration index page item model.
+        /// <see langword="null"/> if this package's registration is paged. The items can be found
+        /// by following the page's <see cref="RegistrationIndexPage.RegistrationPageUrl"/>.
         /// </summary>
+        /// <remarks>This was modified to use BaGetter's extended registration index page item model.</remarks>
         [JsonPropertyName("items")]
-        public IReadOnlyList<BaGetRegistrationIndexPageItem> ItemsOrNull { get; set; }
+        [JsonPropertyOrder(int.MaxValue)]
+        public new IReadOnlyList<BaGetRegistrationIndexPageItem> ItemsOrNull { get; set; }
     }
 }
diff --git a/src/BaGetter.Core/Metadata/BaGetterRegistrationIndexResponse.cs b/src/BaGetter.Core/Metadata/BaGetterRegistrationIndexResponse.cs
@@ -6,40 +6,24 @@ namespace BaGetter.Core
 {
     /// <summary>
     /// BaGetter's extensions to a registration index response.
-    /// Extends <see cref="RegistrationIndexResponse"/>.
     /// </summary>
-    /// <remarks>
-    /// TODO: After this project is updated to .NET 5, make <see cref="BaGetterRegistrationIndexResponse"/>
-    /// extend <see cref="RegistrationIndexResponse"/> and remove identical properties.
-    /// Properties that are modified should be marked with the "new" modified.
-    /// See: https://github.com/dotnet/runtime/pull/32107
-    /// </remarks>
-    public class BaGetterRegistrationIndexResponse
+    /// <remarks>Extends <see cref="RegistrationIndexResponse"/>.</remarks>
+    public class BaGetterRegistrationIndexResponse : RegistrationIndexResponse
     {
-#region Original properties from RegistrationIndexResponse.
-        [JsonPropertyName("@id")]
-        public string RegistrationIndexUrl { get; set; }
-
-        [JsonPropertyName("@type")]
-        public IReadOnlyList<string> Type { get; set; }
-
-        [JsonPropertyName("count")]
-        public int Count { get; set; }
-#endregion
-
         /// <summary>
-        /// The pages that contain all of the versions of the package, ordered
-        /// by the package's version. This was modified to use BaGetter's extended
-        /// registration index page model.
+        /// The pages that contain all of the versions of the package, ordered by the package's version.
         /// </summary>
+        /// <remarks>This was modified to use BaGetter's extended registration index page model.</remarks>
         [JsonPropertyName("items")]
-        public IReadOnlyList<BaGetterRegistrationIndexPage> Pages { get; set; }
+        [JsonPropertyOrder(int.MaxValue)]
+        public new IReadOnlyList<BaGetterRegistrationIndexPage> Pages { get; set; }
 
         /// <summary>
         /// The package's total downloads across all versions.
-        /// This is not part of the official NuGet protocol.
         /// </summary>
+        /// <remarks>This is not part of the official NuGet protocol.</remarks>
         [JsonPropertyName("totalDownloads")]
+        [JsonPropertyOrder(int.MaxValue)]
         public long TotalDownloads { get; set; }
     }
 }
diff --git a/tests/BaGetter.Core.Tests/Metadata/ModelTests.cs b/tests/BaGetter.Core.Tests/Metadata/ModelTests.cs
@@ -100,25 +100,33 @@ public static IEnumerable<object[]> ExtendedModelsData()
             };
         }
 
+        /// <summary>
+        /// Check that the derived type is actually derived from the original type.
+        /// </summary>
+        /// <param name="data">The test data.</param>
+        [Theory]
+        [MemberData(nameof(ExtendedModelsData))]
+        public void ExtendedModelsAreActuallyDerivedFromOriginalModels(ExtendedModelData data)
+        {
+            Assert.True(data.DerivedType.IsSubclassOf(data.OriginalType));
+        }
+
         [Theory]
         [MemberData(nameof(ExtendedModelsData))]
         public void ValidateExtendedModels(ExtendedModelData data)
         {
+            // Arrange
             IReadOnlyDictionary<string, PropertyInfo> originalProperties = data
                 .OriginalType
                 .GetProperties()
                 .ToDictionary(p => p.Name, p => p);
+
             IReadOnlyDictionary<string, PropertyInfo> derivedProperties = data
                 .DerivedType
-                .GetProperties()
+                .GetProperties(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
                 .ToDictionary(p => p.Name, p => p);
 
-            // Check that all properties on the original model are present on the derived model.
-            var missingProperties = originalProperties.Keys.Where(name => !derivedProperties.ContainsKey(name));
-
-            Assert.True(
-                !missingProperties.Any(),
-                $"The following properties are missing from the derived type: {string.Join(',', missingProperties)}");
+            // Act/Assert
 
             // Check that all properties on the derived model are as expected compared to the original model.
             foreach (var derivedProperty in derivedProperties.Values)
@@ -137,9 +145,9 @@ public void ValidateExtendedModels(ExtendedModelData data)
                     continue;
                 }
 
-                // This property should exist on both the original and derived models.
-                // This property should have the same "JsonPropertyName" attribute values.
-                var originalProperty = Assert.Contains(derivedProperty.Name, originalProperties);
+                var originalProperty = originalProperties
+                        .SingleOrDefault(x => string.Equals(x.Key, derivedProperty.Name, StringComparison.OrdinalIgnoreCase))
+                        .Value;
 
                 var originalJsonName = GetAttributeArgs<JsonPropertyNameAttribute>(originalProperty)?.FirstOrDefault();
                 var derivedJsonName = GetAttributeArgs<JsonPropertyNameAttribute>(derivedProperty)?.FirstOrDefault();
@@ -159,9 +167,6 @@ public void ValidateExtendedModels(ExtendedModelData data)
                     $"on type '{data.OriginalType}'.\nExpected: '{originalJsonName}'\n" +
                     $"Actual: '{derivedJsonName}'");
 
-                var originalJsonConverterArgs = GetAttributeArgs<JsonConverterAttribute>(originalProperty);
-                var derivedJsonConverterArgs = GetAttributeArgs<JsonConverterAttribute>(derivedProperty);
-
                 // If the property was modified, check that the property types are expected.
                 if (data.ModifiedProperties.TryGetValue(derivedProperty.Name, out var modifiedTypes))
                 {
@@ -176,31 +181,20 @@ public void ValidateExtendedModels(ExtendedModelData data)
                         $"Expected: {modifiedTypes.To}\n" +
                         $"Actual: {derivedProperty.PropertyType}");
 
+                    var originalJsonConverterArgs = GetAttributeArgs<JsonConverterAttribute>(originalProperty);
+                    var derivedJsonConverterArgs = GetAttributeArgs<JsonConverterAttribute>(derivedProperty);
+
                     if (originalJsonConverterArgs != null || derivedJsonConverterArgs != null)
                     {
-                        throw new NotSupportedException(
-                            "JSON converters on modified properties is not supported");
+                        throw new NotSupportedException("JSON converters on modified properties is not supported");
                     }
 
                     continue;
                 }
-
-                // Otherwise, this property should be identical to the original property.
-                Assert.True(
-                    originalProperty.PropertyType == derivedProperty.PropertyType,
-                    $"Property '{derivedProperty.Name}' on type {data.DerivedType} has unexpected property type\n" +
-                    $"Expected: {originalProperty.PropertyType}\n" +
-                    $"Actual: {derivedProperty.PropertyType}");
-                Assert.True(
-                    originalJsonConverterArgs?.First() == derivedJsonConverterArgs?.First(),
-                    $"Property '{derivedProperty.Name}' on type '{data.DerivedType}' " +
-                    "has unexpected JsonConverter value.\n" +
-                    $"Expected: '{originalJsonConverterArgs?.First()}'\n" +
-                    $"Actual: '{derivedJsonConverterArgs?.First()}'");
             }
         }
 
-        private IList<CustomAttributeTypedArgument> GetAttributeArgs<TAttribute>(PropertyInfo property)
+        private static IList<CustomAttributeTypedArgument> GetAttributeArgs<TAttribute>(PropertyInfo property)
         {
             return property
                 .CustomAttributes
@@ -224,14 +218,13 @@ public class ExtendedModelData
             /// <summary>
             /// The properties added by the model type in the "BaGetter.Core" project.
             /// </summary>
-            public Dictionary<string, Type> AddedProperties { get; set; } = new Dictionary<string, Type>();
+            public Dictionary<string, Type> AddedProperties { get; set; } = [];
 
             /// <summary>
             /// The properties whose types were modified by the model type in the
             /// "BaGetter.Core" project.
             /// </summary>
-            public Dictionary<string, (Type From, Type To)> ModifiedProperties { get; set; }
-                = new Dictionary<string, (Type From, Type To)>();
+            public Dictionary<string, (Type From, Type To)> ModifiedProperties { get; set; } = [];
         }
     }
 }