Throw informative exception when no name in services element

[blairconrad]

Fixes #18.

[tillig]

First, thanks for the PR! I do appreciate the help.

Here's my stumbling block, and maybe it's not a big deal, but is something to consider: while this fixes the very specific issue marked in #18 (the services node in XML not having a name attribute)... it doesn't catch any of the other places that also need to be properly named for ordinal collections.

Here's the documentation showing some configuration snippets.

Making a more complete example with more ordinal collections out of the snippets there, you'll get something like...

{
  "components": [{
    "type": "Autofac.Example.Calculator.Division.Divide, Autofac.Example.Calculator.Division",
    "services": [{
      "type": "Autofac.Example.Calculator.Api.IOperation"
    }],
    "metadata": [{
      "key": "answer",
      "value": 42,
      "type": "System.Int32, mscorlib"
    }]
  }],
  "modules": [{
    "type": "Autofac.Example.Calculator.OperationModule, Autofac.Example.Calculator"
  }]
}

Every place you see an array is a place in XML that will need a name attribute.

<?xml version="1.0" encoding="utf-8" ?>
<autofac>
    <!-- name on component -->
    <components name="0">
        <type>Autofac.Example.Calculator.Division.Divide, Autofac.Example.Calculator.Division</type>
        <!-- name on services -->
        <services name="0" type="Autofac.Example.Calculator.Api.IOperation" />
        <!-- name on metadata -->
        <metadata name="0" key="answer" value="42" type="System.Int32, mscorlib" />
    </components>
    <!-- name on modules -->
    <modules name="0">
        <type>Autofac.Example.Calculator.OperationModule, Autofac.Example.Calculator</type>
    </modules>
</autofac>

I didn't bother putting in properties or parameters that have list values, but you'll see an example of that in the unit tests.

I wonder if a solution to catch this would be to add some sort of integer parsing in the foreach loops where we iterate through ordinal collections, like:

foreach (var serviceDefinition in component.GetSection("services").GetChildren())
{
  // Make sure there's a name that can be parsed into an integer to ensure it's ordinal?
  if(!Int32.TryParse(serviceDefinition.Key, out var name))
  {
    throw new InvalidOperationException("Name in ordinal collection has to be an integer.");
  }

  // "name" is a special reserved key in the XML configuration source
  // that enables ordinal collections. To support both JSON and XML
  // sources, we can't use "name" as the keyed service identifier;
  // instead, it must be "key."
  var serviceType = serviceDefinition.GetType("type", defaultAssembly);
  string serviceKey = serviceDefinition["key"];
  if (!string.IsNullOrEmpty(serviceKey))
  {
    yield return new KeyedService(serviceKey, serviceType);
  }
  else
  {
    yield return new TypedService(serviceType);
  }
}

I'm not glued to that logic or exception message, and didn't even try to compile it, but it'd have to happen on any place where we expect an ordinal collection (anywhere it'd be an array in JSON instead of a dictionary).

I'm not entirely comfortable with putting only XML guidance in there, either, because, again, we don't know if it's XML, INI, JSON, environment variables, command line variables, or what. It's a benefit and a drawback. Can we make it more all-inclusive? Maybe with a link to the documentation at http://autofac.readthedocs.io/en/latest/configuration/xml.html and/or some docs on how to use Microsoft.Extensions.Configuration.

Yeah, the URL there is to xml.html but it's been left like that to avoid breaking links from external sites. It's not just XML anymore.

Maybe a message like...

