ardalis / GuardClauses

A simple package with guard clause extensions.
MIT License
3.06k stars 270 forks source link

Package Release 4.6 update causes Runtime breaking change: Missing Method Exception #354

Open DDH-DGillett opened 2 months ago

DDH-DGillett commented 2 months ago

Environment

Subject of the issue

Where there is a hierarchy of code that use Ardalis.GuardClauses updating the top level package, without updating the tranisitive library package to use the same version complies without error, but throws System.MissingMethodException : Method not found: 'System.String Ardalis.GuardClauses.GuardClauseExtensions.NullOrEmpty(Ardalis.GuardClauses.IGuardClause, System.String, System.String, System.String)'.

Steps to Reproduce:

  1. Update top-level package in client solution from v4.5 to v4.6
  2. Version 4.5 remaining in library packages
  3. Run application or tests
  4. Exception is thrown when guard clause is called.

Expected behaviour

Update to top-level package should not cause runtime exception where no changes have been made to top-level code.

Actual behaviour

Calls to guard clauses throw:

System.MissingMethodException : Method not found: 'System.String Ardalis.GuardClauses.GuardClauseExtensions.NullOrEmpty(Ardalis.GuardClauses.IGuardClause, System.String, System.String, System.String)'.

Tested resolution

Updating all libraries using the package to version 4.6 resolved the issue, as did reverting top-level package to 4.5 or removing top-level package and using transitive.

ardalis commented 2 months ago

I've seen this as well but I'm not sure how to resolve it. I agree it's a problem and especially nasty since it's only detected at runtime.

This seems related but not sure it's actually a fix: https://stackoverflow.com/a/68486007/13729

The "rules" seem to be here: https://learn.microsoft.com/en-us/nuget/concepts/dependency-resolution#dependency-resolution-rules

image seems to indicate there would at least be a warning, so maybe treating warnings as errors would prevent this?

DDH-DGillett commented 2 months ago

My thoughts were a multi-step approach to handle the change:

  1. Update the release notes of 4.6 to note this is breaking change at runtime if there are any transitive dependencies for this package.
  2. Then consideration given to versioning, perhaps this should be 5.0 (since it is breaking) or 8.0 to align with .NET version?
  3. Finally, is it worth considering an alternate approach to resolve this issue entirely? For example via moving away from optional parameters, via refactoring, to method overloads or add new methods that accept the exceptions.

For example:

    public static ReadOnlySpan<char> Empty(this IGuardClause guardClause,
        ReadOnlySpan<char> input,
        string parameterName,
        string? message = null,
        Func<Exception>? exceptionCreator = null)
    {
        if (input.Length == 0 || input == string.Empty)
        {
            Exception? exception = exceptionCreator?.Invoke();

            throw exception ?? new ArgumentException(message ?? $"Required input {parameterName} was empty.", parameterName);
        }
        return input;
    }

Could be split into:

    public static ReadOnlySpan<char> Empty(this IGuardClause guardClause,
        ReadOnlySpan<char> input,
        string parameterName,
        string? message = null)
    {
        if (input.Length == 0 || input == string.Empty)
        {
           throw new ArgumentException(message ?? $"Required input {parameterName} was empty.", parameterName);
        }
        return input;
    }

And:

/// <summary>
    /// Throws an <see cref="ArgumentException" /> or a custom <see cref="Exception" /> if <paramref name="input" /> is an empty string.
    /// </summary>
    /// <param name="guardClause"></param>
    /// <param name="input"></param>
    /// <param name="exceptionCreator">Optional. Custom exception</param>
    /// <returns><paramref name="input" /> if the value is not an empty string.</returns>
    /// <exception cref="ArgumentException"></exception>
    /// <exception cref="Exception"></exception>

    public static ReadOnlySpan<char> Empty(this IGuardClause guardClause,
        ReadOnlySpan<char> input,
        Func<Exception> exceptionCreator)
    {
        if (input.Length == 0 || input == string.Empty)
        {
            throw exceptionCreator.Invoke();
        }
        return input;
    }
ardalis commented 2 months ago

I think the right next step is to publish a v5 version and if possible update the release notes for v4.6 as you suggest.

I've added some more notes here as well: https://github.com/ardalis/GuardClauses/pull/350 which was the PR that added the breaking change (at my request).

As for splitting the methods, I see your point in that any particular call is either going to use a custom exception message or a custom exception (factory). I'll consider that although it does double the size of the codebase...

leus commented 1 month ago

Hm, I think this needs to be reverted in a patch version, then added back in a new major version with a clear method contract change. We are experiencing ripple effects due to this in our platform (we use "4.*" for our versioning, which now we need to rethink).

ardalis commented 2 weeks ago

Ok, I've updated the release notes on 4.6 (later than I'd meant to - sorry!) and published a new v5.0 version which only has one change in it (non-functional - change to Obsolete message for old AgainstExpression guards) relative to the 4.6 release which had the breaking change(s) in it.

Please let me know if this resolves any issues you have or if there are still problems remaining.

My recommendation if you don't want to change anything and you've been on 4.x for a while is:

Stay on 4.5

If you are willing to make some (minor) changes to your code to accommodate the breaking changes introduced in 4.6:

Update to the latest 5.x version.

mwasson74 commented 2 weeks ago

@ardalis

  1. Has the "Missing Method Exception" been fixed in any version, if so, which version(s)?
  2. Is there a version or versions where we can pass a custom exception and also not receive the "Missing Method Exception" message?
  3. What are the minor changes to our code that accommodates the breaking changes?

Thanks!!

Matt

ardalis commented 2 weeks ago
  1. This issue remains; I'm still able to repro it using a parent project targeting 5.0 and a dependent one targeting 4.5.
  2. Using 4.5 for both or using 5.0 for both (parent and dependents) works fine. A mismatch spanning versions 4.x and 5 (or 4.6) triggers the behavior.
  3. My comment about minor changes assumed you didn't have the mismatch in versions described above. In which case it would involve passing a Func if you wanted to specify a custom exception, for instance.
ardalis commented 2 weeks ago

I'm not sure I'm going to be able to resolve the missing method exception across the v4.5 - vLater divide at this point. Adding method overloads just leads to ambiguous calls because of the use of nullable/default parameters on the methods.

I probably could make it work by explicitly creating overloads for every supported parameter combination, maybe, instead of using default parameters. But it would result in a lot more code and might be less flexible.

How important is it to folks to try to achieve backward compatibility with 4.5 and earlier in versions 4.6 and later (at this point it would come in a new version 5.1 or 6.0 or something)? Leave a 👍 or something if you need this, and I'll consider it. Otherwise, stick to 4.5 if anything is on 4.5 or earlier and latest for everything otherwise.