LoggerMessage.LogValues<T> throws execption when calling [0]

[sreicht]

We created an own ILogger and iterate thru each KeyValuePair in the state object.

public class CustomLogger : ILogger
    {
        public IDisposable BeginScope<TState>(TState state)
        {
            return null;
        }

        public bool IsEnabled(LogLevel logLevel)
        {
            return true;
        }

        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
        {
            var structure = state as IReadOnlyList<KeyValuePair<string, object>>;
            if (structure == null)
            {
                return;
            }

            foreach (var pair in structure)
            {
                // do something
            }
        }
    }

We are also using the JwtBearer token authentication. Each time when the authentication is successful a Logger.Define() without any paramter is called.

https://github.com/aspnet/Security/blob/4d6ad51f8a9354d8e62cf8d66db97bd991b7d93c/src/Microsoft.AspNetCore.Authentication.JwtBearer/LoggingExtensions.cs#L37

When iterating thru the ReadOnlyList an ArgumentOutOfRangeException will be thrown because the count of _formatter.ValueNames is zero.

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parametername: index
System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
Microsoft.Extensions.Logging.LoggerMessage.LogValues`1.get_Item(Int32 index)
Submission#12.CustomLogger.Log(Microsoft.Extensions.Logging.LogLevel, Microsoft.Extensions.Logging.EventId, TState, System.Exception, Func<TState, System.Exception, string>)

https://github.com/aspnet/Logging/blob/release/src/Microsoft.Extensions.Logging.Abstractions/LoggerMessage.cs#L298

var logger = new CustomLogger();
var logMessage = LoggerMessage.Define<string>(
                 eventId: 2,
                 logLevel: LogLevel.Information,
                 formatString: "Successfully validated the token.");
logMessage(logger, null, null);

[BrennanConroy]

Why are you using Define<string> if you are passing null to the string everytime and don't have anything in the formatString? Use Define() like you are for _errorProcessingMessage

[sreicht]

I only added a short example do reproduce the problem.
In my case the error occured in the Microsoft.AspNetCore.Authentication.JwtBearer package.

https://github.com/aspnet/Security/blob/4d6ad51f8a9354d8e62cf8d66db97bd991b7d93c/src/Microsoft.AspNetCore.Authentication.JwtBearer/LoggingExtensions.cs

[BrennanConroy]

@kichalla
_tokenValidationSucceeded is Define<string> mistype?

[kichalla]

As @BrennanConroy mentioned, if you are not going to pass in named parmaters, do not use Define<string> and do as you are for ErrorProcessingMessage

[BrennanConroy]

@kichalla This is our code in security FYI

Sent from my HTC

----- Reply message -----
From: "Kiran Challa" notifications@github.com
To: "aspnet/Logging" Logging@noreply.github.com
Cc: "Brennan Conroy" brecon@microsoft.com, "Mention" mention@noreply.github.com
Subject: [aspnet/Logging] LoggerMessage.LogValues throws execption when calling 0
Date: Fri, Apr 22, 2016 5:20 AM

As @BrennanConroyhttps://github.com/BrennanConroy mentioned, if you are not going to pass in named parmaters, do not use Definehttps://github.com/aspnet/Security/blob/4d6ad51f8a9354d8e62cf8d66db97bd991b7d93c/src/Microsoft.AspNetCore.Authentication.JwtBearer/LoggingExtensions.cs#L20 and do as you are for ErrorProcessingMessage

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//issues/410#issuecomment-213401375

[kichalla]

Ah got it. This would need to be fixed for RC2.

[kichalla]

cc @Eilon @DamianEdwards

[muratg]

@kichalla could you start an internal thread to initiate the approval? :)

[kichalla]

Created a new issue on Security repo aspnet/Security#794, so closing this now.

Thanks! @sreicht for reporting the issue.