Skip to content
New issue

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

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

Already on GitHub? # to your account

IQueryable doesn't work with non parameterless ctors #972

Closed
arkchor opened this issue Dec 4, 2023 · 10 comments · Fixed by #1112
Closed

IQueryable doesn't work with non parameterless ctors #972

arkchor opened this issue Dec 4, 2023 · 10 comments · Fixed by #1112
Assignees
Labels
bug Something isn't working

Comments

@arkchor
Copy link

arkchor commented Dec 4, 2023

Describe the bug
It's impossible to generate mapping for IQueryable<T> when there is a non nullable, non parameterless ctor type in mapped object.
Project with such code has compilation error.
Error in such case is: Error RMG002 : {SomeType} has no accessible parameterless constructor.

To Reproduce
Steps to reproduce the behavior:

  1. Prepare some value object
    public record ValueObject(string Value);
  2. Prepare entity to map from
public class TestEntity
{
    public ValueObject ValueObject { get; set; }
}
  1. Prepare dto to map to
    public record TestDto(ValueObject ValueObject);
  2. Prepare IQueryable<T> mapper
[Mapper]
public static partial class TestMapper
{
    public static partial IQueryable<TestDto> SelectDto(this IQueryable<TestEntity> test);
}
  1. Build

Expected behavior

  • Build should pass
  • SelectDto should properly perform mapping when used

Code snippets
Generated mapping seems to be fine:

    public static partial class TestMapper
    {
        public static partial global::System.Linq.IQueryable<global::Test.TestDto> SelectDto(this global::System.Linq.IQueryable<global::Test.TestEntity> test)
        {
#nullable disable
            return System.Linq.Queryable.Select(test, x => new global::Test.TestDto(x.ValueObject));
#nullable enable
        }
    }

Environment (please complete the following information):

  • Mapperly Version: 3.3.0-next.3
  • .NET Version: 8
  • Target Framework: net8.0
  • IDE: Rider 2023.2.3
  • OS: Windows

Additional context
I found a workaround for the problem. It works when we make the type in dto nullable.
So instead of this:
public record TestDto(ValueObject ValueObject);
Make this:
public record TestDto(ValueObject? ValueObject);

I'm very much into DDD and always choose tooling that gives ability to preserve fundamental concepts of DDD.
All above is just example but in real cases I'm implementing I don't want to introduce false statement that something is nullable when in real domain it isn't and should be always there.

Also I would appreciate if somebody could explain me why such error ever happen? What internally leads to the necessity of having parameterless ctors on things when the actual generated code even doesn't need this?
There are more issues related with this (see below) and also I wasn't able to generate mappings for common stuff like IReadOnlyList, IReadOnlyDictionary with same no accessible parameterless constructor error but I don't elaborate on this since somehow I believe the reasoning behind could be same for all cases. But still it's strange for me I can't do it for example for list when below you can see similar issue exactly related to list interfaces that has been closed. I want to better understand internals or maybe there is some common factor across these issues that can be fundamentally solved in design?

Related issues

@latonz
Copy link
Contributor

latonz commented Dec 4, 2023

Thanks for opening this issue. I'm not sure if I get all of it correctly, can you perhaps create a minimal replication repository on GitHub? You mention a nullable constructor parameter works around the issue; are all (the mapper, entity, dto and value object) in the same nullability context? The related issues you listed were for types for which Mapperly couldn't generate an automatic mapping implementation at the time, but these are not related to nullability.

@latonz
Copy link
Contributor

latonz commented Dec 15, 2023

Closing as no update is received, feel free to reopen...

@latonz latonz closed this as not planned Won't fix, can't repro, duplicate, stale Dec 15, 2023
@pawel-pajak-espeo
Copy link

Here we have a minimal reproduction repo. What is worth noting, removing "ProjectToDto" method makes it all work alright, so the problem must be related to the IQueryable.
Also, worth noting, making the InnerRecord in the DTO nullable, also "fixes" the problem.
public record Dto(int Id, InnerRecord? InnerRecord);

https://github.com/pawel-pajak-espeo/MapperlyNullableProblemMinimalSetup

@arkchor
Copy link
Author

arkchor commented Feb 8, 2024

@latonz Can you reopen the issue? @pawel-pajak-espeo cooperates with me regarding this problem because we're working in the same codebase where the problem was discovered.

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Feb 9, 2024

Thanks for the repo 👍 Looks like the error InnerRecord has no accessible parameterless constructor. is produced.

Not sure why this happens, especially when the generated code looks fine, here's my guess:

In IQueryable mappings, Mapperly assumes that the source values are potentially nullable. So in our case Entity.InnerRecord is assumed to have the type InnerRecord?.
As the constructor for Dto accepts a non nullable InnerRecord Mapperly will attempt to generate the following code to handle the potential null ness of InnerRecord.

return System.Linq.Queryable.Select(form, x => new Dto(x.InnerRecord ?? new InnerRecord()));

As InnerRecord does not have a parameterless constructor Mapperly settles on passing the potentially null value into the constructor and emits a warning message.

The non Queryable mapping doesn't treat source values as nullable so it doesn't have this issue. Equally making Dto accept a nullable InnerRecord removes the need to check the source value for null ness.

I'm not sure if this is a bug or expected behaviour.

@arkchor
Copy link
Author

arkchor commented Feb 12, 2024

@TimothyMakkison that makes sense. For me it seems like bug. How can I reopen this issue?

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Feb 12, 2024

@TimothyMakkison that makes sense. For me it seems like bug.

Yeah, It feels super unintuitive.

How can I reopen this issue?

I think dont think github lets people reopen issues. You'll have to wait for @latonz or one of the Riok guys to reopen this.

There are more issues related with this (see below) and also I wasn't able to generate mappings for common stuff like IReadOnlyList, IReadOnlyDictionary with same no accessible parameterless

Thanks for mentioning this, it has the same cause but I think it can be fixed. I'll look into adding a special case where new List<T> etc is emitted.

@latonz
Copy link
Contributor

latonz commented Feb 12, 2024

Thanks for the repro, the bugfix is on the way and should land in the upcoming next release.

@latonz
Copy link
Contributor

latonz commented Feb 12, 2024

@TimothyMakkison You mention a problem with `List', I couldn't find anything about it. Can you elaborate on what the problem is? If you have the time, I'd also appreciate a review on the PR 😊

@TimothyMakkison
Copy link
Collaborator

@TimothyMakkison You mention a problem with `List', I couldn't find anything about it. Can you elaborate on what the problem is? If you have the time,

Iirc something like the following would emit a parameterless constructor error.

public record Source(List<string> Values);
public record Target(IList<string> Values);

....
public static partial IQueryable<Target>Map(IQueryable<Source> src);

It would work fine with List as the target as it has an accessible constructor. I suspect #1112 fixes it though.

I'd also appreciate a review on the PR 😊

Sure! 😊

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
4 participants