Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic_json::value throws exception instead of returning default value #871

Closed
unholytony opened this issue Dec 13, 2017 · 7 comments
Closed
Labels
state: needs more info the author of the issue needs to provide more details state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@unholytony
Copy link

Bug Report

My apologies if this is expected behaviour, it just seemed somewhat unexpected.

I am using json objects to represent configuration files that may or may not be present. Missing files are represented as default constructed json objects.

When trying to retrieve a value using a json_pointer from an empty json object it is throwing an exception instead of returning my specified default value. This is happening because the empty json object is not an object and so it is failing the first test ( JSON_LIKELY(is_object()) ) and thus throwing an exception. This seems counter intuitive given the description of the function.

I would have preferred it that if for any reason the json object could not return a valid value that it should return the default. As it currently stands I will have to wrap all value requests to the json object in a helper function to make sure that regardless of the state of the json object I always either get the stored value or the defaulted value. This essentially makes the "value" function pointless, for my needs at least.

This is on Visual Studio 2017

Thanks for a great library by the way.

@gregmarr
Copy link
Contributor

The value function documentation does say this:

@throw type_error.306 if the JSON value is not an object; in that cases,
using `value()` with a key makes no sense.

@nlohmann Typo: cases should be case.

It might make sense to allow returning the default value for a type of null, indicating that this particular thing was not provided, but continue to throw for anything else.

@unholytony Can you provide a complete sample of how you're using this, including the input that you expect to work and return a default value?

@nlohmann
Copy link
Owner

@gregmarr Thanks for the hint. Fixed with #875.

@unholytony
Copy link
Author

Yes sorry, I had not read the comment fully. My apologies. I see now that value is not meant to be safe in the way I had expected.
I have now modified my code to add a wrapper function that I will use instead of value.

My code was essentially

	nlohmann::json getJsonData( std::string const& filename )
	{
		auto const raw_data = getRawData( filename );	// returns a std::vector of the file content which may be empty if the file does not exist
		if ( raw_data.size() )
		{
			try
			{
				return nlohmann::json::parse( raw_data.begin(), raw_data.end() );
			}
			catch ( std::exception const& e )
			{
				LOG( "Failed to parse json data : ", e.what() );
			}
		}
		return {};
	}
	
	void readConfig()
	{
		auto const config_json = getJsonData( "some_optional_json_file.json" );
		int const width = config_json.value( "/backbuffer/width", 3840 );			// exception
		int const height = config_json.value( "/backbuffer/height", 2160 );			// exception
	}

Incidentally, when writing the safe wrapper I struggled to do so without resorting to a try catch scenario.
Is there any way of testing for the validity of a json_pointer without just trying to request it and catch the generated exception.
I had a brief look but couldnt find anything. It seems that even internally you use exceptions to indicate failure to find a key value.

which leads to the situation where

#define JSON_NOEXCEPTION
#include "json.hpp"

int main()
{
	auto const data = R"({ "hello" : "world" })"_json;
	auto const v1 = data.value( "/hello"_json_pointer, "default_value" );				// v1 == "world"
	auto const v2 = data.value( "/goodbye"_json_pointer, "please don't explode" );		// explodes
	return 0;
}

I don't personally use JSON_NOEXCEPTION so I might be missing something here but this doesnt seem right.

Thanks again.

@nlohmann
Copy link
Owner

So you would like to have a function like

bool json_pointer::is_valid(const json& j) const

returning true iff the JSON Pointer is valid for JSON value j? And this should work without exceptions under the hood?

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label Dec 29, 2017
@ctubio
Copy link

ctubio commented Jan 1, 2018

not sure if helps, but what i do to avoid value() to throw an exception is:

            for (json::iterator it = j.begin(); it != j.end();)
              if (it.value().is_null()) it = j.erase(it); else ++it;

then when the object is clean of null values value() returns the default value (2nd param) if key is not present (i dislike try catch blocks :P)

@stale
Copy link

stale bot commented Feb 1, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Feb 1, 2018
@stale stale bot closed this as completed Feb 8, 2018
@tyhenry
Copy link

tyhenry commented Mar 4, 2019

wondering what the state of this issue is currently?
It would be great to have a value() method that has a no throw guarantee, and always returns the default value on failure. This would remove a lot of boilerplate for config files.
Maybe the method signature could be changed to value(string key, T default, bool exception=true) , where exception=false guarantees no-throw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: needs more info the author of the issue needs to provide more details state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

5 participants