search

feO2x / Light.GuardClauses

A lightweight .NET library for expressive Guard Clauses.
MIT License
85 stars 8 forks source link

Enumerable MustNotBeNullOrEmpty shows wrong message #53

Closed ivancavlek closed 6 years ago

ivancavlek commented 6 years ago

When checking an enumerable guard with MustNotBeNullOrEmpty and setting a custom message with message parameter, a default message is being displayed instead of the custom one.

Example (collection is set to null): collection.MustNotBeNullOrEmpty(message: Message.CustomMessage);

Test result:

Message: Expected exception message to match the equivalent of "Custom message", but "The value must not be null." does not.

I have also noticed that if I set a custom exception, it does not display the custom message as well. The problem, as I debugged it, is in definition of MustNotBeNullOrEmpty where a call to MustNotBeNull does not send the message and exception, but just the parameter. Maybe I am wrong… This could potentially be a flaw in other methods as well.

feO2x commented 6 years ago

You are right - this will be fixed in version 4.0 that will be probably released in two weeks. The new implementation looks like this: https://github.com/feO2x/Light.GuardClauses/blob/996ad03398be68d8ed0fbf63f7217ec47ec97d59/Code/Light.GuardClauses/Guard.EnumerableAssertions.cs#L70

Do you need a hotfix for version 3.5?

ivancavlek commented 6 years ago

Thanks, I don't need the Hotfix. I can wait for two more weeks.

On a side note, have you tested this code when a collection is null for message, exception and parameterName? It looks as it won't work on line 72 on Count()…

As I see it, if parameter is null, Count will throw its internal exception thus again preventing custom message, exception and parameterName. Previous implementation was fine. It only needed an extension for message and exception. Here's the internal implementation of Count -> http://www.csharp-examples.net/linq-count/

feO2x commented 6 years ago

@ivancavlek The Count method is actually an implementation of Light.GuardClauses (there is no Count for the IEnumerable interface, only for IEnumerable<T>). This is done for performance reasons.

https://github.com/feO2x/Light.GuardClauses/blob/996ad03398be68d8ed0fbf63f7217ec47ec97d59/Code/Light.GuardClauses/FrameworkExtensions/EnumerableExtensions.cs#L150

feO2x commented 6 years ago

Sorry for the delay, but version 4.0.0 is out now with this issue being fixed. Please have a look at the release notes as I introduced a lot of breaking changes to support more platforms.

69ab020e8c609022c73f6ff2824833fb7b32e90a

ivancavlek commented 6 years ago

Hi, I just downloaded the new version and ran the tests again. Unfortunately, I still don't get the custom message, but instead "System.ArgumentNullException: 'enumerable must not be null.'".

I am using the method overload with parameter and message and passing by my custom message same as the first time:

collection.MustNotBeNullOrEmpty(message: Message.CustomMessage);

I couldn't debug what was exactly wrong as it looks fine. Version installed is 4.0.

feO2x commented 6 years ago

Hey @ivancavlek,

sorry, I just realized I have been misunderstanding your question up until now. The behavior you are observing in your test is actually the correct behavior of Light.GuardClauses. If you look closely at the XML comment for message, it states: "The message that will be passed to the EmptyCollectionException (optional)". This means that your message is only passed to the exception if an EmptyCollectionException is actually thrown - but in your test scenario, an ArgumentNullException is thrown.

In Light.GuardClauses, there are several assertions that test several things in one call like e.g. MustNotBeNullOrEmpty for collections which checks:

or e.g. MustHaveHttpsUri which checks:

As you can see, different exceptions are thrown depending on the check that fails. When designing Light.GuardClauses, I wondered if it is useful to pass the message to all different exceptions or just to the last one. I went with the latter because I thought "That's what the user is really interested in. The checks before are just preconditions for the last one.". I can see now that this might be irritating to end-users who might expect the message to be in the resulting exception, independent of the exception type.

And this is the same behavior for custom exceptions: only the last exception thrown is replaced, following the same logic.

So, to fix your test, you would have to change it to something like this:

[Fact]
public void CollectionNullCustomMessage()
{
    const string customMessage = "MyCustomMessage";

    Action act = () => ((List<string>)null).MustNotBeNull(message: customMessage).MustNotBeNullOrEmpty(message: customMessage);

    act.Should().Throw<ArgumentNullException>()
         .And.Message.Should().Contain(customMessage);
}

(This test requires xunit.net and FluentAssertions).

So, what's your opinion on this? Is is easier to understand for end-users to always see their message in the resulting exception? Or should e.g. an ArgumentNullCheck (which is the first check most of the time) always have the same message, no matter if the caller passed in a custom one or not?

ivancavlek commented 6 years ago

Ok, my 2c. It's simple - if I state that I want a certain message to be shown, then I want that message to be shown, irrelevant of the end exception.

IMHO, your thinking is faulty. If you really wanted the behavior that you implemented, then you should've separated the methods into MustBeNull and MustBeEmpty. Even in this case, MustBeNull shouldn't receive a message as a parameter.

When you state a message as a parameter, you're giving a clear message that what I write in this parameter, it'll be shown in the exception. Theoretically, current implementation violates S in SOLID (Single Responsibility Principle) because your method is doing two things, although its signature states that it should do one thing only. First, you write a parameter that you ignore; second, you throw two different exceptions. Think of it this way, it's like a method that's returning two different parameters, or (no offense :)), it's signature is a lie.

Unless you're familiar with them, I'd suggest that you read Uncle Bob's Clean Code style guide.

As a third party library (that I believe is really good), your methods should be clear and concise and they should speak for themselves. That I have to use my time in debugging what's wrong with 3rd party library makes you not like it as you can't rely on it. A user wastes his/hers time looking what wrong with the code and then wastes more time looking into 3rd party libraries. And money = time = money :), so it's not helping.

In short, your method names and signatures aren't conveying their true nature. You should either split them as I stated at the beginning, or (IMO, better) let it fall through the stack. Different exceptions are debatable and I don't have a clear answer on that one. I lean more toward custom one as it's not something unexpected and is more in line with SRP. Comments shouldn't convey what does a method do, but it should be clear from it's name and signature.

Maybe I was a bit harsh, but I mean well :).

feO2x commented 6 years ago

Hey @ivancavlek,

thanks for your feedback. Let me give you some context why I choose to manipulate only the last exception (and not all of them). The main purpose of Light.GuardClauses is to minimize the effort of validating parameters at the beginning of methods. Ideally, a single extension method should be called on a parameter, and this lead to some assertions checking two or more things, like e.g. string.MustNotBeNullOrWhiteSpace or Uri.MustBeHttpsUrl. All of these included a null-reference-check at the beginning followed by one or two other checks. The first thing I thought about at the time was "If I specify an exception factory delegate, which of the exceptions should actually be replaced? All of them? Or only the one that I'm checking for?" and I went with only the last one because I wanted to keep the ArgumentNullExceptions intact (when something is null, why shouldn't an ArgumentNullException be thrown? That didn't make sense to me - which, of course, is a subjective decision).

To keep the whole thing consistent, I choose to do that with messages, too. Now I agree with you that this way of thinking is not very intuitive, and I should clearly communicate this in the documentation (it's included in the XML documentation, and I'm currently building up a more cohesive wiki).

But I cannot agree with your SRP argumentation. The Single Responsibility Principle does not state that a module should only do one thing, but that it should only have one responsibility. And responsibility was meant in the context of persons (C-executive members) that have a reason to change it. Thus a module can do several things if you as a developer see fit, as long as these belong to the same problem area / responsibility. And I cannot see how a MustNotBeNullOrWhiteSpace spans over several responsibilities. It's in the problem are of validating a string. See chapter 8 of Agile Principles, Patterns, and Practices in C# or this video of Uncle Bob.

Why don't we take a psychological point-of-view, i.e. what comes to mind when a developer reads the assertion name? Would you expect to manipulate the ArgumentNullException if the assertion was called MustNotBeEmpty instead of MustNotBeNullOrEmpty? Maybe we can agree on this: in MustBeHttpsUrl, there is no null included, thus the end-user does not expect to manipulate the ArgumentNullException, while in MustNotBeNullOrEmpty the Null is part of the name and thus the ArgumentNullException can be customized.

Of course, this is an inconsistent behavior across different assertions. Another thing is to replace all exceptions with your custom one, or inject the message into all exceptions, respectively. I will think about this, but it's a huge undertaking as we basically have to touch every assertion that is passed in an exception factory delegate.

ivancavlek commented 6 years ago

Well, we both agree that it's not intuitive :D. Inconsistency is bad as well.

Unlike other similar libraries, yours is easy to use and doesn't require any special calls or gimmicks. We can debate over exceptions, although I still lean more on custom ones, but messages are definitely non debatable. A signature says "I will do this" and it doesn't do it.

I hope our little talk helped you a bit. I'm happy to debate over stuff if you'd like :).

Viel Glück!

feO2x commented 6 years ago

Hey @ivancavlek,

after a night of sleep I agree with you and I want to change the implementation. If we take a look at a typical exceptionFactory delegate, then it normally looks like this (as a lambda):

parameter => new CustomException($"Here is some custom exception message using {parameter}");

Here are my thoughts: as an end-user, I do not want to fiddle around with parameter being null. I just want to put the invalid value easily inside a message. This would lead me to the following principles for assertions that perform several checks:

What's your opinion on these? Do you think they're intuitive?

ivancavlek commented 6 years ago

Custom exceptions are only thrown when parameter is not null. If it is null, Light.GuardClauses throws an ArgumentNullException

You'll have to explain me the reasoning behind this and why do you want to do it, 'cause I don't understand it. IMO, these methods should be simple with a simple and clear execution, not if-if. This is a simple to use library, not an API of an enterprise system (no offense :)).

If you want to stick to original exceptions, I'd say that you throw them only when the user hasn't specified his exception in exceptionFactory delegate. Thinking out loud, what do you say that you always throw your internal custom exception (if not specified otherwise in exceptionFactory) and make original exceptions (like ArgumentNullException) an inner exception? Just a thought...Would that bring anything (except a total rewrite :))? I'd be consistent and if anyone really wants an original message, he can have a look inside inner exception, if there is one. Inner exception would have a parameter and message as specified by the user.

All other exceptions are replaced with the custom one provided by the exceptionFactory delegate

Agreed!

Messages are always passed to all exceptions (even ArgumentNullException)

Agreed!

feO2x commented 6 years ago

My main reasons for treating null values specially:

Simply put: if it's null, then you can always expect an ArgumentNullException.

ivancavlek commented 6 years ago

Ok, to continue the debate and the train of thoughts, what about other exceptions like ArgumentOutOfRange or something similar, lets say an exception for a DateTimewhich can't be null?

My point is, if you throw a built-in exception, do you have a built-in exception for all of your situations? Are they common for those situations?

Yes, missing registrations are a problem, BUT they're not your concern. Your concern is to give the right, concise and simple answer to the client if the guard failed or not through an exception. We could have pre-conditions, post-conditions (maybe mid-conditions), but having only one custom exception like GuardException tells the customer that the guard failed. Why? Not the libraries' problem! It just validates what you've sent it.

If you throw a built-in exception, you could expect someone to come and say "That should be this built-in, not that" (mostly because I/we use it) and then all hell breaks loose. How can you argument with someone when the .NET documentation itself isn't clear about it? There are hundreds of built-in exceptions.

We got stuck with ArgumentNullException, but there are other situations as well. What about them? Where is the border between built-in and custom? How will you decide what to do in other situations? Documentation and null parameters are not the answer, IMO, and you'll get spaghetti that's unclear.

One custom exception is clear -> contract failed!

feO2x commented 6 years ago

I can see that your solution is the purest one that is easiest to grasp. I was wondering if this is the best one in terms of overall usability (regarding the passed in parameter might be null), However, as Light.GuardClauses provides a fluent API, I could easily write something like this, for example:

uri.MustNotBeNull()
   .MustBeHttpsUrl(url => 
    {
        // url is guaranteed to not be null here because of the previous MustNotBeNull check
        return new MyCustomException("Why does your URL \"{url}\" not use HTTPS? What's wrong with you?");
    });

Thus, let's agree on:

ivancavlek commented 6 years ago

I agree on both!

feO2x commented 6 years ago

I've implemented Light.GuardClauses in the last few days so that custom exceptions and messages now work as we agreed upon.

Implemented in e10fdacf3aa92bc27e51231f6bb81ddd513d6174

Corresponding release: https://github.com/feO2x/Light.GuardClauses/releases/tag/v5.0.0