Skip to content

Conversation

DamianEdwards
Copy link
Owner

It's impossible to know if a given property getter will throw during validation due to the state of the object, so just swallow any exception from getting the value.

Fixes #54

It's impossible to know if a given property getter will throw during validation due to the state of the object, so just swallow any exception from getting the value.

Fixes #54
Comment on lines +384 to 389
try
{
propertyValue = property.GetValue(target);
}
catch (Exception) { }
var propertyValueType = propertyValue?.GetType();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this fail validation?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so? Certainly the bug report indicates they want the validation to continue, the issue is that for anything that has a getter that throws when in certain state, the walk will cause it to throw. I still feel like I'm missing something here. I might have to try out this scenario in MVC and step through what it does.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be an opt in. I think blindly swallowing exceptions is bad. Maybe instead have a way to skip a property? (attribute?)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already have a way to opt-out of recursing at the property level: https://github.com/DamianEdwards/MiniValidation/blob/main/src/MiniValidation/SkipRecursionAttribute.cs

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding an ValidationResult stating that a certain value could not be retrieved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Safer recursive validation.

3 participants