`The '{0}' collection should be ordinal (like an array) with items that have numeric names to indicate the index in the collection. An item under '{1}' didn't have a numeric name so couldn't be parsed. Check http://autofac.readthedocs.io/en/latest/configuration/xml.html for configuration examples.'

Format args could be the name of the thing being parsed (services, modules, etc.) and the parent item from which the collection is being read (the full key, configurationItem.Path IIRC, which will render like components:1:metadata or something like that).

Of course, here's the real kicker if we step back from all this: Autofac really doesn't care about the ordinal nature of the collection.

Look at the example in this Microsoft doc in the XML section. As long as the names are unique it doesn't matter what they are. We don't even use them, we just iterate through them and ignore the actual names. If we did use the names, yeah, they'd have to be numbers because that's how JSON and other parsing works to map arrays to a flat key/value set. But we don't care internally to our config consumer whether it's an array or what. If someone provided flat config like...

components:ignored1:type = "A, MyAssembly"
components:does-not-matter:type = "B, MyAssembly"
components:who-cares:type = "C, MyAssembly"

That's totally legit. Which is kind of cool, because then it leaves the extensibility of the configuration system pretty open. Maybe you want to use XML config so you can control that little piece there and do something with it. Or maybe you need it strongly named so in XML config that gets deployed with your app you can put one thing in and in environment you can easily override it at runtime without re-deploying.

We don't even care if you jam extra attributes and things in there. Want to store a bunch of _comment nodes in your JSON? Go for it. Totally resilient to that.

That's why using the abstraction is really neat... but it's new, so there's some ramp-up for folks and it looks like it's an Autofac problem when it's really malformed config. And that's why I didn't really solve this and was trying to think of ways to catch malformed config while still staying at the abstraction level, not diving into specifics of XML, JSON, or whatever.

Am I off in the weeds?

[blairconrad]

Thanks for the thoughtful response, @tillig. I'm making more work than I'm saving! Responses, not in the order the original points appeared.

it doesn't catch any of the other places that also need to be properly named for ordinal collections

Ah, yes. I was intentionally targeting the mentioned scope, for fear of breaking things I don't understand. Happy to do otherwise.

I'm not entirely comfortable with putting only XML guidance

Ah, yes. At the time I thought the options were XML or JSON, and that JSON would not suffer from the problem, so considered it harmless.

We don't even use them, we just iterate through them and ignore the actual names.

Which was part of the reasoning behind my "just make it work" approach. If Autofac doesn't need them, why not work without them?
But if Microsoft.Extensions.Configuration really likes a name in the items, then a name it shall have. (Although, truth be told, I find it odd that it's so excited about having it, yet makes no attempt to ensure it's there. I guess it can't anticipate the usage.)

Autofac really doesn't care about the ordinal nature of the collection.

Yay! The restriction is relaxed. We can just check for presence.

As long as the names are unique it doesn't matter what they are.

Boo! It got harder again.

Int32.TryParse(serviceDefinition.Key, out var name)

I believe that Key is actually the name of the attribute used as the key, so it'll be type when there's no name (for a services node, that is). We more likely want to check that the key is name and parse the value. Unless we don't want to parse the value because we don't care about the name being a number.

Either way, I'll start reworking and see if I can come up with something more palatable.

[tillig]

Possible confusion: configItem.Key is not the same as configItem["key"]. The config abstraction is all key/value. Ordinal collections parse out like

components:0:type = "A, MyAssembly"
components:1:type = "B, MyAssembly"
components:2:type = "C, MyAssembly"

When iterating through the children of the components section, you'll get the "keys" 0, 1, 2.

The XML parser specifically uses the "name" property to disambiguate elements of the same type (like two <components> elements). That's just their XML parser.

The whole point of any of the parsers is to flatten it all out to key/value pairs. There are some peculiarities around each one. The XML parser is particularly painful. They really want people to use JSON since it's really just easier.

[blairconrad]

When iterating through the children of the components section, you'll get the "keys" 0, 1, 2.

Yeah, sorry. I was wrong. Also off working on stuff and discovering this very thing. Honestly, I don't understand how the Key works at all.
(Where does your knowledge come from? Have you come across a much more extensive trove of documentation than I have, or is it mostly experience? I can't even find the source.)

When you omit name from the services, Key is "type", and Value is the actual value of the "type" attribute. Which lead me to believe (foolishly) that it's picking an attribute to be the Key, and fell back to "type" when "name" wasn't there.

Ah well, I will soldier on. In (my) morning. It's not super late here, but I'm working on about 5 hours of sleep. I'm heading up the wooden stairs to Bedfordshire. Sorry for my continued ineptitude. 😁

[tillig]

Hey, don't sell yourself short. And don't let it sound like I don't appreciate your help. I do, very much.

My knowledge is mostly through experience. Thinking I should write a blog article or something. I had a lot of discussions about this and other concerns in .NET Core with that team in early days both via work and through Autofac so I was in early on with this ordinal collection handling thing. (It was a late add before release.)

Anyway, keep hitting breakpoints with the debugger and using Quick Watch. In this particular instance, it's one of the most educational things. ;)

[blairconrad]

don't let it sound like I don't appreciate your help

Oh, sorry. I didn't mean to sound like I thought you were ungrateful. You've not seemed so in any way.
I'm just bummed because so far me taking a crack at this issue is by no means "lightening your load".

keep hitting breakpoints with the debugger

Will do. For some reason up until yesterday, I was unable to even debug through. TestDriven.NET and ReSharper are still not my friends. Resorted to VS Test Runner, but at least it's functioning. Prior to that I was "debugging by printf". And by "printf", I mean "throwing weird exceptions with informative messages"!

[blairconrad]

Repushed. I generally took your work and pasted it in. The exception message is a little different (and ever so slightly repetitive), mostly due to the lack of IConfiguration.Key (only on IConfigurationSection). If it irks, I'm happy to change.
As you can see, I did nothing for list values in parameters and properties, as the code seemed to be doing the best it could already - in properties or parameters, the presence of the numeric keys is used to determine whether or not a value is a list. Sadly, my investigation of what happens when there's a mixture of numeric and absent "name" attributes went nowhere, as when the "name" is dropped, the entire element is similarly dropped from the configuration. I did add a characterization test during my investigation, which I left in the PR, but if it's not wanted, say the word.

As always, I'm happy to make any changes.

[tillig]

Works for me! Only suggestion I have is to move the string from the exception message into a resx file. We have one in Properties/ConfigurationResources.resx you can piggyback from. Nice work!

[blairconrad]

move the string from the exception message into a resx file

Drat! I'd seen it and initially inlined the string for expediency and… forgot to go back.
I'll fix up tonight. Okay to amend the commit, or do you prefer a fixup?

[tillig]

Whichever is easiest for you. Thanks!

[blairconrad]

Thanks, @tillig. I decided that easiest was "both".
Made the change and squashed. Unsquashed can be seen at https://github.com/blairconrad/Autofac.Configuration/tree/missing-name-throws-unsquashed, in case it eases your diff-reading.

[tillig]

Looks super. Thanks! Merging now!

[blairconrad]

Thanks, @tillig. For the merge and your patience. I had a good time.