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

Use user-defined exceptions #244

Closed
nlohmann opened this issue May 13, 2016 · 13 comments
Closed

Use user-defined exceptions #244

nlohmann opened this issue May 13, 2016 · 13 comments

Comments

@nlohmann
Copy link
Owner

After a small survey on Reddit it seems to be a good idea to replace the standard exceptions used in the library right now with user-defined exceptions.

The motivations for this are:

  • Better differentiation between exceptions raised by the STL and those raised by the JSON library.
  • Clearer semantics of the exceptions rather and the possibility of better diagnosis and documentation.

In the course of this ticket, the list of currently used exceptions need to be classified into user-defined exception classes:

throw std::domain_error("JSON pointer has no parent");
throw std::domain_error("JSON pointer must be empty or begin with '/'");
throw std::domain_error("array index must not begin with '0'");
throw std::domain_error("cannot compare iterators of different containers");
throw std::domain_error("cannot compare order of object iterators");
throw std::domain_error("cannot create object from initializer list");
throw std::domain_error("cannot use at() with " + type_name());
throw std::domain_error("cannot use construct with iterators from " + first.m_object->type_name());
throw std::domain_error("cannot use erase() with " + type_name());
throw std::domain_error("cannot use insert() with " + type_name());
throw std::domain_error("cannot use key() for non-object iterators");
throw std::domain_error("cannot use offsets with object iterators");
throw std::domain_error("cannot use operator[] for object iterators");
throw std::domain_error("cannot use operator[] with " + type_name());
throw std::domain_error("cannot use push_back() with " + type_name());
throw std::domain_error("cannot use swap() with " + type_name());
throw std::domain_error("cannot use value() with " + type_name());
throw std::domain_error("escape error: '~' must be followed with '0' or '1'");
throw std::domain_error("incompatible ReferenceType for get_ref, actual type is " +
throw std::domain_error("invalid value to unflatten");
throw std::domain_error("iterator does not fit current value");
throw std::domain_error("iterators are not compatible");
throw std::domain_error("iterators do not fit current value");
throw std::domain_error("iterators do not fit");
throw std::domain_error("only objects can be unflattened");
throw std::domain_error("passed iterators may not belong to container");
throw std::domain_error("type must be array, but is " + type_name());
throw std::domain_error("type must be number, but is " + type_name());
throw std::domain_error("type must be object, but is " + type_name());
throw std::domain_error("type must be string, but is " + type_name());
throw std::domain_error("unsuccessful: " + val.dump());
throw std::domain_error("values in object must be primitive");
throw std::invalid_argument("JSON patch must be an array of objects");
throw std::invalid_argument("missing low surrogate");
throw std::invalid_argument("missing or wrong low surrogate");
throw std::invalid_argument("operation value '" + op + "' is invalid");
throw std::invalid_argument(error_msg + " must have member '" + member + "'");
throw std::invalid_argument(error_msg + " must have string member '" + member + "'");
throw std::invalid_argument(error_msg);
throw std::out_of_range("array index " + std::to_string(idx) + " is out of range");
throw std::out_of_range("array index '-' (" +
throw std::out_of_range("cannot get value");
throw std::out_of_range("code points above 0x10FFFF are invalid");
throw std::out_of_range("iterator out of range");
throw std::out_of_range("iterators out of range");
throw std::out_of_range("key '" + key + "' not found");
throw std::out_of_range("key '" + last_path + "' not found");
throw std::out_of_range("unresolved reference token '" + reference_token + "'");

Furthermore, STL exceptions need to be caught and rethrown as user-defined exception in several cases.

@nlohmann nlohmann added improvement state: please discuss please discuss the issue or vote for your favorite option labels May 13, 2016
@whackashoe
Copy link
Contributor

For simplicity's sake I think it is much easier to deal with libs that have fewer rather than more different types of exceptions (as long as there isn't weird overlaps). So, here's a few suggestions on how that could be done:


throw std::domain_error("type must be array, but is " + type_name());
throw std::domain_error("type must be number, but is " + type_name());
throw std::domain_error("type must be object, but is " + type_name());
throw std::domain_error("type must be string, but is " + type_name());

These could be type_error


throw std::domain_error("iterator does not fit current value");
throw std::domain_error("iterators are not compatible");
throw std::domain_error("iterators do not fit current value");
throw std::domain_error("iterators do not fit");
throw std::out_of_range("iterator out of range");
throw std::out_of_range("iterators out of range");

Could be invalid_iterator


throw std::domain_error("cannot use at() with " + type_name());
throw std::domain_error("cannot use construct with iterators from " + first.m_object->type_name());
throw std::domain_error("cannot use erase() with " + type_name());
throw std::domain_error("cannot use insert() with " + type_name());
throw std::domain_error("cannot use key() for non-object iterators");
throw std::domain_error("cannot use offsets with object iterators");
throw std::domain_error("cannot use operator[] for object iterators");
throw std::domain_error("cannot use operator[] with " + type_name());
throw std::domain_error("cannot use push_back() with " + type_name());
throw std::domain_error("cannot use swap() with " + type_name());
throw std::domain_error("cannot use value() with " + type_name());


Could be not_supported or invalid_action or invalid_method


throw std::out_of_range("key '" + key + "' not found");
throw std::out_of_range("key '" + last_path + "' not found");

Could be lookup_error or key_error

Anyways, this way there doesn't need to be this large tree of possible exceptions to keep in head if they are grouped a bit.

@gregmarr
Copy link
Contributor

I'd recommend that if you add multiple exception classes derived from any std::exception related class, that you have your own base class first, so that they can all be handled at once if the user desires.

@nlohmann nlohmann modified the milestones: Release 2.1.0, Release 3.0.0 Jun 19, 2016
@nlohmann nlohmann self-assigned this Jul 3, 2016
@nlohmann
Copy link
Owner Author

nlohmann commented Jul 3, 2016

I started a feature branch for user-defined exceptions. There is now a class basic_json::exception with a subclass invalid_iterator which is used in all iterator-related scenarios. The user-defined exception class uses an integer error code to facility better documentation of exceptions - see Doxygen-doucumentation for class basic_json::exception.

I shall replace more standard exceptions in the next days. I would be happy for comments!

@markand
Copy link

markand commented Aug 8, 2016

@whackashoe there are various reasons why we should add own exceptions. For example when you access a property that does not exist or is it invalid, you can throw an exception like missing_property or invalid_property where you can bundle the property name which is valuable for the user of the library.

The general error message that comes with what() is always handy, but sometimes you want more information like the line number in a file, the column, many things like that and exceptions are just the way to go for this purpose.

@ghost
Copy link

ghost commented Oct 15, 2016

@nlohmann I'm a new user of your library. Great job. There are couple of suggestions that I would like to make regarding exception handling.

In the constructor of exception class, you prepend formatted error code to the exception message. Would you consider not doing that? Since this information is available in the exception handler, the user can decide whether to output the error code or not.

Another suggestion that I would like to make is to avoid using string formatting in order to splice extra info in the error message. The reason is that once part of the message, this information cannot be accessed programmatically. In @markand's example above, given the missing property name, the user of the library can decide what info should flow upstream (e.g. message to the user), and how to format it. If the name of the property is part of the message, the user of the library has only two sub-optimal choices: avoid passing this information upstream, or displaying a message beyond user's control.

Also in many cases this extra info in the exception itself is not really necessary: the info is already in scope where the exception should be handled. Last point, however, is arguable. Some people might say that it's okay to let the exception from this library ripple all the way to some high level exception handler where the specific type may not be known.

Finally, one place where I fould some inspiration regarding exception handling is POCO. I think their solution is pretty neat and I would like to recommend you have a look.

@nlohmann
Copy link
Owner Author

@krzysztofwos, thanks for the suggestions. Here are my thoughts:

  • I also think it is a good idea to also provide certain information as members of the exception object rather than just inside the what() string.
  • As a consequence, I would leave the what() string rather general which simplifies the code. The in-depth explanation can then be moved to the documentation. (Then again, do we actually need a what() string?) At the same time, users of the library are free to use as much additional information to compose their own diagnostic messages.
  • One could even think about using a single basic_json variable to hold all information on the exception. What do you think? Is it a good idea to create exception objects that again rely on dynamic memory?

@ghost
Copy link

ghost commented Oct 16, 2016

I think that what() string is useful insofar as if an exception escapes and gets caught by some generic handler up the stack, you get at least some indication of what the error was. But I think that in a well-structured application you should not let exceptions escape so that the developer building on top of json will almost certainly want to throw his own errors.

For example, my use case is a (surprise!) REST API. Any json exceptions will be re-thrown as BadRequest or its derivatives so that handler wrapper can generate an appropriate response. N.b. this is where user-defined exceptions are really necessary: without this change I would not be able to distinguish (without parsing the what() string) whether it's a bad request, or internal server error (an exception in the handler code). For me, what() string won't be of much use.

Regarding your last point, the idea is a bit too self-referential. I think that the content of an exception should be defined statically, and a JSON object certainly isn't. You can document all the properties that it would have, but then what if someone makes a typo accessing a property in the handler for json::exception? kaboom! 3 years down the line in production during critical moment :) As far as dynamic memory is concerned, I think in a high-level API like this, it's perfectly fine.

@gregmarr
Copy link
Contributor

As long as it's not due to an out of memory condition, then you should be okay with dynamic memory allocation.

@nlohmann
Copy link
Owner Author

The user-defined exceptions are done now, see the new feature branch. This branch also contains the option to switch off assertions (#296).

@jlisee
Copy link

jlisee commented Feb 15, 2017

This is really cool to see, do you have any idea when you would be merging this in?

@nlohmann
Copy link
Owner Author

Not really. There are still 1-2 issues that need to be fixed before I can start with the 3.0.0 release.

nlohmann added a commit that referenced this issue Mar 1, 2017
Added class hierarchy for user-defined exceptions (#244). Integrated
parse exceptions 101-103. Parse exceptions include the byte count of
the last read character to locate the position of the error (#301).
@nlohmann
Copy link
Owner Author

nlohmann commented Mar 8, 2017

This issue is nearly complete. If anyone would like to have a look at the soon-to-be-merged state, please have a look at branch https://github.com/nlohmann/json/tree/feature/exceptions_3.0.0. It contains user-defined exceptions, and only the documentation needs some final touches.

nlohmann added a commit that referenced this issue Mar 12, 2017
- If an overflow occurs during parsing a number from a JSON text, an
exception (std::out_of_range for the moment, to be replaced by a
user-defined exception #244) is thrown so that the overflow is detected
early and roundtripping is guaranteed.
- NaN and INF floating-point values can be stored in a JSON value and
are not replaced by null. That is, the basic_json class behaves like
double in this regard (no exception occurs). However, NaN and INF are
serialized to “null”.
- Adjusted test cases appropriately.
@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: please discuss please discuss the issue or vote for your favorite option labels Mar 12, 2017
@nlohmann
Copy link
Owner Author

Merged this feature into develop. All there is left to do is to document the changes between old and new exceptions in the wiki page.

@nlohmann nlohmann removed the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Mar 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants