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

fix: make instantiate assign undefined (instead of a new Date instance) for Date property when Source is undefined #254

Closed
abouroubi opened this issue Feb 5, 2021 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@abouroubi
Copy link

Hi, and thanks for the great job with this new version.

I'm trying to understand how the classes mapper work, I have two situation where an empty class is created instead of just returning undefined.

The first is when I have an undefined object returned from the database, when I send it to it to the mapper, instead of returning just undefined, it returns an instance of the destination class with empty values.

The second is kind of the same but with properties of date type, I have a property lastEdited which is of type Date, and can be undefined, unfortunately when this value is undefined in the source object, automapper will initialize it with the current date, how can I avoid this behavior ?

@nartc
Copy link
Owner

nartc commented Feb 5, 2021

Hi @abouroubi , thanks for the kind words.

  1. For first issue, this is due to map() does not null-check for sourceObj, which I'd consider a bug in this case.
    -> this has been fixed with 2.1.1 release

  2. For this issue, this is due to this code right here: https://github.com/nartc/mapper/blob/main/packages/classes/src/lib/utils/instantiate.util.ts#L60. When classes plugin instantiates a class with the metadata (from @AutoMap()), if it sees a Date, it will instantiate a new Date instance to be used down in the pipeline. There's a workaround, but it's quite involved which I'm going to list the steps as follow:

    • You can extend the classes plugin with this documentation: https://automapperts.netlify.app/docs/plugins-system/extend-plugin ( the docs isn't that good but feel free to ask questions ).
    • When you extend the classes plugin, you will want to override the preMap() function. preMap() returns a new instance of a Source (and Destination) with the metadata, this is where the new Date() is set. So, you will want to run your custom logic to keep the undefined Date as undefined before you return from preMap(). The example in the docs is to turns all null values to undefined.
    • Use the extended plugin for pluginInitializer

Would that work? In addition, would you argue that instantiate() should assign undefined to undefined regardless of the metadata?

@nartc nartc self-assigned this Feb 5, 2021
@nartc nartc added bug Something isn't working question Further information is requested and removed bug Something isn't working labels Feb 5, 2021
@nartc nartc removed their assignment Feb 6, 2021
@abouroubi
Copy link
Author

Thanks for the quick reply.

Indeed it's a lot of code rewrite for just the undefined part.

My take on this, is that if my property is optional (myDate?: Date), (I don't now if this is reflected in the metadata, if not maybe add something like this AutoMap({ required: false }) ?) automapper should assign undefined to it if the source is also undefined.

@micalevisk
Copy link
Contributor

micalevisk commented Feb 6, 2021

I'm with you @abouroubi

I didn't understand why Date properties should be(?) initialized while other primitive properties are not 😕
L54 vs L62

if (isPrimitiveConstructor(metaResult)) {
(instance as Record<string, unknown>)[key] = isDefined(valueAtKey, true)
? valueAtKey
: undefined;

if (isDateConstructor(metaResult)) {
(instance as Record<string, unknown>)[key] = isDefined(valueAtKey)
? new Date(valueAtKey as number)
: new Date();

@nartc
Copy link
Owner

nartc commented Feb 15, 2021

@abouroubi That would work but that would also increase the complexity in the core a lot. Introducing an option that affects how Automapper maps a property would require conditional check in various places, and the new option isn't actually mandatory when there are workarounds to achieve the same thing. That said, I'll change how instantiate works for Date which is to assign an undefined value instead of a new Date() when source is undefined, to keep in consistent with the other primitives (String, Number, Boolean). Feel free to submit a PR for this.

@nartc nartc added enhancement New feature or request good first issue Good for newcomers and removed question Further information is requested labels Feb 15, 2021
@nartc nartc self-assigned this Feb 15, 2021
@nartc nartc changed the title How to manage undefined values fix: make instantiate assign undefined (instead of a new Date instance) for Date property when Source is undefined Feb 15, 2021
@nartc nartc removed their assignment Feb 15, 2021
@micalevisk
Copy link
Contributor

micalevisk commented Feb 19, 2021

I'm stuck in an issue while doing unit tests due to this new Date() usage instead of undefined (I've changed L62 of instantiate.utils.ts locally and then everything works as expected)

I'm basically using the same approach as here https://github.com/nartc/mapper/blob/eaef6813ec6fa26cee4cc60094c14abf68b9e326/packages/nestjs-integration-test/src/app/profiles/user.profile.spec.ts

In my test suite map doesn't map null to null, while mapArray do:

class User {
  @AutoMap(() => Date)
  someDateNullField: Date | null
}
class UserVm {
  @AutoMap()
  // @AutoMap(() => Date)
  someDateNullField: Date | null
}

// ...

const user = new User()
user.someDateNullField = null

const expectedUser = new UserVm()
expectedUser.someDateNullField = null

const userVm = mapper.map(user, UserVm, User)
expect(userVm).toEqual(expectedUser) // this fails ✕
                                     // because `userVm.someDateNullField` is `new Date()`

const usersVm = mapper.mapArray([user], UserVm, User)
expect(usersVm).toEqual([expectedUser]) // this passes, which is right

@nartc can you please add this case in your user model fixture?

@nartc
Copy link
Owner

nartc commented Feb 19, 2021

@micalevisk Yeah, there is a slight inconsistency between map and mapArray as of the moment. map does have preMap() run but mapArray() does not. However, I'm putting in a fix to make instantiate assigning undefined instead of new Date() instance for Date member. I'll add this test case to the user model fixture. Thank you!

@nartc nartc closed this as completed in 8e17527 Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants