.string and .stringConvertable metadata value equality

[dankinsoid]

Expected behavior

I would expect that .string and .stringConvertible values with equal underlying values would be equal.

Actual behavior

.string and .stringConvertible values with equal underlying values are not equal.

Steps to reproduce

let string = "Some String"
let value1 = Logger.MetadataValue.string(string)
let value2 = Logger.MetadataValue.stringConvertible(string)
print(value1 == value2) // prints false

Or, if we make Logger.MetadataValue Codable, then:

enum SomeStringEnum: String, CustomStringConvertible {
  case onlyValue
}

let value1 = Logger.MetadataValue.stringConvertible(SomeStringEnum.onlyValue)
let data = try JSONEncoder().encode(value1)
let value2 = try JSONDecoder().decode(Logger.MetadataValue.self, from: data) // .string("onlyValue")
print(value1 == value2) // prints false

I suggest updating the Equatable conformance to this:

extension Logger.MetadataValue: Equatable {

    public static func == (lhs: Logger.MetadataValue, rhs: Logger.MetadataValue) -> Bool {
        switch (lhs, rhs) {
        case (.string(let lhs), .string(let rhs)):
            return lhs == rhs
        case (.stringConvertible(let lhs), .stringConvertible(let rhs)):
            return lhs.description == rhs.description
        case (.stringConvertible(let lhs), .string(let rhs)): // add this
            return lhs.description == rhs
        case (.string(let lhs), .stringConvertible(let rhs)): // and this
            return lhs == rhs.description
        case (.array(let lhs), .array(let rhs)):
            return lhs == rhs
        case (.dictionary(let lhs), .dictionary(let rhs)):
            return lhs == rhs
        default:
            return false
        }
    }
}

This will lead to equality of some string and non-string values like .stringConvertible(0) == .string("0") or even .stringConvertible(0) == .stringConvertible("0"), but I think it's fine for dynamic types.

[Lukasa]

Hmm, I'm not so sure I agree with this, but I'm curious as to what @FranzBusch thinks

[dankinsoid]

or maybe it would be better to use more specific types instead of .stringConvertable, like .bool, .double, .int

[dankinsoid]

The most harmless fix is using type checks like this:

extension Logger.MetadataValue: Equatable {

    public static func == (lhs: Logger.MetadataValue, rhs: Logger.MetadataValue) -> Bool {
        switch (lhs, rhs) {
        case (.string(let lhs), .string(let rhs)):
            return lhs == rhs
        case (.stringConvertible(let lhs), .stringConvertible(let rhs)):
            return lhs.description == rhs.description
        case (.stringConvertible(let lhs), .string(let rhs)): // Add this
            return (lhs as? String) == rhs
        case (.string(let lhs), .stringConvertible(let rhs)): // And this
            return lhs == (rhs as? String)
        case (.array(let lhs), .array(let rhs)):
            return lhs == rhs
        case (.dictionary(let lhs), .dictionary(let rhs)):
            return lhs == rhs
        default:
            return false
        }
    }
}

[FranzBusch]

I would be interested in getting a bit more background here. When are you doing this equitability check and what do intend to get out of it? Purely from a type perspective these things are not equal and I am not a fan of writing custom Equatable conformances. This sounds to me more like you want a specific method that does the checking like you expect it for your particular use-case.

[dankinsoid]

@FranzBusch, I've never actually tried to compare metadata. I just implemented a similar type for my analytics package and noticed this behaviour.

I might misunderstand the reasons why you added the stringConvertible case. I assumed you included it to retain metadata value type information, so in my opinion, from a type perspective, .string(string) and .stringConvertible(string) are equivalent, because the underlying type is the same and the value is the same. But I'm not insisting on it.

I believe that a case with a type-erased argument like any CustomStringConvertible could potentially lead to inconsistency or ambiguous behaviour in one way or another.

[FranzBusch]

I disagree that the cases .string and .stringConvertible are equivalent just because the associated value is a String. Both cases indicate that the value comes through different APIs. The former means a user put in a String whereas the latter means it was derived by calling the description property of the type. There might be cases where it is valuable to check if the metadata value comes from a description since those could potentially leak user data more easily. Especially if these types come from arbitrary libraries. A logging handler might decide that they redact any .stingConvertible in production environments for security purposes.

Taking a step back, I don't see a reason why we should diverge from the auto synthesised Equatable implementation here. Diverging requires a strong motivation because it suddenly changes the meaning of == in unexpected ways. Furthermore, you can easily implement your own method that checks equality in a custom way that assumes the two cases are equal if the underlying Strings are equal.

[dankinsoid]

@FranzBusch okay, no problem, I just wanted to highlight some possible ambiguous behaviour. But of course, I wouldn't suggest diverging from the auto-synthesized Equatable implementation, but there is no auto-synthesized implementation. Right now, in the library, Equatable conformance is already written manually.