Include filename in load exception

[BrennanConroy]

We spent a few minutes today trying to debug a file load error only to find out that we were trying to fix the wrong file because the exception doesn't say what file failed to load.

https://github.com/aspnet/Extensions/blob/06dcb43bfb372585d3801cfc333a2d9ee9c189ea/src/Configuration/Config.FileExtensions/src/FileConfigurationProvider.cs#L90

cc @jkotalik

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[analogrelay]

This could be a nice and low-cost 5.0 fix-up, so I'll leave it untriaged for now.

[KimKiHyuk]

umm.. is it good? like

...
               using (var stream = file.CreateReadStream())
                {
                    try
                    {
                        Load(stream);
                    }
                    catch (FileNotFoundException e)
                    {
                        var error = new StringBuilder($"The configuration file '{Source.Path}' was not found and is not optional.");
                        HandleException(ExceptionDispatchInfo.Capture(error));
                    }
                    catch (Exception e)
                    {
                        HandleException(ExceptionDispatchInfo.Capture(e));
                    }
                }

[BrennanConroy]

Not quite, the original issue wasn't about a file missing, it was about the file load failing because of bad config, in the case of a json file there was invalid json in the file.

[KimKiHyuk]

Not quite, the original issue wasn't about a file missing, it was about the file load failing because of bad config, in the case of a json file there was invalid json in the file.

what about checking json file's syntax? (validation)
like

private void Load(bool reload)
{
            var file = Source.FileProvider?.GetFileInfo(Source.Path);
            if (file == null || !file.Exists)
            {
                if (Source.Optional || reload) // Always optional on reload
                {
                    Data = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
                }
                else
                {
                    var error = new StringBuilder($"The configuration file '{Source.Path}' was not found and is not optional.");
                    if (!string.IsNullOrEmpty(file?.PhysicalPath))
                    {
                        error.Append($" The physical path is '{file.PhysicalPath}'.");
                    }
                    HandleException(ExceptionDispatchInfo.Capture(new FileNotFoundException(error.ToString())));
             } 
            //  pseudo code starts here
             else if (isInvalidJsonFormat()) 
             {
                  // throw exception if json file is invalid
                  ....
             }
 
}

[heesunlee9]

@BrennanConroy, did you solve the problem? It seems like you already solve it.

[danmoseley]

@maryamariyan still 5.0?

[maryamariyan]

no it's not a show stopper for 5.0. moving the milestone.

[ericstj]

Ensure that the fix for this covers the suggestion from #42337

It looks like it would be very simple to pass the Source.Path to the FileNotFoundException's constructor.

The line in question is this:

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs

Line 74 in 8933510

HandleException(ExceptionDispatchInfo.Capture(new FileNotFoundException(error.ToString())));

[IEvangelist]

Hey, @maryamariyan and @eerhardt - do you mind if I take this one?

[eerhardt]

@IEvangelist - it's all yours!

[ericstj]

Make sure to consider the constraints that came up in the last PR: #35183

[IEvangelist]

Make sure to consider the constraints that came up in the last PR: #35183

Thanks, will do.