Never let one property getter lose a whole OptionsDescription (#590)

src/CoreTests/Descriptors/OptionsDescriptionTests.cs

@@ -61,6 +61,87 @@ public void children_and_sets_round_trip_through_default_json_options()
     }
 }
 
+public class describing_awkward_properties
+{
+    // Regression for the report in JasperFx/wolverine#3740: AzureServiceBusTransport.HostName threw a
+    // NullReferenceException for credential-based connections, and because an OptionsDescription reads every
+    // public property reflectively, that single getter took out the whole Wolverine ServiceCapabilities
+    // snapshot -- so the monitoring console got nothing at all for the service, permanently.
+    [Fact]
+    public void a_throwing_getter_does_not_lose_the_whole_description()
+    {
+        var description = new OptionsDescription(new AwkwardObject());
+
+        description.PropertyFor(nameof(AwkwardObject.Name))!.Value.ShouldBe("Rogue");
+        description.PropertyFor(nameof(AwkwardObject.Tolerable))!.Value.ShouldBe("42");
+
+        var explosive = description.PropertyFor(nameof(AwkwardObject.Explosive));
+        explosive.ShouldNotBeNull();
+        explosive.Value.ShouldStartWith(OptionsValue.UnreadablePrefix);
+        explosive.Value.ShouldContain(nameof(NullReferenceException));
+        explosive.RawValue.ShouldBeNull();
+    }
+
+    [Fact]
+    public void the_exception_message_is_never_included()
+    {
+        // Descriptions are shipped to monitoring tools and go to some lengths to keep secrets out; exception
+        // messages love to quote the offending configuration value
+        var description = new OptionsDescription(new AwkwardObject());
+        description.PropertyFor(nameof(AwkwardObject.Explosive))!.Value
+            .ShouldNotContain(AwkwardObject.SecretishMessage);
+    }
+
+    [Fact]
+    public void skips_indexers_and_set_only_properties()
+    {
+        var description = new OptionsDescription(new AwkwardObject());
+
+        // "Item" is what an indexer is called in reflection -- reading one with no arguments throws
+        description.Properties.ShouldNotContain(x => x.Name == "Item");
+        description.Properties.ShouldNotContain(x => x.Name == nameof(AwkwardObject.WriteOnly));
+    }
+
+    [Fact]
+    public void a_throwing_child_description_does_not_lose_the_whole_description()
+    {
+        var description = new OptionsDescription(new AwkwardParent());
+
+        description.PropertyFor(nameof(AwkwardParent.Name))!.Value.ShouldBe("Storm");
+        description.Children.ShouldNotContainKey(nameof(AwkwardParent.Child));
+        description.PropertyFor(nameof(AwkwardParent.Child))!.Value
+            .ShouldStartWith(OptionsValue.UnreadablePrefix);
+    }
+
+    [Fact]
+    public void still_serializable_with_unreadable_properties()
+    {
+        new OptionsDescription(new AwkwardObject()).ShouldBeSerializable();
+    }
+}
+
+public class AwkwardObject
+{
+    public const string SecretishMessage = "Endpoint=sb://myns.servicebus.windows.net;SharedAccessKey=shhh";
+
+    public string Name { get; set; } = "Rogue";
+    public int Tolerable => 42;
+
+    public string Explosive => throw new NullReferenceException(SecretishMessage);
+
+    public string WriteOnly { set { } }
+
+    public string this[int index] => throw new NotSupportedException();
+}
+
+public class AwkwardParent
+{
+    public string Name { get; set; } = "Storm";
+
+    [ChildDescription]
+    public AwkwardObject Child => throw new InvalidOperationException("nope");
+}
+
 public class SomeObject : ITagged
 {
     public string[] Tags => ["blue", "green"];

src/JasperFx/Descriptors/OptionsDescription.cs

@@ -1,5 +1,6 @@
 using System.Collections;
 using System.Diagnostics.CodeAnalysis;
+using System.Reflection;
 using JasperFx.Core;
 using JasperFx.Core.Reflection;
 
@@ -107,54 +108,78 @@ private void readProperties(object subject)
 
         foreach (var property in type.GetProperties().Where(x => !x.HasAttribute<IgnoreDescriptionAttribute>()))
         {
-            if (property.HasAttribute<ChildDescriptionAttribute>())
+            // Neither of these can be read with PropertyInfo.GetValue(subject): a set-only property has no
+            // getter at all, and an indexer demands index arguments (TargetParameterCountException without
+            // them). Both are legal on a type that's being described, and neither is configuration data.
+            if (!property.CanRead || property.GetIndexParameters().Length > 0) continue;
+
+            // An OptionsDescription is a *diagnostic* view of somebody else's configuration object, built by
+            // calling arbitrary property getters. Any one of those getters can throw -- lazily connecting to
+            // a broker, asserting on state that isn't initialized yet, dereferencing a null connection string
+            // -- and losing the entire description (and everything built on top of it, e.g. Wolverine's
+            // ServiceCapabilities snapshot) over one bad property is a terrible trade. Report what couldn't be
+            // read, in place, and carry on.
+            try
             {
-                var child = property.GetValue(subject);
-                if (child == null) continue;
-
-                var childDescription = child is IDescribeMyself describes ? describes.ToDescription() : new OptionsDescription(child);
-                Children[property.Name] = childDescription;
-
-                continue;
+                readProperty(subject, type, property);
             }
-
-            if (property.HasAttribute<DescribeAsStringArrayAttribute>())
+            catch (Exception e)
             {
-                var value = property.GetValue(subject);
-                var items = value is IEnumerable enumerable
-                    ? enumerable.Cast<object?>().Select(x => x?.ToString() ?? string.Empty).ToArray()
-                    : Array.Empty<string>();
-
-                Properties.Add(new OptionsValue
-                {
-                    Subject = $"{type.FullNameInCode()}.{property.Name}",
-                    Name = property.Name,
-                    Type = PropertyType.StringArray,
-                    RawValue = items,
-                    Value = items.Join(", ")
-                });
-
-                continue;
+                Properties.Add(OptionsValue.Unreadable(property, subject, e));
             }
+        }
+    }
+
+    [RequiresUnreferencedCode("Reads subject.GetType().GetProperties() reflectively.")]
+    private void readProperty(object subject, Type type, PropertyInfo property)
+    {
+        if (property.HasAttribute<ChildDescriptionAttribute>())
+        {
+            var child = property.GetValue(subject);
+            if (child == null) return;
+
+            var childDescription = child is IDescribeMyself describes ? describes.ToDescription() : new OptionsDescription(child);
+            Children[property.Name] = childDescription;
 
-            if (property.HasAttribute<DescribeAsConfigurationStateAttribute>())
+            return;
+        }
+
+        if (property.HasAttribute<DescribeAsStringArrayAttribute>())
+        {
+            var value = property.GetValue(subject);
+            var items = value is IEnumerable enumerable
+                ? enumerable.Cast<object?>().Select(x => x?.ToString() ?? string.Empty).ToArray()
+                : Array.Empty<string>();
+
+            Properties.Add(new OptionsValue
             {
-                var state = property.GetValue(subject) != null ? "Configured" : "Default";
-                Properties.Add(new OptionsValue
-                {
-                    Subject = $"{type.FullNameInCode()}.{property.Name}",
-                    Name = property.Name,
-                    Type = PropertyType.Text,
-                    RawValue = state,
-                    Value = state
-                });
-
-                continue;
-            }
+                Subject = $"{type.FullNameInCode()}.{property.Name}",
+                Name = property.Name,
+                Type = PropertyType.StringArray,
+                RawValue = items,
+                Value = items.Join(", ")
+            });
+
+            return;
+        }
 
