Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

ConfigBuilder.AddJsonFile throws ArgumentOutOfRange if given an empty file #335

Closed
ghost opened this issue Nov 12, 2015 · 15 comments
Closed
Assignees
Milestone

Comments

@ghost
Copy link

ghost commented Nov 12, 2015

Given the following code:

var config = new ConfigurationBuilder();
var tempFilePath = System.IO.Path.GetTempPath() + Guid.NewGuid().ToString() + ".json" 
var tempFile = File.Create(tempFilePath);
tempFile.Close();
config.AddJsonFile(tempFilePath);

You'll get the following exception:

   at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
   at System.Collections.Generic.List`1.get_Item(Int32 index)
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationProvider.Load(Stream stream)
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationProvider.Load()
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Add(IConfigurationProvider provider)
   at Microsoft.Extensions.Configuration.JsonConfigurationExtensions.AddJsonFile(IConfigurationBuilder configurationBuilder, String path, Boolean optional)
   at Microsoft.Extensions.Configuration.JsonConfigurationExtensions.AddJsonFile(IConfigurationBuilder configurationBuilder, String path)

Now I know that that's probably not the right way to use AddJsonFile but I would have expected a FormatException or something like that.

I'm using 1.0.0-rc2-15822, I attempted to add this as a unit test but I'm having issues with AddJsonFile being in a class that's not referenced by the unit test file I was editing.

@pgrudzien12
Copy link
Contributor

I assume that

configPath = System.IO.Path.GetTempPath() + Guid.NewGuid().ToString() + ".json"

?

@ghost
Copy link
Author

ghost commented Nov 12, 2015

@pgrudzien12 Fixed, thanks.

That's what I get for copy-pasting from two different files.

@ses4j
Copy link

ses4j commented Nov 13, 2015

I get the same error with a configuration file that has only one setting, such as this:

{
    "site": "dev",
}

Note, though, that a second line will workaround the issue.

{
    "site": "dev",
    "whatever": "a second line!"
}

`

@kirthik
Copy link
Contributor

kirthik commented Nov 13, 2015

This is happening because if SetBasePath is not called, we assume BasePath to be root of the application. In your case, you will have to do this

var config = new ConfigurationBuilder();
var tempFileName = Guid.NewGuid().ToString() + ".json";
var tempFilePath = System.IO.Path.GetTempPath() + tempFileName;
var tempFile = File.Create(tempFilePath);
tempFile.Close();
config.SetBasePath(System.IO.Path.GetTempPath()).AddJsonFile(tempFileName);

@divega
Copy link

divega commented Nov 13, 2015

It is strange because we use Path.Combine() to concatenate the BasePath and the file name, and that already short-circuits on IsPathRooted() to return just the file name.

@pgrudzien12
Copy link
Contributor

+1
Format exception should be thrown because 'System.ArgumentOutOfRangeException' is misleading if the file is empty.

SetBasePath does not help - file path is constructed fine. At least for RC1

@ses4j first example is corrupted (it has extra comma). Nevertheless in RC1 it loads correctly for me. Could you provide an example of your code that loads configuration and version of dependencies you used?

@divega
Copy link

divega commented Nov 14, 2015

I agreed it would be nice to wrap any of these exception during parsing into a FileFormatException

@divega divega added this to the 1.0.0-rc2 milestone Nov 16, 2015
@divega
Copy link

divega commented Nov 16, 2015

@kirthik can you look into this for rc2?

@pgrudzien12
Copy link
Contributor

I will if you don't mind (both of you).

@divega
Copy link

divega commented Nov 16, 2015

Sure thing! Looking forward to your PR.

pgrudzien12 added a commit to pgrudzien12/Configuration that referenced this issue Nov 17, 2015
@ghost
Copy link
Author

ghost commented Nov 19, 2015

@divega looks like #337 needs a bit of discussion, but this shouldn't be tagged as up-for-grabs AFAICT

pgrudzien12 added a commit to pgrudzien12/Configuration that referenced this issue Dec 20, 2015
@Eilon
Copy link
Member

Eilon commented Jan 15, 2016

@divega I unassigned @kirthik because she won't be back before RTM.

@divega
Copy link

divega commented Jan 15, 2016

Thanks @Eilon thanks. @pgrudzien12 actually provided the fix in #337 and a better one in #358 and the PRs are already assigned to @HaoK.

@HaoK
Copy link
Member

HaoK commented Jan 15, 2016

81115be

@divega
Copy link

divega commented Jan 16, 2016

Thanks @pgrudzien12 for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants