Skip to content
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.

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

Mapper seems to ignore a configured data-source #146

Closed
bslbckr opened this issue Jun 14, 2019 · 6 comments
Closed

Mapper seems to ignore a configured data-source #146

bslbckr opened this issue Jun 14, 2019 · 6 comments
Assignees
Labels

Comments

@bslbckr
Copy link

bslbckr commented Jun 14, 2019

Hi,

I have a, at first sight, pretty easy example:

namespace Source {
  public interface IData {
	  string Id {get; set;}
  }
  public interface IEmpty : IData {}
	public class Data : IEmpty {
		public string Id {get; set; }
	}
	
	public class Container {
		private readonly IEmpty _empty;
		public Container() {
			_empty = new Data{Id = "12345"};
		}
		public string Name {get; set;}
		public IEmpty Empty {get { return _empty;}}
	}
}

namespace Target {
	public class Data {
		public string Id {get;set;}
		public string Value {get;set;}
	}
	
	public class Cont {
		public Data Info {get; set;}
		public string Name {get; set;}
	}
}

the mapping configuration looks as follows:

Mapper.WhenMapping.From<Source.Container>().To<Target.Cont>().Map(src => src.Source.Empty).To( tgt => tgt.Info);

the corresponding mapping plan looks like:

// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
// Map Container -> Cont
// Rule Set: CreateNew
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 

cToCData =>
{
    try
    {
        var cont = new Cont();
        cont.Info = cont.Info;
        cont.Name = cToCData.Source.Name;

        return cont;
    }
    catch (Exception ex)
    {
        throw MappingException.For(
            "CreateNew",
            "Container",
            "Cont",
            ex);
    }
}

My expectation is that the mapper maps Source.Container.Empty to Target.Cont.Info

Further, I only build this small example as I had the impression, that inheritance structure in the Source namespace confuses the mapper, such that it doesn't recognize the IEmpty actually has a property named Id.

As usual I have a corresponding fiddle: https://dotnetfiddle.net/6AupKd

SteveWilkes added a commit that referenced this issue Jun 15, 2019
* Taking a source interface's implemented interfaces into account when matching source members

* Test coverage for configured interface source member
@SteveWilkes SteveWilkes self-assigned this Jun 15, 2019
@SteveWilkes
Copy link
Member

Thanks!

There was a bug here with the mapper not checking for a source interface member's implemented interface members (there's a mouthful) - it's fixed in the latest 1.5 release code, release to follow.

If Source.Data - but not Source.IEmpty or Source.IData - had a Value member, would you expect Target.Value to be populated? The mapper doesn't currently check an interface member's implementation type to look for data sources, but it could...

Thanks again :)

@SteveWilkes
Copy link
Member

This is fixed as of v1.5 preview 2, which is now on NuGet.

The mapper still uses the declared type of an interface source member instead of checking the implementation type - I'd be interested to hear your thoughts on that!

@SteveWilkes SteveWilkes added the in-preview A feature or bug fix exists in a preview release, and a full release will follow label Jun 16, 2019
@bslbckr
Copy link
Author

bslbckr commented Jun 16, 2019

This is fixed as of v1.5 preview 2, which is now on NuGet.

That's great news. We will give it a try the next days.

The mapper still uses the declared type of an interface source member instead of checking the implementation type - I'd be interested to hear your thoughts on that!

I really struggle giving a concrete answer to this question. The mapper's overall experience is strongly typed what gives the benefit of cached and compiled execution plans and assumably speed up the overall mapping performance. So this is a strong point towards keeping it, as it is. However, I also could imagine situations where the expectation of users could be that the implementation type should be considered. I fail to weigh both points against each other and don't want to give the lame and obvious answer: make it configurable :)

However, what I found out being quite problematic at the moment is a slightly different situation: Assuming you have the following source structure

interface MyInterface {
  int Id {get; set; }
}
class ImplementationA : MyInterface {
  public int Id {get; set: }
  public string FancyMember {get; set;}
}
class ImplementationB : MyInterface {
  public int Id {get; set: }
  public bool IsFancy { get; set;}
}

class Container {
  public MyInterface Foo {get; set;}
}

and you want to map it to a pretty simple target structure:

enum Implementation {
   A,
   B
}

class Interface {
   public Implementation {get;set;}
   publi int Id { get; set;}
}

class Container {
  public Interface Foo {get;set;}
}

There is no way to specify different mapping configurations for the two possible implementation types. It would be great, if there were a generic conditional, i.e. If<ImplementationA>().Map(Implementation.A).To(tgt => tgt.Impl). the existing Map<ImplemenationA> could not be re-used for this purpose. Probably this is a completely new enhancement.

the usual link: https://dotnetfiddle.net/z3X3de

@SteveWilkes
Copy link
Member

Interesting! I might make implementation-type mapping configurable...

You can configure the second example like this:

    Mapper.WhenMapping
        .To<Target.Interface>()
	.If(ctx => ctx.Source.GetType() == typeof(Source.ImplementationA))
	.Map(Target.Implementation.A).To(tgt => tgt.Impl)
	.But
        .If(ctx => ctx.Source.GetType() == typeof(Source.ImplementationB))		 
        .Map(Target.Implementation.B).To(tgt => tgt.Impl);

...but it would be smoother to have the mapper perform a map-time check on the source object. It also currently throws an exception if you use an is or as test in an .If() condition, but I should probably allow that if it's mapping from an interface:

    Mapper.WhenMapping
        .From<Source.MyInterface>().To<Target.Interface>()
	.If(ctx => ctx.Source is Source.ImplementationA)
	.Map(Target.Implementation.A).To(tgt => tgt.Impl)
	.But
        .If(ctx => ctx.Source is Source.ImplementationB)
        .Map(Target.Implementation.B).To(tgt => tgt.Impl);

Thanks again for the feedback!

@bslbckr
Copy link
Author

bslbckr commented Jun 17, 2019

It also currently throws an exception if you use an is or as test in an .If() condition,

we already noted that ;-)
My idea with the special form of the .If() condition was to not confuse users if the mapper sometimes allows it and sometimes it throws an exception.

@SteveWilkes
Copy link
Member

I've updated the latest 1.5 release code to allow type tests when the source type is an interface, but I want to encourage using .From<TSource>() where possible (hence the exception) as it allows the mapper to create better mapping functions.

We could have .IfSourceIs<TSource>(), but I'm not sure about adding to the configuration API for what feels like a fairly edge case...