-            if (property.PropertyType != typeof(string) && property.PropertyType.IsEnumerable()) continue;
-            Properties.Add(OptionsValue.Read(property, subject));
+        if (property.HasAttribute<DescribeAsConfigurationStateAttribute>())
+        {
+            var state = property.GetValue(subject) != null ? "Configured" : "Default";
+            Properties.Add(new OptionsValue
+            {
+                Subject = $"{type.FullNameInCode()}.{property.Name}",
+                Name = property.Name,
+                Type = PropertyType.Text,
+                RawValue = state,
+                Value = state
+            });
+
+            return;
         }
+
+        if (property.PropertyType != typeof(string) && property.PropertyType.IsEnumerable()) return;
+        Properties.Add(OptionsValue.Read(property, subject));
     }
 
 

src/JasperFx/Descriptors/OptionsValue.cs

@@ -50,8 +50,20 @@ public static OptionsValue Read<T>(T subject, Expression<Func<T,object>> express
 
     public static OptionsValue Read(PropertyInfo property, object subject)
     {
-        var value = property.GetValue(subject);
-
+        object? value;
+        try
+        {
+            value = property.GetValue(subject);
+        }
+        catch (Exception e)
+        {
+            // Reading a property on somebody else's configuration object can fail in all kinds of ways --
+            // it might lazily open a connection, assert on uninitialized state, or dereference a null. This
+            // is a diagnostic view, so record *that* it couldn't be read and let the rest of the description
+            // through instead of throwing out of a getter walk.
+            return Unreadable(property, subject, e);
+        }
+
         var description = new OptionsValue
         {
             Subject = $"{subject.GetType().FullNameInCode()}.{property.Name}",
@@ -62,10 +74,40 @@ public static OptionsValue Read(PropertyInfo property, object subject)
         };
 
         description.WriteValue(value);
-        
+
         return description;
     }
 
+    /// <summary>
+    /// Prefix on <see cref="Value"/> marking a property whose getter threw when the description was built.
+    /// </summary>
+    public const string UnreadablePrefix = "Could not be read -- ";
+
+    /// <summary>
+    /// Represents a property that could not be read at all -- its getter threw. Used in place of the real
+    /// value so that one misbehaving property can't cost you an entire diagnostic description. Deliberately
+    /// reports only the exception *type*: descriptions are shipped off to monitoring tools, and exception
+    /// messages have a habit of quoting the very configuration values (connection strings, keys) that
+    /// descriptions work hard to keep out.
+    /// </summary>
+    public static OptionsValue Unreadable(PropertyInfo property, object subject, Exception exception)
+    {
+        // Reflection wraps anything thrown by the getter itself; the inner exception is the interesting one
+        var actual = exception is TargetInvocationException { InnerException: not null } wrapped
+            ? wrapped.InnerException
+            : exception;
+
+        return new OptionsValue
+        {
+            Subject = $"{subject.GetType().FullNameInCode()}.{property.Name}",
+            Name = property.Name,
+            Type = PropertyType.Text,
+            RawValue = null,
+            Value = UnreadablePrefix + actual.GetType().Name
+        };
+    }
+
+
     public void WriteValue(object? value)
     {
         if (value == null)