Automapper: Building custom IObjectMappers based on existing mappers requires a lot of internal class duplication

[matt-psaltis]

Some general feedback on upgrading some custom IObjectMappers to the latest signatures. I've had to take copies of:-

1. CollectionMapperExtensions
2. ExpressionExtensions
3. TypeHelper
4. TypeExtensions

As these are all internal classes, the time to implement a custom object mapper which is closely based on an out of the box mapper is much longer. Is there a way this functionality can be exposed without causing method name ambiguity due to duplicate extension method names?

[lbargaoanu]

See the discussion in https://github.com/AutoMapper/AutoMapper/pull/1734.

[matt-psaltis]

Not sure if I should continue the discussion here on the pull request or back over in my original feedback issue, but I would definitely like to voice a +1 at the very least for simplifying the experience of building custom object mappers. I like the approach of exposing the internal namespaces as described in the EF blog link above. As a developer, I would fully expect the following when utilising something in an "Internal" namespace:

1. The code is without general support - submitting a ticket / issue would generally require proving the internal functionality breaks when used through the standard API surface area.
2. I'm responsible for how I interact with this code, my own tests proving it works correctly for my use cases. (Really that's ways the case, just that I would put a larger emphasis on testing this code in isolation with some behavioural tests.
3. The methods may come and go on a whim and signatures may change without notice.

Having to explicitly add namespaces to my custom modules is a minor inconvenience and given the built in support in VS2017 and tooling like R# etc, this is very much a passing thought these days - IMHO of course :)

[TylerCarlson1]

I think the argument against this was you could be using these functions outside of AutoMapper or IObjectMapper instance and not realize it was tied to AM instead of .NET.

As the creator of AutoMapper.Collections I had to go through the same thing copying code that was previously exposed as public. I also see this as slightly annoying and with R# I can easily copy and paste it.

As a person who helps support AutoMapper, I see it as IObjectMapper being an advance feature and to use it you are going to have to go dig around through code anyways to find what you want. I think since it's open source it's better to have internal because advanced users who want IObjectMapper can get access to it.

Yea it's not ideal to get internal function calls, but if you are someone who wants to re-use it you should be skilled enough to find a work around to make it work.

[jbogard]

So originally these were exposed publicly. Then people complained when I removed extension methods as I wasn't using them anymore.

I'm OK with an internal namespace, though, with the expectation that those could be removed/broken without needing a major version bump. Especially for the extension methods that I really don't want polluting the "public" API.

[lbargaoanu]

@SaltyDH PR?

[matt-psaltis]

Happy to, we've got an imminent arrival in the household which will put me out of action for a few weeks. Any other candidate classes I should consider?

[lbargaoanu]

I'd say just what you need.

[matt-psaltis]

Here's my first iteration on this. As discussed, I've kept extension methods as internal visibility and introduced Internal namespaces (AutoMapper.Internal and AutoMapper.Mappers.Internal) to expose the underlying functionality.

Testing this against the original use case, my modifications are reduced. I replace the calls to Extension methods with the underlying methods in the Internal namespace. So whilst not a copy/paste, its pretty close.

Thoughts, feedback, dislikes - all welcome!

https://github.com/SaltyDH/AutoMapper/commit/c267f47f148e096b9d8f21d34576684979582bab
https://github.com/SaltyDH/AutoMapper/commit/4dd8597ca0e7e15d6009c576852b26535fe59f59

Unit tests all passed except for 2 in AutoMapper.UnitTests.Projection which seem to use an actual DbContext?

[lbargaoanu]

The diff is kind of messed up. Not necessarily your fault, but it would be nice if you could clean it up :)
I don't see much value in hiding the extension methods when you have an internal namespace, but I know that's what @jbogard wanted.
The tests have to pass :) Only integration tests use a real db, MSLocalDB. They might fail locally but they should work in a PR.

[matt-psaltis]

I see what you mean about the diff, I suspect its a line feed difference that has caused that. The diff didn't look anything like that in VS2017 when I committed it. I can clean that up.

As a R# user, I think its much easier to accidentally import an extension method from AutoMapper if we expose these. I can see how there might have been prior feedback around Extension method pollution.

I understand that tests will have to pass and I plan to setup a local environment so that integration tests can run. Can you please review QueryableInterfaceInheritanceIssue.cs its in the UnitTests namespace but uses a real database. That's all I was trying to communicate.

[matt-psaltis]

Yep, ended up being the EOL format.

Here's a new commit.

https://github.com/SaltyDH/AutoMapper/commit/b64ec498e6777b885c6f7d1634fcfb84ff4a086d

I was able to successfully run all of the Unit and Integration Tests against this.

[lbargaoanu]

LGTM. PrimitiveExtensions has also useful stuff but I guess you didn't need it.

[matt-psaltis]

I added PrimitiveExtensions using the same approach.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.