Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullReferenceException when property is null #2152

Closed
MeikelLP opened this issue Sep 4, 2023 · 13 comments
Closed

NullReferenceException when property is null #2152

MeikelLP opened this issue Sep 4, 2023 · 13 comments
Milestone

Comments

@MeikelLP
Copy link
Contributor

MeikelLP commented Sep 4, 2023

FluentValidation version

latest

ASP.NET version

No response

Summary

When you validate an object that may have nullable properties:

public class NullType {
	public List<int> List { get; set; }
}

and you validate their property without checking for null first

public class NullReferenceValidator : AbstractValidator<NullType> {
	public NullReferenceValidator() {
		RuleFor(x => x.List.Count).NotEmpty();
	}
}

It throws a generic NullReferenceException because the rule (Expression / Func) cannot be compiled. From what I read so far this is by design. However this is very hard to debug and to find out what property exactly is failing. I forked this repo and modified the Create in PropertyRule to include the expression in the closure. When you run you app in debug mode this massively helps you find out what property is null.

However this obviously has some downsides to:

  • Performance due to increased allocations (I don't really know how to fix this here)
  • Readability (may be improved somehow)

See repo for details. I even added a test to showcase this behaviour.

Is this the only way to debug this behaviour or is there another way?

Steps to Reproduce

See repo:
main...MeikelLP:FluentValidation:fix-null-debuggability

@JeremySkinner
Copy link
Member

JeremySkinner commented Sep 4, 2023

Hi

It throws a generic NullReferenceException because the rule (Expression / Func) cannot be compiled.

This isn't the case - the expression compiles successfully. However when it's invoked you'll get a NullReferenceException if the property is null, in the same way if you were to access it directly outside of a rule definition. When you work with nested properties you should always include an appropriate null check, in the same way you would with regular code outside of fluentvalidation too.

I forked this repo and modified the Create in PropertyRule to include the expression in the closure

Could you provide an example of the output so I can see what difference this makes when debugging? Thanks

@MeikelLP
Copy link
Contributor Author

MeikelLP commented Sep 5, 2023

This isn't the case - the expression compiles successfully. However when it's invoked you'll get a NullReferenceException if the property is null, in the same way if you were to access it directly outside of a rule definition. When you work with nested properties you should always include an appropriate null check, in the same way you would with regular code outside of fluentvalidation too.

Yes you are correct. The compilation does not fail but the invokation. Still this is totally fine for me but I just wanna know where it's failing not that it doesn't fail at all. I wanna know what rule/expression fails.

Could you provide an example of the output so I can see what difference this makes when debugging? Thanks

Variables available in the scope where the exception is thrown (automatically provided by Rider when debugging/decompiling the lib):

Before:
image

After:
image

@JeremySkinner
Copy link
Member

JeremySkinner commented Sep 5, 2023

Could you run the FluentValidation.Benchmarks project both with and without this change to see if it makes any meaningful difference to performance? If it doesn't then I'd be fine with including it

@MeikelLP
Copy link
Contributor Author

MeikelLP commented Sep 6, 2023

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19045
Intel Core i5-8500 CPU 3.00GHz (Coffee Lake), 1 CPU, 6 logical and 6 physical cores
.NET Core SDK=7.0.203
[Host] : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT
DefaultJob : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT

Before

Method N Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Validate_SingleRule 10000 6.329 ms 0.0692 ms 0.0648 ms 2718.7500 - - 12.21 MB
Validate_TenRules 10000 24.234 ms 0.1367 ms 0.1141 ms 6531.2500 - - 29.37 MB

After

Method N Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Validate_SingleRule 10000 8.485 ms 0.0733 ms 0.0686 ms 3359.3750 - - 15.11 MB
Validate_TenRules 10000 45.140 ms 0.2470 ms 0.2311 ms 13000.0000 - - 58.36 MB

As you can see times & allocations drastically increased. This was expected for me. So implementing it this way is a no go. Do you have any other idea?

@JeremySkinner
Copy link
Member

Yes with such a drastic difference this is a no-go.

The only alternative I can think of is to catch NullReferenceExceptions and then re-throw them with additional detail such as the current rule expression

@MeikelLP
Copy link
Contributor Author

MeikelLP commented Sep 6, 2023

But from what I can see this is not possible as the context (member info) is not captured. The exception is thrown in the compiled expression which has no information about the member which it was compiled for 🤔

@JeremySkinner
Copy link
Member

It doesn't need to. The exception can be caught by the rule instance that invokes the expression (which does have that context available), and then re-throw it.

@MeikelLP
Copy link
Contributor Author

MeikelLP commented Sep 6, 2023

Ah. If you are able to implement it that way I would greatly appreciate it :)

@JeremySkinner
Copy link
Member

I don't personally have the time to do that at the moment, but I'd be happy to review a PR. Otherwise I can look at it when I get back to working on 12.x, but I don't know when that'll be yet.

@vipwan

This comment was marked as off-topic.

@JeremySkinner

This comment was marked as off-topic.

@vipwan

This comment was marked as off-topic.

@JeremySkinner JeremySkinner added this to the 11.9 milestone Nov 24, 2023
@JeremySkinner
Copy link
Member

Merged for 11.9

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants