Autofac: ParameterExtensions.Named<T> throws for NamedPropertyParameter collection

Created on 21 Mar 2019  路  4Comments  路  Source: autofac/Autofac

Extension method ParameterExtensions.Named<T> take as an input parameter collection of IEnumerable<Parameter>. This way it could be called on collections of any types that derives from Parameter abstract class. Method itself calls ConstantValue<TParameter, TValue> with NamedParameter constraint and that causes filtering and further InvalidOperationException.

Test case that confirm current behavior (bug):

public class ParameterExtensionsTests
{
    [Fact]
    public void Named_Support_Only_NamedParameter_But_CanBe_Called_For_Any_Other_Abstract_Type()
    {
        var somePropertyParameter = new NamedPropertyParameter("Repository", new object());
        var propertyParameters = new[] { somePropertyParameter };

        Assert.Throws<InvalidOperationException>(() => propertyParameters.Named<object>("Repository"));
    }
}
enhancement help wanted

All 4 comments

This is actually functioning as designed. Named<T> is a convenience shorthand operator that combines several LINQ statements together for you.

Let's say you have this test:

```c#
[Fact]
public void LargeNumberTest()
{
var numbers = new int[] { 1, 2, 3 };
var result = numbers.Where(n => n > 100).First();
}


You have a list of numbers and you want to find the first one that matches a criteria. In this case, the `First()` call will result in `InvalidOperationException` because there aren't any numbers that are greater than 100, so the `Where` clause will return an empty list... and there's no first element of an empty list.

Now say you commonly want to get the first number greater than 100, so you wrap that logic into an extension.

```c#
public static class NumberExtensions
{
    public static int FirstNumberGreaterThan100(this IEnumerable<int> numbers)
    {
        return numbers.Where(n => n > 100).First();
    }
}

Then you can use it without having to write that out. Nice.

```c#
var val = numbers.FirstNumberGreaterThan100();


You can't return `FirstOrDefault()` because if you ask for a number greater than 100 and there isn't one... you'd get a 0. You actually are _very much requiring_ there be a number greater than 100 in that list or you will get an error. You can handle the exception coming out of there, but it doesn't really make sense to return 0 as the answer.

Technically, you could turn this into a Try style method...

```c#
if(!numbers.TryGetFirstNumberGreaterThan100(out var result))
{
  // handle the missing value
}
else
{
  // do something with the result
}

but that's kind of cumbersome when the point of it is that you're 100% expecting the proposition to be fulfilled - that you'll get a number greater than 100.

That's how Named<T> (and the others, like Positional<T>) work. You'll get a list of all the possible parameters and when you ask for it, you expect the proposition to be fulfilled. It allows you to shortcut a lot of null checking language and just make the assumption during lambda registrations.

```c#
builder.Register((ctx, p) =>
{
var value = p.Named("value");
return new Thing(value);
});


This is sort of part of the public API contract for it at this point, folks expect the exception to occur. We can't change that or it'll break that expectation, for better or worse.

You could always add an extension method of your own if that's a scenario you use a lot (or we would take a PR) to add a Try version of these. From the outside (like in your project) you could do...

```c#
public static bool TryGetNamed<T>(this IEnumerable<Parameter> parameters, string name, out string value)
{
  try
  {
    value = parameters.Named<T>(name);
    return true;
  }
  catch(InvalidOperationException)
  {
  }

  value = null;
  return false;
}

If you did a PR, you could potentially refactor the internals of the ParameterExtensions class to be more efficient than that, maybe using FirstOrDefault and manually throwing the InvalidOperationException as needed to ensure the contract is upheld.

Thanks for the reply. Probably I have not clearly described the problem. I'm not complaining that this method throws an exception. The only thing is, that I can call it i.e. on a collection of IEnumerable<NamedPropertyParameter> and that will always throw - even for the existing parameter - due to call ConstantValue<NamedParameter, T>. See ParameterExtensions.cs:67

My assumption was that I can use it on any kind parameter collection.
Summarizing maybe header of the method should be:

public static T Named<T>(this IEnumerable<NamedParameter> parameters, string name)

To be more explicit. What do you think?

You can't make the assumption that the IEnumerable will only include NamedParameter - that's the point. It looks through a list of _all the parameters passed to the resolve_, which will potentially be all kinds - NamedParameter, NamedPropertyParameter, TypedParameter, etc. - and it filters out properties of the correct type. If you have an enumerable that only has a NamedPropertyParameter in it, then yeah, it's going to throw.

I added some unit tests that show some example situations and the expected outcomes. The key one, I think, is this:

```c#
[Fact]
public void NamedFindsCorrectParameter()
{
var parameters = new Parameter[]
{
new TypedParameter(typeof(int), 5),
new NamedPropertyParameter("prop", "property"),
new NamedParameter("value", "named"),
new PositionalParameter(2, "position")
};

        Assert.Equal("named", parameters.Named<string>("value"));
    }

This is totally valid. Because, like I said, in the registration you'd have:

```c#
builder.Register((ctx, p) =>
{
  var value = p.Named<string>("value");
  return new Thing(value);
});

and in the resolve, you might have:

```c#
var service = scope.Resolve(
new NamedParameter("value", "service-identifier"),
new TypedParameter(typeof(Guid), Guid.NewGuid()),
new ResolvedParameter(
(pi, ctx) => pi.ParameterType == typeof(ILog) && pi.Name == "logger",
(pi, ctx) => LogManager.GetLogger("service")));


(You can see this [in the docs about resolve parameters](https://autofac.readthedocs.io/en/latest/resolve/parameters.html).)

The set of parameters provided to you in the lambda is _all the parameters_ not just named ones.

From a DI perspective, keep in mind that it's usually more complicated than this. The `Thing` you're resolving is usually an `IThing` and you may change the implementation. When you resolve the `IThing`, you _won't and shouldn't know_ what the underlying type is. So you may pass parameters that are sometimes needed, sometimes not.

Looking at your example, what you have is basically:

```c#
var service = scope.Resolve<Thing>(
                new NamedPropertyParameter("prop", "service-identifier"));

so when the p.Named<string>("value") runs in the lambda (like in my example), it's not going to find any NamedProperty instances in the list and it'll throw. The thing you asked for isn't in the collection. The expected behavior is to see that InvalidOperationException.

Am I still not understanding? I feel like there may be a gap in what the intended usage/use case is and what you're trying to do with it.

Oh gosh, now I get it! The funny thing is that I tried to use that method in one of my unit tests as a shorthand. And my first thought was that sth is wrong with it.
I'm making some kind of metaprogramming. Reading xml data (in runtime) -> extracting params -> inject to methods.
And I use Named<> in test that check if correct params was prepared. Something like that:

var parameters = ParamTransform.MakeDownloadCheckDefinitionParams(Repository, downloadCondition, downloadCustom);

var namedParameters = parameters.OfType<NamedParameter>();
var propertyParameters = parameters.OfType<NamedPropertyParameter>();

Assert.That(namedParameters.First(x=>x.Name.Equals(nameof(downloadCondition))).Value, Is.Not.Null, "downloadCondition is not set");
Assert.That(namedParameters.First(x=>x.Name.Equals(nameof(downloadCustom))).Value, Is.Not.Null, "downloadCustom is not set");

Assert.That(namedParameters.First(x=>x.Name.Equals("repository")).Value, Is.Not.Null, "repository param is not set");
Assert.That(propertyParameters.First(x=>x.Name.Equals(nameof(CheckBase.CheckErrorLevel))).Value, Is.EqualTo(CheckErrorLevel.Fatal), "CheckErrorLevel property is not set");
Assert.That(propertyParameters.First(x=>x.Name.Equals(nameof(CheckBase.ErrorMessage))).Value, Is.EqualTo("Ups! Fatal one more time."), "ErrorMessage property is not set");

And I thought that I can use Named<> instead of lambda in assertions.
Thank you for explanation!

Was this page helpful?
0 / 5 - 0 ratings