MessageSequence's IEnumerable Handling Assumptions lead to erroneous serialization

[fitdev]

The following cases in the ‎MessageSequence.ToString methods are very problematic:

https://github.com/Cysharp/ZLogger/blob/b373ebbf67a41eb54ec283b4945785b29d2f7bca/src/ZLogger/ZLoggerInterpolatedStringHandler.cs#L255C1-L258C26

https://github.com/Cysharp/ZLogger/blob/b373ebbf67a41eb54ec283b4945785b29d2f7bca/src/ZLogger/ZLoggerInterpolatedStringHandler.cs#L294C1-L298C26

1. They default to JsonSerializer which may not be desired behavior in all cases.
2. Worse they pass the object to be serialized as plain IEnumerable and not as actual type. This is really problematic because:

• The type may implement several IEnumerable<T>s: IEnumerable<Foo> and IEnumerable<Bar>, and its non-generic IEnumerable version may not be applicable at all.
• Furthermore, IEnumerable may not at all apply for the purposes of serialization (not every IEnumerable<T> is a "normal" collection - it may have inherited the interface, or the interface may be there for technical reasons, but should not be considered for serialization purposes).
• The type may have JsonConverterAttribute that actually specifies how it should be serialized. This routine completely disregards it.
• JsonSerializerOptions are not at all exposed. If JsonSerializer is used at all as a fallback mechanism, then there should be a way to specify its options - either globally through static API, or on in the Logger's options.

So, to that end:

1. IEnumerable case should perhaps be removed altogether, and ToString be used instead
2. Perhaps more cases like IFormattable or ISpanFormattable cases should be added - types checked against those, and these interfaces used for stringification.
3. If JsonSerializer is to be used, then at least allow providing options for it, and pass in the concrete type to the serializer, not IEnumerable, such that serializer can pick up JsonConverterAttribute.

[neuecc]

thanks, i will check soon.

[neuecc]

First, there is no provision for customizing generic output, and we are not using JsonSerializer for that purpose. The reason IEnumerable is targeted is to cater to minor use cases, such as wanting to pass arrays of primitives like int[]. It's worth noting that since Cysharp/Utf8StringInterpolation supports ISpanFormattable, my understanding is that it's supported if it's not IEnumerable. When outputting in JSON, you need to explicitly write :json like {user:json}. Now, it's indeed a problem that there's nowhere we're passing JsonSerializerOptions even in such cases, so I'd like to fix that.

[fitdev]

Json is not the only issue for me, it really has to do with how IEnumerables are treated by default. I think it is simply wrong to assume that all IEnumerables should be treated as collections for the purposes of logging (whether they use Json or not).

To give you a concrete example, I have a type FileSystemPath which is a struct that basically wraps a string and is a effectively a string for logging/serialization purposes. However, it is also an IEnumerable<PathSegment>. So, when I want to log it as something like: logger.LogInfo($"Could not open the file: '{file.Path}'") where Path property is of type FileSystemPath, I really just want its ToString representation, not it being automatically treated as IEnumerable, which is what happens by default, because IEnumerable branch IIRC precedes the more general case where it is just essentially ToString. Of course, I could do logger.LogInfo($"Could not open the file: '{file.Path.ToString()}'") but that is rather inconvenient, and you have to remember all the types for which you have to do this.

[neuecc]

However, if it can't handle simple arrays, that would be unsatisfactory in its own way. I think this is a question of how to set better defaults, and since examples like your FileSystemPath are rather special cases, if it can be worked around using ToString, I think that would be fine.

[neuecc]

I thought about it, but from a consistency standpoint, it might seem like unnecessary meddling. Even for arrays, there's already a mechanism in place to log them in a relatively readable format using :json. So it might be better to simply use ToString() as is.

I'll think about it a bit more.

[fitdev]

I think that perhaps my 2nd suggestion would be preferable. Simply adding one or more cases for IFormattable or ISpanFormattable before the IEnumerable case would solve it. If a type implements it, then do the formattable logic instead of defaulting to IEnumerable logic.

[neuecc]

Such additions that are meant to address only one's own specific cases are not desirable at all.

[fitdev]

One can argue the same way for IEnumerable itself that it is also a special case. But I understand it is a point of view. Still I think IFormattable / ISpanFormattable are so common/standard that they should be directly supported, especially when the result of not doing so leads to ugly hacks around and/or limits library's usage (i.e. polluting user code with needless ToStrings resulting in poorepr performance had it supported ISpanFormattable)