Lucene.Net.Replicator.SessionToken ToString() overload doesn't output values correctly

[Shazwazza]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

ReplicationClient logs Lucene.Net.Replicator.SessionToken as a string and its ToString() overload results in a logging output that can't be used for debugging. For example, on this line:

lucenenet/src/Lucene.Net.Replicator/ReplicationClient.cs

Line 176 in 36f587d

WriteToInfoStream(string.Format("DoUpdate(): handlerVersion={0} session={1}", version, session));

WriteToInfoStream(string.Format("DoUpdate(): handlerVersion={0} session={1}", version, session));

It will end up logging like:

ReplicationThread - DoUpdate(): handlerVersion= session=id=1 version=2:1 files=System.Collections.Generic.Dictionary`2[System.String,System.Collections.Generic.IList`1[Lucene.Net.Replicator.RevisionFile]]

Which is using the ToString() override in SessionToken, but this is just outputting the generic ToString() of a generic list instead of outputting what is in that list.

Expected Behavior

Ideally it would output all of this information:

However, this will incur some overhead, so then all places where a SessionToken is output as a string within the ReplicationClient would need to be put behind a check like

if (infoStream.IsEnabled(INFO_STREAM_COMPONENT))

Before the ToString() overload would be called. In fact, this should be done anyways since it is concatenating and allocating a ton of strings even if the above check was in place because the string allocations happen before the call to WriteToInfoStream which does this check.

Steps To Reproduce

No response

Exceptions (if any)

No response

Lucene.NET Version

4.8.latest

.NET Version

8

Operating System

windows

Anything else?

No response

[NightOwl888]

Thanks for the report.

None of the BCL collection types are structurally formattable and Dictionary<TKey, TValue> wasn't replaced everywhere with JCG.Dictionary<TKey, TValue> because we used to have a wrapped implementation that was slower than the BCL type to handle null keys. However, since J2N 2.1.0, we have a low-level dictionary that will perform about the same on .NET Core and probably faster on .NET Framework than the BCL types.

Short Term

There are 2 potential fixes for this issue:

1. Replace Dictionary<TKey, TValue> with JCG.Dictionary<TKey, TValue> (preferred)
2. Use J2N.Text.StringFormatter as the IFormatProvider in string.Format()

Long Term

Longer term, we should do a review on the whole solution to catch issues such as this.

Option A

Create a Roslyn analyzer to catch scenarios where we are formatting collection types (or collection interfaces) to ensure the underlying implementation is structurally formattable (as well as any nested collections).

• The analyzer should check for string concatenation, string.Format() and string interpolation
• The code fix should be:
  • If the collection is only created inside of the current type and there is a structurally formattable J2N replacement, the J2N replacement should be used. Note that ConcurrentDictionary<TKey, TValue> has no replacement in J2N.
  • If the collection is passed into the current type (thus we cannot control the collection type), the code fix should be to use J2N.Text.StringFormatter.

This would be preferable because it would check for regressions on this issue in future versions of Lucene.NET.

Option B

A quick fix for this Lucene.NET release would be to simply replace all Dictionary<TKey, TValue> instances with JCG.Dictionary<TKey, TValue> throughout the solution. This will catch any potential issues where a null key was allowed in Lucene but they didn't create any tests to confirm null keys would work.

Of course, as previously mentioned, there may be cases where the collection is passed in through the constructor or set through a field or property using an interface and we don't have control over the collection type. In those cases, we should use J2N.Text.StringFormatter.