Throw all configuration errors together

[simonmckenzie]

Previously, if the configuration contained bad type maps, AssertConfigurationIsValid would throw immediately without performing a dry run. Now, type map errors are thrown alongside any dry run errors.

#4524

[lbargaoanu]

But my point is, why throw and catch? We can simply gather the errors instead.

[lbargaoanu]

Just remove all the empty lines.

[simonmckenzie]

But my point is, why throw and catch? We can simply gather the errors instead.

Hi @lbargaoanu,

I certainly could change the error collection to remove the throw/catch/throw - I'd like to discuss the approach first if that's ok with you.

With DryRunTypeMap, there isn't really an error class to encapsulate an error, so it could either become

private IEnumerable<AutoMapperConfigurationException> DryRunTypeMap(...)

or, since the exceptions are always constructed directly from the MemberMap (see below)

https://github.com/AutoMapper/AutoMapper/blob/b521096a3e05782df85fdb42356601a9bb3f35f8/src/AutoMapper/Configuration/ConfigurationValidator.cs#L93

, it could be this:

private IEnumerable<MemberMap> DryRunTypeMap(...)

I feel that returning exceptions (option 1) is not idiomatic .NET, while option 2 makes it harder to indicate that the output of the dry run is the set of failed member maps. It could perhaps be renamed to GetInvalidMemberMaps(...) - what do you think?

[lbargaoanu]

I think we can simply use the existing exceptions list. But DryRunTypeMap already has too many parameters. Local methods should work here. It might be a little difficult because ConfigurationValidator is a struct. We could make it a class, but it would have to be instantiated only when validation is called. Not sure about the details, but it shouldn't be too hard.

[simonmckenzie]

@lbargaoanu agreed, it should be fine. I have a cleaner version working and will push it within the week once I've finished tidying it.

[simonmckenzie]

Hi @lbargaoanu,

I've pushed a refactor where the exceptions are all created at the top layer, from the set of invalid maps. I've also done some renaming, flattened out some of the recursion to make it (to my eye) easier to understand and made the local function change you suggested in order to reduce the number of parameters.

It would probably be cleaner if I turned the struct back into a class and replaced the MapperConfiguration's
readonly ConfigurationValidator _validator with a readonly Lazy<ConfigurationValidator> _validator. Shall I do that? I don't really have enough context to understand why, if it becomes a class, it should only be created when needed - I assume it's a performance optimisation. Is the construction on a critical path?

I also need to add another test, as the replacement of the throws with yields means the validation will go deeper and surface more mapping errors upfront. I'll do that next week.

Please tell me what you think, as I'm aware I have made some opinionated changes.

[lbargaoanu]

I'm sorry, but I think this went in the wrong direction. I've opened a PR with minimal changes.

[simonmckenzie]

@lbargaoanu, I think we may just have different philosophies, but perhaps "went in the wrong direction" is more about me not being entirely clear about the reason for the changes:

• DryRunTypeMap is, to me, a misleading name. It isn't doing any mapping, as far as I can see, so it's not a dry run. It's just finding type maps that can't work. That's why I renamed it
• The recursion is very hard to follow for someone unfamiliar with the code - DryRunTypeMap -> CheckPropertyMaps -> DryRunTypeMap .... Splitting out GetPropertyMemberMaps into a separate method removed the need for this alternating conversion
• This is more of an opinion, but pushing values into a shared array is not very clean or easy to follow - my change to IEnumerable and yield allowed the direct production of a stream of errors

[lbargaoanu]

We kind of need a good reason to rewrite working code and I just don't see it here. Minor improvements should mean minor changes.

[simonmckenzie]

We kind of need a good reason to rewrite working code and I just don't see it here. Minor improvements should mean minor changes.

I find this ironic, as that's what I felt I was doing when I first opened this PR. I just moved the exception collection upward one block and added a test (e364d54).

Your response to that was this:

But my point is, why throw and catch? We can simply gather the errors instead.

i.e. to rewrite working code. I took this as a request from you to refactor what is, to me, a very complex piece of code, which of course led us to this point, where it turns out that isn't what you wanted.

I feel that what you've done in #4526 is on the same level of change as what I've done - you've changed the validator instantiation model, you've changed a struct to a class, started collecting exceptions etc.

Anyway, of course I have no expectation of getting any of my changes into the project, I just feel we have not been able to communicate clearly with one another throughout this process, and it's been very disappointing.

[lbargaoanu]

Sorry about that. But if you look again, hopefully you'll see how my PR is a bunch of trivial changes, as opposed to what you've done here.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed.