Never let one property getter lose a whole OptionsDescription (#590) - #591
Open
jeremydmiller wants to merge 1 commit into
Open
Never let one property getter lose a whole OptionsDescription (#590)#591jeremydmiller wants to merge 1 commit into
jeremydmiller wants to merge 1 commit into
Conversation
) An OptionsDescription is a diagnostic view built by calling arbitrary property getters on somebody else's configuration object, so any one of them can throw -- and losing the entire description over one bad property is a terrible trade. Reported from the field as JasperFx/wolverine#3740, where AzureServiceBusTransport.HostName threw a NullReferenceException for credential-based connections and a monitored service could consequently never build its ServiceCapabilities snapshot at all. - Skip set-only properties and indexers, neither of which can be read via PropertyInfo.GetValue(subject) (ArgumentException / TargetParameterCountException). Wolverine.Pulsar's PulsarTransport has a this[Uri] indexer, which was enough to make it undescribable. - Catch whatever a getter throws -- including from the [ChildDescription], [DescribeAsStringArray] and [DescribeAsConfigurationState] branches -- and record OptionsValue.Unreadable(...) in its place. - Report the exception TYPE only, never the message: descriptions get shipped to monitoring consoles and work hard to keep secrets out, and exception messages habitually quote the offending configuration value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RainvDHdde5JUAiGEPEgWw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #590.
OptionsDescription(object subject)builds a diagnostic view by calling arbitrary property getters on somebody else's configuration object. Any one of those getters can throw, and until now that lost the entire description — along with everything built on top of it.Reported from the field as JasperFx/wolverine#3740:
AzureServiceBusTransport.HostNamethrew aNullReferenceExceptionfor credential-based (managed identity) connections, so a monitored Wolverine service could never build itsServiceCapabilitiessnapshot, so CritterWatch never completed a handshake for it — permanently, since the same doomed read is retried on every batch.What changed
ArgumentException) and indexers (GetValuewith no index arguments →TargetParameterCountException). Both are legal on a described type and neither is configuration data.Wolverine.Pulsar'sPulsarTransporthas athis[Uri]indexer, which alone was enough to make it undescribable.[ChildDescription],[DescribeAsStringArray]and[DescribeAsConfigurationState]branches — and recordOptionsValue.Unreadable(...)in its place, withValue="Could not be read -- <ExceptionTypeName>".The per-property body moved into a
readProperty(...)helper so the try/catch wraps every branch, with no behavior change for properties that read cleanly.Tests
CoreTests/Descriptors/OptionsDescriptionTests.csgains adescribing_awkward_propertiesfixture: a throwing getter, a throwing[ChildDescription], an indexer, a set-only property, secret-in-the-exception-message, and still-serializable. FullCoreTests(485) andCommandLineTests(295) green locally.🤖 Generated with Claude Code
https://claude.ai/code/session_01RainvDHdde5JUAiGEPEgWw