Add the ability to reset object properties to nil from JSON

[DulMephistos]

The current version has a problem that makes it impossible for us to in our app in production, so we did all necessary modifications and now I'm requesting this pull request here and it may help other as well.

We needed the ability to reset some fields to nil when updating existing objects with new JSON data. We do that by setting the desired field in the JSON string with "null" value and we expect that ObjectMapper will set that field to nil when mapping to the object since the JSON contains the field in its body.

Example:

Imagine a Player class like this:

class Player {
   var firstName: String?
   var lastName: String?
   var team: String?
}

Initialized:

let json = "{\"firstName\": \"Bob\",\"lastName\": \"Sass\",\"team\": \"Red\"}"
let player = Mapper<Player>().map(json)

Update name and remove team

let json = "{\"firstName\": \"Ryan\",\"lastName\": \"Mitchel\",\"team\": null}"
Mapper().map(json, toObject: player)

This was previously impossible because the Mapper would not update the field if the value was nil

[tristanhimmelman]

Hi @pixel4, thanks for your work. This looks good and I think I will merge. I'm curious though, why was it necessary to add the isKeyPresent check?

[DulMephistos]

Hi @tristanhimmelman, I'm glad you like it, thanks. We use this library all the time and this change is key for us.

You need the flag isKeyPresent in order to tell if the value was present in the JSON or if it was missing. When the value is present and it's nil you can't tell by just setting the value to nil because it's ambiguous.
So when're about to map the properties and you see a nil value, without this flag it's impossible to differentiate between both scenarios.

[tristanhimmelman]

This check is important for performance reasons. We previously didn't include it and the valueFor function became a performance bottleneck because it was being called so often without any need.

Can you please revert this

[DulMephistos]

absolutely.. I'll revert it and check my tests tonight.

[tristanhimmelman]

One other thing that would be great to have before I merge this is a simple unit test that checks this functionality so that we ensure it is not broken down line with other updates. Would you be able to add one?

[DulMephistos]

yes.. I can add unit tests as well, np

[DulMephistos]

Hey @tristanhimmelman,

I just reverted the change you asked and also added unit tests for this new functionality.

I think we should also update the minor version of it since we added some considerable changes to its behavior on some edge cases, but I did not put the version change on this pull request because I figure you'd want to do this on a separate commit.

[tristanhimmelman]

@pixel4 thanks for the updates. Looks great!

I agree about updating the minor version. I will take care of that when I release.

[DulMephistos]

Thanks @tristanhimmelman