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)

[neuecc]

This means that ISpanFormattable is supported, but IEnumerable is given priority.
Types that are both ISpanFormattable and IEnumerable typically do not exist.

[fitdev]

Types that are both ISpanFormattable and IEnumerable typically do not exist.

This is just an assumption until you are hit by one of those cases. I think that since Logging is more to do with serialization/formatting, if a type is both IEnumerable and ISpanFormattable, ISpanFormattable should be given priority since it "knows how to format" and in fact is the reason for the interface in the first place.

[neuecc]

Since no one likes unnecessary breaking changes, I've kept the basic behavior the same while prioritizing IFormattable/ISpanFormattable when dealing with IEnumerable, just as you suggested.
In most cases, this probably wasn't an issue before, so this change shouldn't be considered a breaking change as such.

[fitdev]

Thank you for supporting IFormattable.

However there is also a secondary issue which I mentioned too:

IEnumerables get passed as plain IEnumerable and not as actual type when they go Json route: CodeGeneratorUtil.AppendAsJson(ref stringWriter, enumerable);

Again this is problematic for cases where a type defines its own Json serialization logic. So perhaps they should instead be passed as CodeGeneratorUtil.AppendAsJson(ref stringWriter, p.BoxedValue);. To make it a non-breaking change this can happen only when the type defines its own Json serialization logic - for example by having JsonConverterAttribute on it.

The general point here is that if the logging ends up going via Json serialization route, then it should properly handle Json serialization as defined by the types themselves, not relying on general assumptions simply by looking at the most common interfaces.

[neuecc]

Ah, since this is Boxed, it's correct to pass the Type.
I'll fix it.