Logging in SegmentTemplateHelpers causes `System.FormatException`

[sherlock1982]

Assemblies affected Microsoft.AspNetCore.OData 9.2.1

Describe the bug Logging in SegmentTemplateHelpers causes System.FormatException

 Index (zero based) must be greater than or equal to zero and less than the size of the argument list.

Reproduce steps I'm not sure which exact invalid request causes it by

Expected behavior A proper exception must be logged

Screenshots

Additional context This is caused by not following CA2254 and fixed by #1451

Overall you should consider not logging the full exception in that case probably. Because this might be caused by a hacker and will flood logs but let's see the original exception first

[sherlock1982]

Actually it might be even worse because I see in the code it should not end with unhandled exception but rather return null

[WanjohiSammy]

Thanks @sherlock1982 for reporting this issue.

Does this mean that we should use {} placeholder syntax when logging messages. Log message template formatting

For example:

// Using concatenation
logger.LogWarning("Person " + firstName + " " + lastName + " encountered an issue"); // In violation

// String interpolation
logger.LogWarning($"Person {firstName} {lastName} encountered an issue"); // In violation

// Is this also in violation
logger.LogWarning(firstName);

Also, thanks for your contribution to fix this issue through PR https://github.com/OData/AspNetCoreOData/pull/1451. We will take a look

[sherlock1982]

You should not use string interpolation for performance reasons Also the message should be safe from format specifiers and user input. Ideally constant.

As for exception you can decide to attach it as a first param and logger will decide if and how it will be logged. On the other hand you can simply log exception message only. If you think the error and stacktrace is clear I would prefer not to log exception at all because it is probably not needed.

Some examples for reference. Note these are all working examples but do not pass code analysis

        string user = "Ivan";
        Exception exception = new("Alarm");

        // Wrong CA2254
        logger.LogInformation(user);
        logger.LogInformation($"Hello, {user}!");
        // Wrong CA2253
        logger.LogInformation("Hello, {0}!", user);
        // Wrong CA1727
        logger.LogInformation("Hello, {user}!", user);
        // Ideal
        logger.LogInformation("Hello, {User}!", user);

        // Wrong - exception message will be logged 2 times
        logger.LogWarning(exception, "Something is wrong: {Message}", exception.Message);
        // Ideal with full exception info
        logger.LogWarning(exception, "Something is wrong");
        // Ideal with only exception message
        logger.LogWarning("Something is wrong: {Message}", exception.Message);

[WanjohiSammy]

Fixed