Skip to content

Avoid static state; Mapper registry managed through mapper configuration #1848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 19, 2016
Merged

Avoid static state; Mapper registry managed through mapper configuration #1848

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
353f7b2
avoid statics
lbargaoanu Dec 9, 2016
8c4851f
added a test
lbargaoanu Dec 12, 2016
f3d25f2
Exposing mapper list through the overall mapper configuration
jbogard Dec 16, 2016
3659ca9
Merge pull request #5 from AutoMapper/lbargaoanu-RegistryMapper
lbargaoanu Dec 19, 2016
56206ff
make Mappers a method and hide MapperRegistry
lbargaoanu Dec 19, 2016
63033d2
we need a list
lbargaoanu Dec 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/AutoMapper/MapperConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class MapperConfiguration : IConfigurationProvider
private readonly ConfigurationValidator _validator;

public MapperConfiguration(MapperConfigurationExpression configurationExpression)
: this(configurationExpression, MapperRegistry.Mappers)
: this(configurationExpression, MapperRegistry.Mappers())
{
}

Expand All @@ -43,7 +43,7 @@ public MapperConfiguration(MapperConfigurationExpression configurationExpression
Seal();
}

public MapperConfiguration(Action<IMapperConfigurationExpression> configure) : this(configure, MapperRegistry.Mappers)
public MapperConfiguration(Action<IMapperConfigurationExpression> configure) : this(configure, MapperRegistry.Mappers())
{
}

Expand Down
18 changes: 1 addition & 17 deletions src/AutoMapper/Mappers/MapperRegistry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace AutoMapper.Mappers
{
public static class MapperRegistry
{
private static readonly IObjectMapper[] _initialMappers =
public static IObjectMapper[] Mappers() => new IObjectMapper[]
{
Copy link
Contributor

@jbogard jbogard Dec 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird method. I wonder if we could just expose the property still, but make it very explicit of an IReadOnlyCollection? What would be the tradeoff?

Copy link
Contributor

@jbogard jbogard Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a breaking change to the API. Is there a way to preserve the property but make this thread-safe?

Copy link
Contributor

@jbogard jbogard Dec 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbargaoanu sorry did you see these questions?

Copy link
Contributor Author

@lbargaoanu lbargaoanu Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, didn't see them :)
It doesn't help to make it ReadOnlyCollection, it's still a breaking change. The only way I can think of to keep it is through ThreadStatic. But that doesn't really make sense if it's exposed through config. It's breaking, but minor, few people use mappers I would say.

Copy link
Contributor

@jbogard jbogard Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! I was trying out the Review feature in GitHub. Honestly don't understand the point of it.

Copy link
Contributor Author

@lbargaoanu lbargaoanu Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither do I :)

new NullableSourceMapper(),
new ExpressionMapper(),
Expand Down Expand Up @@ -34,21 +34,5 @@ public static class MapperRegistry
new FromDynamicMapper(),
new ToDynamicMapper()
};

private static readonly List<IObjectMapper> _mappers = new List<IObjectMapper>(_initialMappers);

/// <summary>
/// Extension point for modifying list of object mappers
/// </summary>
public static IList<IObjectMapper> Mappers => _mappers;

/// <summary>
/// Reset mapper registry to built-in values
/// </summary>
public static void Reset()
{
_mappers.Clear();
_mappers.AddRange(_initialMappers);
}
}
}
11 changes: 3 additions & 8 deletions src/UnitTests/Mappers/CustomMapperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,11 @@ namespace AutoMapper.UnitTests.Mappers
{
public class When_adding_a_custom_mapper : NonValidatingSpecBase
{
public When_adding_a_custom_mapper()
{
MapperRegistry.Mappers.Insert(0, new TestObjectMapper());
}

protected override MapperConfiguration Configuration { get; } = new MapperConfiguration(cfg =>
{
cfg.CreateMap<ClassA, ClassB>()
.ForMember(dest => dest.Destination, opt => opt.MapFrom(src => src.Source));
});
}, MapperRegistry.Mappers().Concat(new[] { new TestObjectMapper() }));

[Fact]
public void Should_have_valid_configuration()
Expand Down Expand Up @@ -78,7 +73,7 @@ public class When_adding_a_simple_custom_mapper : AutoMapperSpecBase
{
cfg.CreateMap<ClassA, ClassB>()
.ForMember(dest => dest.Destination, opt => opt.MapFrom(src => src.Source));
}, MapperRegistry.Mappers.Concat(new[] { new TestObjectMapper() }));
}, MapperRegistry.Mappers().Concat(new[] { new TestObjectMapper() }));

protected override void Because_of()
{
Expand Down Expand Up @@ -162,7 +157,7 @@ public override string Map(object source, string destination, ResolutionContext
protected override MapperConfiguration Configuration => new MapperConfiguration(cfg =>
{
cfg.CreateMap<Source, Destination>();
}, new[] { new EnumMapper() }.Concat(MapperRegistry.Mappers));
}, new[] { new EnumMapper() }.Concat(MapperRegistry.Mappers()));

protected override void Because_of()
{
Expand Down