Validate (and throw error) against non matching primitive data types

[ByteSturm]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the issue

Hello everyone,

while introducing Automapper into my project I noticed that the mapping between primitive data types is not type safe. I don't know if this behavior is intended or a bug but I assume it's the second one.

Models/DTOs/VMs

No response

Mapping configuration

No response

Steps to reproduce

import { createMap, createMapper } from '@automapper/core';
import { classes, AutoMap } from '@automapper/classes';
import 'reflect-metadata'

const mapper = createMapper({
  strategyInitializer: classes(),
});

class Foo {
  @AutoMap()
  foo: string;
}
class Bar {
  @AutoMap()
  foo: number;
}
createMap(
  mapper, Foo, Bar,
);

const foo = new Foo()
foo.foo='aaa'

console.log(
  mapper.map(foo, Foo, Bar)
)

Result:

Bar { foo: 'aaa' }

Expected behavior

At least I would expect Automapper to throw an error similar to the one in the case that no mapping between two types exists.

Screenshots

No response

Minimum reproduction code

No response

Package

• [ ] I don't know.
• [X] @automapper/core
• [X] @automapper/classes
• [ ] @automapper/nestjs
• [ ] @automapper/pojos
• [ ] @automapper/mikro
• [ ] @automapper/sequelize
• [ ] Other (see below)

Other package and its version

No response

AutoMapper version

8.5.0

Additional context

typescript v4.7 Node v16

[nartc]

https://automapperts.netlify.app/docs/mapping-configuration/type-converters it is intended. You can check this doc page out for more info

[ByteSturm]

Oh ok, thank you for pointing that out. I missed the following line

Instead of throwing an error, AutoMapper will map as-is to respect the dynamic nature of JavaScript. To control the conversions for these types when the properties are matching, we need to supply Type Converters to a specific Mapping

Maybe we can change this in a feature request? At least I would love to have a (global) configuration option which would make Automapper throw an error if the primitive types don't match. As Typescript provides type safety we are loosing it here. This could avoid some runtime errors if you forget to add the TypeConverters. I can understand why to treat the values as-is but I think it could also be an useful feature, introducing this optional type safety.

[nartc]

@ByteSturm we can discuss about it for sure. One main reason I didn't throw an error when first introducing Type Converter was I considered it a big breaking change to start throwing error now.

As for a feature request, I think a better solution would be to incorporate something like zod to do TRUE schema check against a model's metadata. However, I'm not sure whether I want to include zod as a dep to @automapper/core. Maybe a secondary entry point like @automapper/core/validation would be better. Thoughts?

[ByteSturm]

Right I see, that's a good reason, I fully understand that. Also thanks for changing the labels and title :)

I had a brief look at the zod project. It looks like a good fit for this use case. Also do I agree on the point of trying not to include it as a dependency in the core project as the behavior in general should be optional. So yes, adding something like @automapper/core/validation sounds like a good approach to me.

Regarding on how to configure the type checks, so if an error should be thrown or values should be used as-is, I think it should be possible to have different levels as with other mapping configurations. It should be something we can configure per mapping, on profile level and I would prefer to also set it globally when we create the mapper (like the strategy configuration).

I think with this approach you won't have breaking changes, no dependencies in @automapper/core and it could be flexibly configured on multiple levels.

[nartc]

@ByteSturm Yeah, I'm thinking about something like this

// before
import { createMap, createMapper, addProfile } from '@automapper/core';

// after
// these versions have zod integrated
import { createMap, createMapper, addProfile } from '@automapper/core/validation';

1. Mapper level with createMapper; by using this createMapper, we can also validate the Source instead of just the Destination. Like when you run: this.mapper.map(sourceObject, Source, Destination) and sourceObject does not conform to Source then we can throw validation error
2. Mapping level with createMap
3. Profile level with addProfile

They basically do some forms of 2 things:

• Dynamically create once (and store) the Zod schema from the discovered metadata on a model
• Run Zod validator against the destination object on afterMap (external) or postMap (internal) so Zod throws an error.

We also need to think about how to suppress the logs/errors on different environment (like in dev mode maybe suppress it but in prod mode, it needs to throw error)

[ByteSturm]

I mostly agree, just some remarks/thoughts I have here:

• Do you really need to store the Zod schema? You already have the information in your own meta data which type it is, don't you? Can't we reuse that with Zod?
• I think doing the validation after the afterMap method would make more sense. Then it also catches errors made by the developer when implementing some additional logic for the mapping. But I can also see that some people might have a different opinion on that.
• I don't know if it makes sense to suppress the errors in dev mode. If I do a mistake when assigning a string to a number, why shouldn't I get an error in dev mode too? If I pick a statically typed language like Java, the compiler will complain when I do it and that's not in production. In my opinion detecting the error as early as possible makes more sense than only enabling it in prod mode.

[nartc]

@ByteSturm

• AFAIK zod (idk zod that well tbh), zod works based on the schema that we created. So we create the schema once then store them makes sense to me. Of course, I need to implement it first and see if it actually makes sense or not
• Yeah. Again, whether or not we want to be flexible here is still up for debate
• Just a thought as someone recently requested a way to override the Logger.

Slightly off-topic, maybe we can even use Zod to build out the Classes/Interfaces so maybe a separate package like @automapper/zod 👀 to use in tandem with @automapper/classes/@automapper/pojos.

// before with AutoMap
class Foo {
   @AutoMap()
   foo!: string; 
}

// before with PojosMetadataMap.create
interface Foo {
   foo: string;
}

PojosMetdataMap.create<Foo>('Foo', {
   foo: String
});

// after with zod
const Foo = z.object({
   foo: z.string();
}); // explicitly using "z" here but @automapper/zod might expose some abstractions over zod to do extra things

// we have the schema, we can either do
// PojosMetadataMap.create('Foo', Foo);
// or think of a more fluid API for pojos

EDIT: maybe something like this

[ByteSturm]

Ok got your points, fine for me.

Regarding the @automapper/zod package, so you would use the zod models/schemas internally instead of the Automapper metadata? Also I doubt that you can omit the original class definition. One might use things like class-validator on it. If you "force" the developer to use these createModel() methods to get validation, the developer has to do double the effort and keep both in sync. Let's say from a users perspective it would be easier to use, if you can infer the zod model / schema from the existing class definition. As far as I understand the code and metadata, you already know which properties have which type.

[nartc]

using zod would make class-validator obsolete imo but I agree there should be something to add addtional decorators if I go with automapper/zod.

I think it can have a positive impact. Zod is pretty powerful and popular in the typescript ecosystem