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

Erroneous or unnecessary restriction on specific object mappers #20820

Closed
1 task done
nivedano opened this issue Sep 18, 2024 · 2 comments · Fixed by #20829
Closed
1 task done

Erroneous or unnecessary restriction on specific object mappers #20820

nivedano opened this issue Sep 18, 2024 · 2 comments · Fixed by #20829
Assignees
Milestone

Comments

@nivedano
Copy link

nivedano commented Sep 18, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Description

Currently, DefaultObjectMapper.TryToMapCollection searches only public methods of a specific object mapper via GetMethods() with no args:

return specificMapper
      .GetType()
      .GetMethods()
      .First(x =>
          x.Name == nameof(IObjectMapper<object, object>.Map) &&
          x.GetParameters().Length == (destination == null ? 1 : 2)
      );

This call only returns public methods, omitting explicit interface realizations. Such a restriction leads to rather rather inefficient designs, when you need to implement a bunch of related mappings with the same source type, but IObjectMapper<TSource, TDestination>.Map(TSource) definitions will collide on the mapper type if you try to implement the interfaces in a regular way.

Also, .First method, when no map found throws an unspecific exception, which tells nothing essential about what is wrong.

Reproduction Steps

No response

Expected behavior

  1. Change the line from .GetMethods() to .GetMethods(System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public) to allow explicit interface implementations for mappers. Plus parameter matching logic.
  2. Change .First(...) for more descriptive error explanation, like this .FirstOrDefault(...) ?? throw new InvaildOperationException($"No specific mapper method found on type {mapperType.FullName}") or better

Actual behavior

No response

Regression?

No response

Known Workarounds

No response

Version

8.2.2

User Interface

Common (Default)

Database Provider

EF Core (Default)

Tiered or separate authentication server

None (Default)

Operation System

Windows (Default)

Other information

No response

@maliming
Copy link
Member

Is this a problem? How to reproduce?

@nivedano
Copy link
Author

nivedano commented Sep 18, 2024

Is this a problem? How to reproduce?

Yes, this is a design problem.

class ADto {}
class AWithDetailsDto {}

class ABMapper: 
    // You can't have these both implemented via public methods, Map() methods collide.
    // You've got to spawn the classes. It got the more complicated, the more complex logic is associated with the mappings
    IObjectMapper<A, ADto>, 
    IObjectMapper<A, AWithDetaildDto>,
    ITransientDependency
{
    // you can compile this, but it won't work with the current DefaultObjectMapper.TryToMapCollections
    ADto IObjectMapper<A, ADto>.Map(A source) => ...;
    AWithDetailsDto IObjectMapper<A, AWithDetailsDto>.Map(A source) => ...;
}

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

Successfully merging a pull request may close this issue.

3 participants