SteveWilkes added a commit that referenced this issue Sep 14, 2019
* Bugs/unused constructor members (#140)

* Adding intermediate-step ConstructionInfos to determine constructability without always creating the construction Expression

* Checking object constructability using ConstructionInfos

* Tidying

* - Populating members with a matching constructor parameter that might not be used
- Respecting single-data-source population conditions for readonly member populations

* Fixing mapping validation detection of unconstructable members

* Updating version numbers

* Features/root enum pairing (#141)

* Support for root enum pairing

* Test coverage for nullable target enums and out-of-range source enum values

* Test coverage for root enum mapping

* Support for mapping callbacks around root enum mapping

* Support for mapping (not projecting) queryables (#142)

* v1.5 preview 1 release notes and NuGet package

* Features/entity key mapping refinements (#144)

* Always allowing entity key mapping in projections

* Allowing entity key mapping for view models and DTOs

* Handling null nested members in .ToTarget() data sources, fixes #145

* Bugs/issue146 (#146, #147)

* Taking a source interface's implemented interfaces into account when matching source members

* Test coverage for configured interface source member

* Mapping to target interface members' implemented interface members, re: #146

* Features/coverage optimisations (#148)

* Removing unused dictionary member population code and optimising

* QueryString class test coverage

* Optimising DictionaryTargetMember.TryGetMappingBody, IList.Append(IList) and MappingFactory.UseLocalValueVariable

* Tidying

* Removing unused code

* Test coverage for ExpressionInfoFinder index logic

* Extending array index data source code coverage

* Extending test coverage for index expression data sources

* Using instance mapper

* Updating to v2.3.1 of ReadableExpressions
Removing Index tests from .NET 3.5 tests

* Adding coverage TODOs

* Increasing data source reversal test coverage / Lazy-loading ExpressionInfoFinder collection members

* Fixing reverse data source test coverage

* Increasing code coverage / Removing unused or obselete code

* Updating to v1.5-preview2

* v1.5-preview 2 NuGet package

* Allowing configured condition type tests if mapping from an interface, re: #146

* Support for custom DateTime format strings with nullable DateTimes, re #149 (#150)

* Accounting for a conflicting member check being checked against QualifedMember.All - fixes #152

* Support for zero-configuration mapping of interfaces to strings, fixes #153

* Updating to preview3, updating release notes, adding NuGet package

* Bugs/simple to complex to target (#154)

* ToTarget changes

* Tidying

* Removing Newtonsoft dependency

* Optimising deep clone member-matching

* Support for simple type to enumerable ToTarget data sources

* Tidying

* Fixing tests

* Features/tidying (#156)

* Updating ReadableExpressions to v2.3.2

* Tidying

* Moving member population logic into MemberPopulator from DataSourceSet and MemberPopulationFactory

* Optimising Array Prepend + Append calls

* Organising fallback data sources

* Moving logic into EnumerableTypeHelper

* Avoiding yielding null expressions from population factory

* Tidying

* Tidying

* Features/data source improvements (#158)

* Renames

* Creating DataSourceSets to generate root mapping Expressions

* Removing unused MappingExpressionFactory code

* Removing ExpressionFactory singleton instances

* Switching DataSourceFactory to a delegate

* Adding ComplexTypeDataSource factory methods

* Start of generating derived type mappings via data sources

* Rearranging derived type mapping generation

* Adding condition-or-default helper method

* Handling unconditional derived type mappings

* Detecting derived new element mappings

* Fixing derived type mapping, all tests pass!

* Organising data source factory files

* Features/ignore data sources (#160)

* Support for ignoring a given source member
Tidying

* Reusing created variable value

* Switching single-method interfaces to delegates

* Features/member population context (#159)

* Reusing created variable value

* Switching single-method interfaces to delegates

* Adding MemberPopulationContext

* Avoiding capture creation in .Project()

* Avoiding capture creation in .Filter()

* Avoiding closure creation in .FirstOrDefault()

* Avoiding closure creation in .First()

* Reusing MemberPopulationContext and DataSourceFindContext throughout complex type member population creation

* Start of SourceMemberMatchContext
Tidying

* Adding reuseable SourceMemberMatchContext

* Tidying

* Support for conditional member ignores

* Erroring if redundant source ignore configured

* Support for all-target source member ignores / Test coverage for ignoring a write-only source member

* Invalid configuration test coverage

* Fixing configured source member ignore conflict detection

* Extra test coverage

* Extending test coverage

* Support for extending source ignores in inline configuration

* Test coverage for incorrect inline source ignore configuration

* Start of support for ignoring source members by filter

* Support for ignoring source members by filter

* Erroring if invalid source member filters are specified

* Extending test coverage for global source filters

* Support for maptime ignoring of source members by derived type

* Splitting source member ignore classes

* Splitting target member ignore classes

* Renaming ignore classes

* Moving shared member ignore logic into interface extension methods

* Optimising single-value DataSourceSets
Renames

* Basic source value filter support

* Support for global object-value filters

* Test coverage for multi-clause global typed source value filter

* Extending test coverage

* Renames

* Support for filtering simple type source enumerable element values

* Extending test coverage

* Test coverage for filtering a simple type configured data source

* Updating test

* Not falling back to a default data source if a configured data source has no default matching source member

* Optimising source value filter creation

* Tidying

* Support for maptime-checked derived source type filters

* Test coverage for source value filter-ignored runtime typed derived sources

* Extending test coverage

* Support for inline-configured source value filters and overlapping multiple source value filters

* Removing duplicate filter application / Start of handling null nested members in value filter expressions

* Support for nested access checking in source value filters

* Support for combining conditional data sources with inline source value filters

* Erroring if duplicate source value filters are defined

* Erroring if empty source filter configured

* Extra test coverage

* Support for conditional source value filters using the If() syntax

* Organising source value filter classes

* Adding / updating documentation

* Updating release notes

* Updating to v1.5

* Adding v1.5 NuGet package
@SteveWilkes SteveWilkes removed the in-preview A feature or bug fix exists in a preview release, and a full release will follow label Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants