Auto flattening of array properties

LennartH commented 2 years ago

Is your feature request related to a problem? Please describe.

I'd like to use the auto flattening features for array properties. I tried a datastructure like this:

class Foo {
  @AutoMap()
  id: string;
  @AutoMap()
  name: string;
}

class Bar {
  @AutoMap()
  id: string;
  @AutoMap(() => Foo)
  foos: Foo[];
}

class BarDto {
  @AutoMap()
  id: string;
  @AutoMap(() => String)
  foosId: string[];
}

When mapping the following object the resulting foosId property is 'undefined`:

{
  id: 'bar1',
  foos: [
    { id: 'foo1', name: 'Foo 1' },
    { id: 'foo2', name: 'Foo 2' },
  ]
}

The property mapping is found since the names match the used CamelCaseNamingConvention, but when mapInitializedValue is resolved the get function doesn't check that source['foos'] returns an array so foos['id'] results inundefined`.

I also checked the following data structures to verify that flattening for simple cases works as expected:

class Baz {
  @AutoMap()
  id: string;
  @AutoMap()
  foo: Foo;
}

class BazDto {
  @AutoMap()
  id: string;
  @AutoMap()
  fooId: string;
}

Describe the solution you'd like

I'd like to be able to use auto flattening when mapping an array property to another array property. The perfect solution would allow for slight variations in source and destination property names (e.g. foos to fooIds instead of foosId). Covering all plural edge cases seems unfeasible, but removing/adding a trailing s when the source/destination property is an array and the default name isn't in source could cover most of the use cases.

Describe alternatives you've considered

My current workaround is to use convertUsing:

const dtoArrayToIdArray: Converter<Foo[], string[]> = {
  convert(source: Foo[]): string[] {
    return source.map((f) => f.id);
  }
}
createMap(mapper, Bar, BarDto,
  forMember((destination) => destination.foosId, convertUsing(dtoArrayToIdArray, (source) => source.foos))
);

Another workaround is mapFrom:

createMap(mapper, Bar, BarDto,
  forMember((destination) => destination.foosId, mapFrom((source) => source.foos.map((f) => f.id)))
);

Additional context

No response

nartc commented 2 years ago

What about making AutoMap accepts additional information like:

//                                        👇 this needs better naming
@AutoMap({ type: () => [String], flattenSourcePath: ['foos', 'id']})
fooIds: string[] // it doesn't matter how this is named.

LennartH commented 2 years ago

Sounds good! Much better than some fragile internal magic :sweat_smile:

A few thoughts:

Allow dot separated paths like foos.id (I'd prefer this because it looks cleaner imho)
Allow Record<TSource, string | string[]> in case the type is destination of multiple source types with different paths (feels like an edge case, but who knows)
Is it possible to a selector function like in forMember for better tool support and easier refactorings?
The parameter could be named sourcePath or mapFrom, since it looks like a shortcut of the mapFrom configuration function.

One minor question, is there a semantic difference between () => [String] and () => String or is it just an option to visually differentiate between array and non-array properties?

nartc commented 2 years ago

I agree dot notation looks cleaner. However, the internal of AutoMapper works with an array of paths instead so it's easier for AutoMapper to work with array.
Can you elaborate on this?
Yeah it sure can. However, we don't really know the type of the source type to infer correct type for Selector function to work. Eg: @AutoMap({type: () => [String], sourcePath: (source) => source.foos.id }). There's no way to infer source is a Foo[]. Also it is awkward for array type with Selector function as you can see
It's more like: "hey, when you flatten stuffs, look at these in the Source instead of trying to figure out on your own".

Yes. [String] lets AutoMapper knows that a type is an Array type. This doesn't affect much except for typeConverter() (afaik xD)

LennartH commented 2 years ago

You could use a type guard to catch paths in dot notation:

function AutoMapper(config) {
  if (config.flattenSourcePath && typeof config.flattenSourcePath === 'string') {
    config.flattenSourcePath = config.flattenSourcePath.split('.');
  }
  // The remaining code can use config.flattenSourcePath as string[]
}

For example if two types have a property with different names (for whatever reasons) that are mapped to the same destination id array:

class Foo {
  @AutoMap()
  id: string;
  @AutoMap()
  name: string;
}

class Bar {
  @AutoMap(() => [Foo])
  foos: Foo[];
}

class BarVariant {
  @AutoMap(() => [Foo])
  specialFoos: Foo[];
}

class FlattenedBar {
  @AutoMap({
    type: () => [String],
    flattenSourcePath: {
      [Bar.name]: ['foos', 'id'],
      [BarVariant.name]: ['specialFoos', 'id'],
    }
  })
  fooIds: string[];
}

Yeah, the usage with arrays looks awkward, but if the option could be used for things except tying two arrays together I think I would prefer a selector function with explicit typing like (source: Bar) => source.foo.id to catch renaming of foo or id. Otherwise the mapping breaks, since there's no way to type check the provided path.

I see :smile: Is this also considered by convertUsing so a custom converter function A -> B can also be used to map arrays?

nartc commented 2 years ago

Yeah, the "refactor-able" makes sense to me. I'll think about it.

This won't work for applications that use "mangle" when obfuscating/minimizing code for production. We might be able to make it work with Tuple instead:

@AutoMap({ type: () => [String], flattenSourcePath: [[Bar, ['foos', 'id']], [BarVariant, ['specialFoos', 'id']]]})

However, I think this gets into the weed where AutoMapper might not be a good fit if you have complex models like this.

LennartH commented 2 years ago

I was thinking a bit more about my third point and I don't think it's reasonable to handle this case in the AutoMap-Config. Most of the time there will be one "main relation" between two models. The flattening for these models can be derived from the flattening path in the config and any other special cases can be handled by overriding the default behaviour with a mapFrom modifier.

If you haven't started working on this I could try to create a PR for this. If you give me a pointer to how property selector functions work in decorators, I could also try to add support for this.

nartc commented 2 years ago

Thanks @LennartH for the offer!! You can start here: https://github.com/nartc/mapper/blob/main/packages/core/src/lib/mapping-configurations/for-member.ts#L31 to see how a Selector is turned into a string[]. Although, I would still think Selector with Array seems weird because we cannot do: s => s.foos.id

LennartH commented 2 years ago

I wanted to get started on this, but when running npm install I get the error Invalid package name "@automapper/classes/mapped-types": name can only contain URL-friendly characters.. I'm using node version 14.19.1 and npm version 8.7.0. It seems that the second slash is not allowed. Are there any restrictions/recommendations for the node and npm version? I couldn't see any in the Contributing Guide.

nartc commented 2 years ago

@LennartH I'm using npm v6 still. I'd love to change to pnpm but I need patch-package at the moment :(

nartc / mapper