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

Add Object Mappings APIs #14932

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

Add Object Mappings APIs #14932

wants to merge 6 commits into from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Dec 21, 2023

Related to #14920

@jtkech I think it's good to map the objects automatically, to avoid such missing. I remember we can apply this pattern in places in the code-base

@sebastienros
Copy link
Member

Do we need an abstraction? Maybe just use the mapper directly when necessary. We could think that one day we want to change it but I don't think that would be an issue, just replace with new calls. Or maybe a static helper to wrap the implementation (not suggesting it).

@sebastienros
Copy link
Member

Do you have arguments for/against Mapster?

@hishamco
Copy link
Member Author

Or maybe a static helper to wrap the implementation (not suggesting it).

I thought to introduce a static helper at the beginning, but later I thought about the extensibility which is not an issue as you mentioned

Do you have arguments for/against Mapster?

I just looked for some top implementations, of course AutoMapper is one of the popular object mappings libraries but it requires configuration. In the end, I decided to put simplicity & efficiency on top of my head

If you prefer something else in terms of performance, please suggest it, you're our PERF guru :)

@hishamco
Copy link
Member Author

If we agree about the principal I might need to add several unit tests

@MikeAlhayek
Copy link
Member

I used AutoMapper in a large project in the past. It made things easy in terms of mapping 1:1 objects. But when properties don't match up, you'll have to configure it so it understands how to map. I think I had a MapFrom and MapTo properties that would allow me to define any custom properties.

What happens when you want to define mappers? Do we have to register all these mappers to the IoC? Could this be an over kill at a framework level?

I don't mind auto mapping from coding perspective. I am just concern about performance issues that we could end up with. Like could we end up with 100s of mapper implementations that needs to be evaluated and added to the IoC container?

We just think about the pros and cons of bringing this to a framework.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@hishamco
Copy link
Member Author

@sebastienros while I'm cleaning up my PRs, is it fine to introduce an object Mappings APIs such other frameworks or shall I use a static class / method to handle the case I faced in SMTP settings

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

Having used AutoMapper before as well, it's awesome when everything is simple, 1-1 mapping with similar properties. When not, you have to juggle around and end up with less than beautiful code.

I think using some auto-mapper is good if you write such mapping code a lot, but not if you write it infrequently but run it all the time (because then there's a performance penalty).

In OC, all the viewmodel-model mappings might be a good candidate too (but there can be security implications, so maybe again we end up ith something more complex).

However, I think this only makes sense if we use source generators: then, there's no practical runtime impact, just build time, but that we can live with if we can delete a lot of code. Could you try something like https://github.com/riok/mapperly?

@hishamco
Copy link
Member Author

I will try to use mapperly or build something custom for OC, let me start on it then properly demo it next week to get the feedback

# Conflicts:
#	src/OrchardCore.Build/Dependencies.props
#	src/OrchardCore.Modules/OrchardCore.Email/Services/SmtpSettingsConfiguration.cs
@hishamco
Copy link
Member Author

I'm still struggling to use a generic version instead of creating a mapper per type. I'm not sure if the generic works well with mapperly I did a quick example but the unit tests fails

Copy link
Contributor

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up and remove the "stale" label.

@github-actions github-actions bot added the stale label May 23, 2024
Copy link
Contributor

github-actions bot commented Jun 9, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

@hishamco
Copy link
Member Author

hishamco commented Jun 9, 2024

@Piedone is this something I should continue with?

@Piedone
Copy link
Member

Piedone commented Jun 9, 2024

I don't think this matter is too important because I don't think having some kind of auto-mapping would bring much value, but I'm open to being convinced. If you're interested in the topic for personal reasons then I'd recommend you do a small PoC with a source generator and then we can discuss.

@hyzx86
Copy link
Contributor

hyzx86 commented Jun 21, 2024

I also use AutoMapper in my OC projects, and it's really handy when I need to convert from a ContentPart to an object type, using the following configuration

    public class ContentFieldsMappings : Profile
    {
        public ContentFieldsMappings()
        {
            #region OC fileds maping

            CreateMap<MultiTextField, string[]>().ConvertUsing(s => s.Values);
            CreateMap<LinkField, string>().ConvertUsing(s => s == null ? default : s.Text);
            CreateMap<HtmlField, string>().ConvertUsing(s => s == null ? default : s.Html);
            CreateMap<TextField, string>().ConvertUsing(s => s == null ? default : s.Text);
            CreateMap<BooleanField, bool?>().ConvertUsing(s => s == null ? default : s.Value);
            CreateMap<BooleanField, bool>().ConvertUsing(s => s == null ? false : s.Value);

            CreateMap<NumericField, decimal?>().ConvertUsing(s => s == null ? default : s.Value);
            CreateMap<NumericField, decimal>().ConvertUsing(s => s == null ? 0 : Convert.ToInt64(s.Value));

            CreateMap<NumericField, int?>().ConvertUsing(s => s == null ? default : Convert.ToInt32(s.Value));
            CreateMap<NumericField, int>().ConvertUsing(s => s == null ? 0 : Convert.ToInt32(s.Value));

            CreateMap<NumericField, long?>().ConvertUsing(s => s == null ? default : Convert.ToInt64(s.Value));
            CreateMap<NumericField, long>().ConvertUsing(s => s == null ? 0 : Convert.ToInt64(s.Value));

            CreateMap<UserPickerField, string[]>().ConvertUsing(s => s == null ? default : s.UserIds);
            CreateMap<UserPickerField, string>().ConvertUsing(s =>
             s != null && s.UserIds != null && s.UserIds.Length > 0 ?
                            s.UserIds.FirstOrDefault() : string.Empty);

            CreateMap<ContentPickerField, string[]>().ConvertUsing(s => s.ContentItemIds ?? Array.Empty<string>());
            CreateMap<ContentPickerField, string>().ConvertUsing(s =>
                                s != null && s.ContentItemIds != null && s.ContentItemIds.Length > 0 ?
                            s.ContentItemIds.FirstOrDefault() : string.Empty);

            CreateMap<DateField, DateTime?>().ConvertUsing(s => s == null ? default : s.Value);
            CreateMap<DateField, DateTime>().ConvertUsing(s => s == null ? default : s.Value.GetValueOrDefault());

            CreateMap<DateTimeField, DateTime?>().ConvertUsing(s => s == null ? default : s.Value);
            CreateMap<DateTimeField, DateTime>().ConvertUsing(s => s == null ? default : s.Value.GetValueOrDefault());

            CreateMap<TimeField, TimeSpan?>().ConvertUsing(s => s == null ? default : s.Value);
            CreateMap<TimeField, TimeSpan>().ConvertUsing(s => s == null ? default : s.Value.GetValueOrDefault());

            CreateMap<JObject, object>().ConvertUsing(s => s == null ? default : s.ToObject<object>());
            CreateMap<JValue, object>().ConvertUsing(s => s == null ? default : s.Value);

            CreateMap<ContentTypeDefinition, ContentTypeDefinitionDto>().ConvertUsing((s, t) => s == null ? default : s.ToDto());
            CreateMap<ContentPartDefinition, ContentPartDefinitionDto>().ConvertUsing((s, t) => s == null ? default : s.ToDto());
            #endregion

            CreateMap<ContentItem, ContentItemIndex>();

        }
    }

@MikeAlhayek
Copy link
Member

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

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

Successfully merging this pull request may close these issues.

5 participants