Test cases for LosslessValue Bool Int value

[serjooo]

Currently, the LosslessValue PropertyWrapper saves you the trouble of managing the inconsistencies of the server sending different types for one expected type and it does it well. However, it doesn't cover all the cases. It misses the case where a Bool value might be sent as an Int value, instead of a String. This PR shows the failure of LosslessValue when you attempt to decode a Bool as an Int by adding test cases for the scenario.

Expected Behavior

The LosslessValue PropertyWrapper to successfully decode a Bool value from an Int 0/1 value.

Current Behavior

The LosslessValue PropertyWrapper throws a typeMismatchError as Bool can not be constructed from a number string "0"/"1"

Possible Solution

This PR is to discuss possible solutions as it can be done many ways. Two possible solutions might be:

1. First solution can be that we add explicit code in LosslessValue PropertyWrapper to handle the Bool being constructed from an Int value. Problem with this solution is that it seems a bit hacky to explicitly handle the bool value case decoding in a more general PropertyWrapper like the LosslessValue.

2. Second solution that might also be desirable is to write a KeyedDecodingContainer extension for when the Decodable type is DefaultFalse PropertyWrapper and handle the specific scenario in that extension. If everything fails while attempting to decode successfully, we default to the default value. Problem with this solution is that if someone were to add a DefaultTrue PropertyWrapper the code would have to be duplicated.

Steps to Reproduce

Run tests added in this PR

[marksands]

Thanks for this! Something similar to this came up in a conversation today, so this is good timing 😄. I'll try to find some time this week to noodle on this.

[serjooo]

Anytime! If I can contribute more let me know. I'll maybe add my proposed solutions to this PR as an example with test cases so we can have an idea how it would work in reality.

[marksands]

@serjooo thanks for the new commits! The only thing that has me hesitating is that NSJSONSerialization has historically treated 0s and 1s as BOOLs (see: https://github.com/apple/swift/blob/88b093e9d77d6201935a2c2fb13f27d961836777/stdlib/public/Darwin/Foundation/JSONEncoder.swift#L2121-L2140), so I don't think it would be too out of the ordinary to go with solution 1. Appreciate your thoughts on that topic.

[marksands]

Having slept on it I think your solution of the container extension is the best way to go. It's the least surprising result and arguably programmer intended if this code path is ever executed. I'll take a closer look today before merging it in—thanks again! 👍

[serjooo]

@marksands don't mention it. It is my pleasure to be able to contribute!

I was thinking about this whole day at work as well and I was just about to start with a proposed solution for the LosslessValue as well. I just started with solution number 2 as it was more intuitive.

I think what you mentioned is very valid, but what is mind boggling me is why did the Swift team not take care of this directly in the Swift standard library. They only check that the value references the kCFBooleanTrue and kCFBooleanFalse value exactly rather than check equality or else we wouldn't need to do any workaround from our side.

One thing I also thought of today where implementing it in the container extension is advantageous over the LosslessValue for at least the boolean case is that most of the time you would wish to have a default value for booleans. LosslessValue prevents you from having optional currently and a decoding error will be thrown if the value doesn't exist. This will not happen with DefaultCodable and decoding the right value will be very important for the booleans in any case.

Let me know what you decide. We can also continue having the discussion and come up with new ideas!

[marksands]

I think we can benefit from both solutions. I pushed a potential solve for the LosslessValue decoding, let me know what you think. I'll add some minor comments in a review as well.

[serjooo]

@marksands there is still one more test case we haven't covered yet, and not sure if we should. Would love to hear your opinion. It is when the data for both DefaultFalse and LosslessValue is actually String Int Bool Value "0"/"1". I know this is super edge case, but I would not be surprised if somebody comes up with this case.

[marksands]

@marksands there is still one more test case we haven't covered yet, and not sure if we should. Would love to hear your opinion. It is when the data for both DefaultFalse and LosslessValue is actually String Int Bool Value "0"/"1". I know this is super edge case, but I would not be surprised if somebody comes up with this case.

I don't think this is an edge case I'm too worried about. I think at that point LosslessValue will only get them so far, and they should really fix their back end 😉 . Thanks for your contributions!! Merging and closing 🎉