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

Mapperly does not properly map DateTimeOffset to gRPC entities #1107

Closed
DaveNayMT opened this issue Feb 7, 2024 · 3 comments
Closed

Mapperly does not properly map DateTimeOffset to gRPC entities #1107

DaveNayMT opened this issue Feb 7, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@DaveNayMT
Copy link

Describe the bug
When mapping from a domain entity containing a DateTimeOffset property to a gRPC DTO containing a google.protobuf.Timestamp property, Mapperly does not set the correct property value. Instead the default value is assigned.

Declaration code

using Riok.Mapperly.Abstractions;

namespace Mapperly_gRPC.Services
{
    [Mapper]
    public static partial class ResponseMapper
    {
        public static partial HelloReply MapToHelloReply(this Response response);

        //private static Google.Protobuf.WellKnownTypes.Timestamp DateTimeOffsetToTimestamp(DateTimeOffset source)
        //{
        //    return Google.Protobuf.WellKnownTypes.Timestamp.FromDateTimeOffset(source);
        //}
    }
}

Actual generated code

// <auto-generated />
#nullable enable
namespace Mapperly_gRPC.Services
{
    public static partial class ResponseMapper
    {
        public static partial global::Mapperly_gRPC.HelloReply MapToHelloReply(this global::Mapperly_gRPC.Services.Response response)
        {
            var target = new global::Mapperly_gRPC.HelloReply();
            target.Message = response.Message;
            target.ResponseTime = MapToTimestamp(response.ResponseTime);
            return target;
        }

        private static global::Google.Protobuf.WellKnownTypes.Timestamp MapToTimestamp(global::System.DateTimeOffset source)
        {
            var target = new global::Google.Protobuf.WellKnownTypes.Timestamp();
            return target;
        }
    }
}

Expected generated code

// <auto-generated />
#nullable enable
namespace Mapperly_gRPC.Services
{
    public static partial class ResponseMapper
    {
        public static partial global::Mapperly_gRPC.HelloReply MapToHelloReply(this global::Mapperly_gRPC.Services.Response response)
        {
            var target = new global::Mapperly_gRPC.HelloReply();
            target.Message = response.Message;
            target.ResponseTime = MapToTimestamp(response.ResponseTime);
            return target;
        }

        private static global::Google.Protobuf.WellKnownTypes.Timestamp MapToTimestamp(global::System.DateTimeOffset source)
        {
            return Google.Protobuf.WellKnownTypes.Timestamp.FromDateTimeOffset(source);
        }
    }
}

Environment (please complete the following information):

  • Mapperly Version: 3.3.0
  • .NET Version: .Net 8
  • Target Framework: .Net 8
  • Compiler Version: 4.8.0-7.23572.1 (7b75981c)
  • C# Language Version: 12.0
  • IDE: Visual Studio v17.8.6
  • OS: Windows 10

Additional context
Example project is available here: https://github.com/DaveNayMT/Mapperly_gRPC

@DaveNayMT DaveNayMT added the bug Something isn't working label Feb 7, 2024
@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Feb 8, 2024

Hey, unfortunately this is "intentional" but not ideal. Mapperly will try to convert DateTimeOffset to Timestamp but was unable to find any matching properties or a valid constructor for TimeStamp that would accept DateTimeOffset as an value. It falls back to using the default parameterless constructor and never uses DateTimeOffset.

Expected Generated Code

Mapperly doesn't have ability to look for matching static constructors like Timestamp.FromDateTimeOffset. It could be possible but I suspect it isn't done in order to avoid weird behaviour.

As you've found, providing Mapperly with a DateTimeOffset -> Timestamp method fixes the issue. I understand this isn't perfect.

@latonz do you think it would be worth adding support for GRPC/Proto types? I suspect they are frequently used with Mapperly and would be appreciated by a lot of people.

@latonz
Copy link
Contributor

latonz commented Feb 9, 2024

I think adding support for non .NET types is not a good idea as this would get endless. Instead we could think about adding support for

  • static methods on the target type named From{SourceType.Name} accepting the source type as a single parameter and the target type as return type
  • static methods on the source type named To{TargetType.Name} accepting the source type as single parameter returning the target type

@latonz
Copy link
Contributor

latonz commented Feb 9, 2024

Closing this as it works as expected.

@latonz latonz closed this as not planned Won't fix, can't repro, duplicate, stale Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants