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

Support for Type mapping. #1802

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

olivier-spinelli
Copy link
Contributor

This adds support for Type mapping, typically resolving the issue #1104 by allowing a requested type to be mapped to another type. The core of this PR is rather light since everything is done in the TypeDeserializerCache:

When a type is new to the cache, instead of considering only the requested type, a mapping function is called and if it provides a type, both the requested and mapped types are cached.

Currently, no check is done (with IsAssignableFrom) to enforce the relationship between the requested and mapped types
but this can easily be done. Not sure it's useful though...

No check is done with IsAssignableFrom to enforce
the relationship between the source and target types
but this can easily be done.
Not sure it's useful...
olivier-spinelli added a commit to Invenietis/CK-SqlServer-Dapper that referenced this pull request Jul 11, 2022
This version implements the issue: DapperLib/Dapper#1802
@mgravell
Copy link
Member

mgravell commented Oct 3, 2023

I think we need to check, here, what the actual feature is that we're enabling - the "why", not the "how". Only then can we assess whether this is an appropriate implementation of that "why". You cite #1104, but a key part of that is the generation aspect; that seems like something we could now do very easily over in "AOT". Config APIs should usually be a last resort - what is the common scenario where this would be used, and can we make that easier? Would static factory methods on the interface (where allowed by the runtime) be an option?

Can you elaborate on the perceived main use case for the feature?

@olivier-spinelli
Copy link
Contributor Author

Hello,

Glad that this PR caught the attention!
The "why" is rather basic: I have a IThing at compile time, only at runtime do I have a Thing class that implements the interface. This PR simply enables such mapping to be known and handled by Dapper.

Whether such mapping can be triggered by an automagic pattern is always possible (including code generation) and I fully agree that when AOT generation is possible, this configuration may be done "more genuinely". However, in my case, the implementation class is not generated in the project/package that declares the IThing but by the final application (IThing is unified with other ISpecializedThing : IThing that can exist in other - orthogonal - packages).

This is one of the reason "why" I choose this implementation. Another reason was that I tried to follow the current design principles of Dapper and tries to minimize the impact as much as possible (and I think that the result is bearable), avoiding reflection/discovery/assembly scans and without platform restrictions... We use this for years now without any issues.

I've not yet explored the C#11 "static interface methods". It seems to me that the "strange recurring pattern" (the Interface<TSelf>) would unfortunately be required on the top of my hierarchy of types (named IPoco) to have the static abstract TSelf CreateInstance() be correctly typed. But... since Dapper uses reflection afterwards, it seems that a static abstract IPoco CreateInstance() may do the job. I've not yet put a lot of thought on this feature... However here, it may well be an alternative to the configuration way...

@mgravell
Copy link
Member

mgravell commented Oct 3, 2023

Can you elaborate on the "only at runtime"? where is the concrete implementation coming from? can it ever be statically known? if not, then indeed we have fewer options

@olivier-spinelli
Copy link
Contributor Author

olivier-spinelli commented Oct 3, 2023

Okay... This may appear surprising but I'll try to explain below:

  • An abstract package defines a public interface IPoco {}.
  • The package UserManagement defines a public interface IUserInfo : IPoco { string Name {get;set;} }.
    This defines a "family", the root type of a value type (in the DDD sense, not the runtime): other packages can then extend this definition.
  • The package 'UserHasColor' depends on UserManagement and defines public interface IUserWithColor : IUserInfo { int Color {get;set;} }.
  • The package 'UserHasFirstName' depends on UserManagement and defines public interface IUserInfo : UserManagement.IUserInfo { string FirstName {get;set;} } (in its UserHasFirstName namespace, this why it can reuse the same type name).

Now, a developper create a project for our AcmeCorp client. She needs user with color and first name: she installs both packages.
In this project, a post-build step analyzes the dependencies and creates an implementation for the IUserInfo family:

public sealed class IUserInfo_CK: UserManagement.IUserInfo, UserHasColr.IUserWithColor, UserHasFirstName.IUserInfo
{
   public string Name {get;set;}
   public int Color {get;set;} 
   public string FirstName {get;set;} 
}

This code is compiled in an assembly (Roslyn) and/or the generated source code is copied in a target "host" project. To create an instance of such type that nobody knows, a factory is also generated and injected in the DI. This is today.
May be tomorrow, IPoco will have a static abstract IPoco CreateInstance() and the IUserInfo_CK will implement the IPoco CreateInstance() => new IUserInfo_CK();.... and using the factory from the DI would become optional.

I hope this is not too much explanations and they are understandable. This framework is not "in the mainstream" at all... but it's really cool!

@olivier-spinelli
Copy link
Contributor Author

BTW, I just realized that you may have addressed this bug: #1822 because I see now a wrapper around the reader (but the issue is still opened).

I also fixed it here: #1824
The explanation is here: #1822 (comment).
This fix doesn' use systematically a wrapper (note that the GetSameReaderForSameShape test - and its invariant - was preserved).

@mgravell
Copy link
Member

mgravell commented Oct 3, 2023

Right; so - separately, I'm having a conversation about allowing factory methods; at the moment we support constructors only, but there is a good case for factories, and there is a PR in AOT that adds support. That prompted the question: should Dapper (vanilla, not AOT) allow factories? I wonder if this is an example of where a factory might be ideal; consider the following:

public interface IFoo // your POCO... POIO?
{
    [SomeAttribute] // from Dapper, TBA
    public static IFoo Create()
    {
        // your logic here; your own project-specific
        // type registry/lookup/whatever
        return new DummyFoo();
    }
}

// just for the example
class DummyFoo : IFoo { }

If Dapper recognized this attribute, everything works; a quick test shows that this is usable:

var factory = (from method in typeof(IFoo).GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static)
               where method.IsStatic && !method.IsAbstract && Attribute.IsDefined(method, typeof(SomeAttributeAttribute))
               select method).SingleOrDefault();

if (factory is not null)
{
    var obj = factory.Invoke(null, null);
    Console.WriteLine("Created: " + obj);
}

This seems a lot more flexible, and is less impact to Dapper - and it simply reuses a feature we were talking about adding anyway. Thoughts?

Note: this static method usage on interfaces is framework dependent; it needs .NET 5 or later, and will not work on .NET Framework.

@mgravell
Copy link
Member

mgravell commented Oct 3, 2023

the downside is that it makes an IFoo.Create method available more generally, but: it can be private

@olivier-spinelli
Copy link
Contributor Author

olivier-spinelli commented Oct 3, 2023

I understand the gain. What annoys me is the attribute (not all IPoco needs to be database aware but this concern is not necessarily known WHERE the Poco is defined).

Question: Can a static named CreateInstance() that returns a type that is assignable from the declaring type be considered an implicit candidate IIF no ctor exist (when the declaring type is a class)?

The assignable from here is crucial (for me): this abstract factory will be defined at the top of the hierarchy. Only the ultimate abstraction is available. By design it WILL return an instance that is assignable to the actual type, but this cannot be asserted at the type system level.

In other words, static object CreateInstance() is a valid factory... This seems weird, I know, but this is the key to a truly modular system in which parts of a system interacts with types that can be refined by others: you cannot guaranty type closure "right now", you rely on the fact that types will be "eventually satisfied".

Edit: Currently, the DI hides the trick (the factory methods are totally Type Safe). Ideally, the object CreateInstance() will be "hidden behind" a code generated (Roslyn Generator) typed extension method that returns the declaring type: for the developer, everything is type safe.

Update: typos (sorry)

@mgravell
Copy link
Member

mgravell commented Oct 4, 2023

By design it WILL return an instance that is assignable to the actual type, but this cannot be asserted at the type system level.

Why can't it return the abstract type? In what valid scenario would it be returning something that isn't at least that type? Can you provide an example?

@mgravell
Copy link
Member

mgravell commented Oct 4, 2023

implementation note: there's also static interface methods as an alternative design idea:

interface IBar : IFactory<IBar>
{
    static IBar IFactory<IBar>.CreateInstance() => default!; // your code here
}

public interface IFactory<T>
{
    abstract static T CreateInstance();
}

This presumably has the same problems as the attribute, to you?

@olivier-spinelli
Copy link
Contributor Author

olivier-spinelli commented Oct 4, 2023

It will be typed (in my case) to return the most abstract type (the IPoco hierarchy root), but the actual runtime type will necessarily be a type that is assignable to any interface that extends the interface family.

I detail below the context quickly described previously.

  • Abstraction package:
public interface IPoco
{
     public static abstract IPoco CreateInstance();
}
  • UserManagement package (depends on Abstraction). It defines the IUserInfo family:
public interface IUserInfo : IPoco
{
     public string Name { get; set; }
}
  • UserHasColor package (depends on UserManagement):
public interface IUserWithColor : IUserInfo
{
     public string Name { get; set; }
}
  • UserHasFirstName package (also depends on UserManagement but doesn't know UserWithColor at all):
namespace UserHasFirstName;
public interface IUserInfo : UserManagement.IUserInfo
{
     public string FirstName { get; set; }
}
  • In AcmeCorp users must have a color and a first name: the project depends on UserHasColor and
    UserHasFirstName.
    The "Post Code Generation" generates an implementation that unifies the family (there is much more automatic code of
    course in real life, for instance non nullable fields, collections, etc. are recursively initialized):
public sealed class IUserInfo_CK: UserManagement.IUserInfo, UserHasColr.IUserWithColor, UserHasFirstName.IUserInfo
{
  public string Name {get;set;}
  public int Color {get;set;} 
  public string FirstName {get;set;} 
}

This (awful) class is out of reach from any user code.

Any code, in any package, can always safely require the IPocoFactory<IUserInfo>, or IPocoFactory<IUserWithColor> or any member of the family that is known at its level from the DI (the generated factory implementation is a singleton that works for all the members of the family):

var uInfo = services.GetRequiredService<IPocoFactory<IUserWithColor>>().Create();
// or from the PocoDirectory.
var uInfo = services.GetRequiredService<PocoDirectory>().Create<IUserWithColor>();

The developper focuses on its Poco, there is absolutely no boilerplate. This example introduces only 2 family members. In practice much more refinements extensions exist.

That being said, back to your interrogation:

The IFactory<T> would be needed in the user code, and repeated for each member of the family (can we call it "TSelf pollution"?).
Each typed CreateInstance() would have to be generated (this is easlity doable) but today, only one exposed generic
methods is required that simply casts the object in T : where T : class, IPoco.

I totally understand your concern. Note that I'v always considered the design I propose as a low level, advanced, feature. This is why I exposed the thread safe "combiner" as the only API: this is not inteded to be used by mundane developers but by architects/lead devs who understand what they do :). In my mind, this would have been used by the infrastructure of a code base (just like I do), not by everyone (how much Dapper users configure the SqlMapper mapping capabilities?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants