Include the file path in the exception when configuration loading fails

[0xced]

If the configuration file doesn't exist, a nice FileNotFoundException is thrown, including the physical path where it is expected to be found. If the file is found but fails to load, the file configuration provider exception is thrown, losing the file path information, making it hard to diagnose. Here is what it looks like. The line number is included but the file path is lost:

Unhandled exception. System.FormatException: Could not parse the JSON file.
 ---> System.Text.Json.JsonReaderException: Expected depth to be zero at the end of the JSON payload. There is an open JSON object or array that should be closed. LineNumber: 9 | BytePositionInLine: 0.
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
   at System.Text.Json.Utf8JsonReader.ReadSingleSegment()
   at System.Text.Json.Utf8JsonReader.Read()
   at System.Text.Json.JsonDocument.Parse(ReadOnlySpan`1 utf8JsonSpan, Utf8JsonReader reader, MetadataDb& database, StackRowStack& stack)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 utf8Json, JsonReaderOptions readerOptions, Byte[] extraRentedBytes)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 json, JsonDocumentOptions options)
   at System.Text.Json.JsonDocument.Parse(String json, JsonDocumentOptions options)
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.ParseStream(Stream input)
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationProvider.Load(Stream stream)
   --- End of inner exception stack trace ---
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationProvider.Load(Stream stream)
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.Load(Boolean reload)
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.HandleException(ExceptionDispatchInfo info)
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.Load(Boolean reload)
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.Load()
   at Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()
   at Microsoft.Extensions.Hosting.HostBuilder.BuildAppConfiguration()
   at Microsoft.Extensions.Hosting.HostBuilder.Build()

This commit wraps the internal file configuration provider exception into an InvalidDataException that include the physical path in the exception's message in order to ease diagnostics.

[ghost]

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[AaronRobinsonMSFT]

This will need to be localized properly. The string should be added to String.resx and then referenced here. Are any tests validating this exception type is what should be thrown? I am unfamiliar with this code path but there should be public documentation that identifies expected exceptions.

[0xced]

I have done proper localization in both 11d7eb2ad4fa7ac50d68b311542b76732dfb68ba and cc37340265d69462d9bc196742b0bcebdff96c6f.

Are any tests validating this exception type is what should be thrown?

I haven't found any. Anyway, I can't run tests because I'm currently stuck with a Could not load PlatformManifest error that I'm not able to solve. This is discussed at #30501 and dotnet/arcade#3878

I am unfamiliar with this code path but there should be public documentation that identifies expected exceptions.

I have also documented the new exception in 11d7eb2ad4fa7ac50d68b311542b76732dfb68ba.

[AaronRobinsonMSFT]

LGTM as soon as all green. I don't know about the subtle contract changes here. Perhaps @stephentoub has an opinion?

The exception message and type has been changed. Is this an issue for the API in question?

[0xced]

Question: should we use FileFormatException instead of InvalidDataException? (Or even a new custom exception?)

Pros:

• FileFormatException feels more accurate than InvalidDataException
• FileFormatException already has a localized message
• FileFormatException has a SourceUri property

Cons:

• Although FileFormatException is in the System.IO namespace, we need to add a reference to System.IO.Packaging.dll
• Creating a proper Uri from a local file path is very tricky, is it worthh it?

[0xced]

Replying to myself: using FileFormatException is finally not a good idea. The documentation says

This constructor initializes the Message property of the new instance to a system-supplied message that describes the error and includes the file name, such as "The file 'sourceUri' does not conform to the expected file format specification." This message takes into account the current system culture.

The reality is that the Message property is just Invalid file format., without information about the source URI at all.

[maryamariyan]

@0xced your PR would need an update as it has a conflict.

[ericstj]

Changing from one type of exception to another is considered a breaking change, unless the new exception derives from the previous. This change is wrapping all exceptions that might come from the Load method in a new exception type so no matter what is chosen this is potentially breaking. Marking as such.

[ericstj]

Can you choose an exception type already thrown from this method, or an exception type that derives from one already thrown?

[maryamariyan]

Related to #36007

[safern]

I think I would instead do what @ericstj is suggesting to avoid introducing a breaking change.

@0xced is this something that we can achieve?

Since this PR has not been active since April in either commits or responses, I will go ahead and close, but feel free to re-open and we can move this forward.

[danmoseley]

from @0xced: For the record: while this particular pull request has not been merged, an equivalent pull request (#51713) has been merged and has shipped as part of the .NET 6 release.