Merge pull request #70 from Magisus/exception-improvement/HC-84

(HC-84) Improve exception messages from config API

lib/inc/hocon/config.hpp

@@ -668,7 +668,6 @@ namespace hocon {
         config(shared_object object);
 
         static shared_object env_variables_as_config_object();
-        static not_resolved_exception improve_not_resolved(path what, not_resolved_exception const& original);
 
     protected:
         shared_value find(std::string const& path_expression, config_value::type expected) const;

lib/inc/hocon/config_exception.hpp

@@ -45,9 +45,9 @@ namespace hocon {
      * was set to null.
      */
     struct null_exception : public missing_exception {
-        null_exception(config_origin const& origin, std::string const& path, std::string const& expected) :
-                missing_exception(origin, (expected.empty() ? "Configuration key '" + path + "' is null"
-                                                            : "Configuration key '" + path + "' is set to null but expected " + expected)) { }
+        null_exception(config_origin const& origin, std::string const& path, std::string const& expected = "") :
+                missing_exception(origin, (expected.empty() ? "Configuration key \"" + path + "\" is null"
+                                                            : "Configuration key \"" + path + "\" is set to null but expected " + expected)) { }
     };
 
     /**

lib/inc/internal/resolve_source.hpp

@@ -4,6 +4,7 @@
 #include <list>
 #include <hocon/types.hpp>
 #include <internal/resolve_result.hpp>
+#include <hocon/config_exception.hpp>
 
 namespace hocon {
 
@@ -45,6 +46,7 @@ namespace hocon {
         static value_with_path find_in_object(shared_object obj, path the_path);
         static result_with_path find_in_object(shared_object obj, resolve_context context, path the_path);
         static value_with_path find_in_object(shared_object obj, path the_path, node parents);
+        static not_resolved_exception improve_not_resolved(path what, not_resolved_exception const& original);
 
         shared_object root_must_be_obj(std::shared_ptr<const container> value) const;
 

lib/src/config.cc

@@ -75,7 +75,11 @@ namespace hocon {
         try {
             peeked = _object->peek_path(raw_path);
         } catch (config_exception& ex) {
-            throw config_exception(raw_path.render() + " has not been resolved, you need to call config::resolve()");
+            if (_object->get_resolve_status() == resolve_status::RESOLVED) {
+                throw ex;
+            }
+            throw config_exception(
+                    raw_path.render() + " has not been resolved, you need to call config::resolve()");
         }
         return peeked;
     }
@@ -121,7 +125,8 @@ namespace hocon {
 
     shared_value config::throw_if_null(shared_value v, config_value::type expected, path original_path) {
         if (v->value_type() == config_value::type::CONFIG_NULL) {
-            throw config_exception(original_path.render() + " was null");
+            // TODO Once we decide on a way of converting the type enum to a string, pass expected type string
+            throw null_exception(*(v->origin()), original_path.render());
         } else {
             return v;
         }
@@ -136,7 +141,7 @@ namespace hocon {
                                                  path original_path) {
         shared_value v = self->peek_assuming_resolved(key, original_path);
         if (!v) {
-            throw config_exception("Value missing at " + original_path.render());
+            throw missing_exception(original_path.render());
         }
 
         if (expected != config_value::type::UNSPECIFIED) {
@@ -146,7 +151,7 @@ namespace hocon {
         if (expected != config_value::type::UNSPECIFIED &&
                 v->value_type() != expected &&
                 v->value_type() != config_value::type::CONFIG_NULL) {
-            throw config_exception(original_path.render() + " could not be converted to the requested type");
+            throw wrong_type_exception(original_path.render() + " could not be converted to the requested type");
         } else {
             return v;
         }
@@ -166,6 +171,9 @@ namespace hocon {
                 return find_or_null(o, next, expected, original_path);
             }
         } catch (config_exception& ex) {
+            if (self->get_resolve_status() == resolve_status::RESOLVED) {
+                throw ex;
+            }
             throw config_exception(desired_path.render() +
                                            " has not been resolved, you need to call config::resolve()");
         }
@@ -377,14 +385,5 @@ namespace hocon {
         return make_shared<simple_config_object>(origin, move(values), resolve_status::RESOLVED, false);
     }
 
-    not_resolved_exception config::improve_not_resolved(path what, not_resolved_exception const& original) {
-        string new_message = what.render() + " has not been resolved, you need to call config::resolve()"
-                                           + " see API docs for config::resolve()";
-        if (new_message == original.what()) {
-            return original;
-        } else {
-            return not_resolved_exception(new_message);
-        }
-    }
 
 }  // namespace hocon

lib/src/resolve_source.cc

@@ -117,11 +117,21 @@ namespace hocon {
         }
     }
 
+    not_resolved_exception resolve_source::improve_not_resolved(path what, not_resolved_exception const& original) {
+        string new_message = what.render() + " has not been resolved, you need to call config::resolve()"
+                             + " see API docs for config::resolve()";
+        if (new_message == original.what()) {
+            return original;
+        } else {
+            return not_resolved_exception(new_message);
+        }
+    }
+
     resolve_source::value_with_path resolve_source::find_in_object(shared_object obj, path the_path) {
         try {
             return find_in_object(obj, the_path, {});
         } catch (const not_resolved_exception &e) {
-            throw config::improve_not_resolved(move(the_path), e);
+            throw improve_not_resolved(move(the_path), e);
         }
     }
 

lib/tests/config_test.cc

@@ -2,6 +2,7 @@
 
 #include <hocon/config.hpp>
 #include "fixtures.hpp"
+#include "test_utils.hpp"
 
 using namespace std;
 using namespace hocon;
@@ -26,7 +27,7 @@ TEST_CASE("should be able to get bare C++ types from a config", "[config]") {
     SECTION("get list values") {
         auto conf = config::parse_file_any_syntax(TEST_FILE_DIR + string("/fixtures/test01.conf"))->resolve();
 
-        vector<int> ints { 1,2,3 };
+        vector<int> ints { 1, 2, 3 };
         REQUIRE(ints == conf->get_int_list("arrays.ofInt"));
 
         vector<int64_t> longs { 1, 2, 3 };
@@ -47,4 +48,68 @@ TEST_CASE("should be able to get bare C++ types from a config", "[config]") {
         auto string_conf = config::parse_string(bad_list);
         REQUIRE_THROWS(string_conf->get_int_list("bad"));
     };
-}
+}
+
+TEST_CASE("correct exceptions should be thrown", "[config]") {
+    SECTION("missing exception should be thrown when the value is not in the config") {
+        bool thrown = false;
+        try {
+            auto conf = config::parse_file_any_syntax(TEST_FILE_DIR + string("/fixtures/test01.conf"))->resolve();
+            conf->get_int("badSetting");
+        } catch (const config_exception& e) {
+            thrown = true;
+            REQUIRE_STRING_CONTAINS(e.what(), "No configuration setting found");
+            REQUIRE_STRING_CONTAINS(e.what(), "badSetting");
+        }
+        REQUIRE(thrown);
+    }
+
+    SECTION("a missing exception should be thrown when single-quoted strings are queried incorrectly") {
+        bool thrown = false;
+        try {
+            auto conf = config::parse_string("object : { 'key' : value }");
+            conf->get_string("key");
+        } catch (const config_exception& e) {
+            thrown = true;
+            REQUIRE_STRING_CONTAINS(e.what(), "No configuration setting found");
+            REQUIRE_STRING_CONTAINS(e.what(), "'key'");
+        }
+        REQUIRE(thrown);
+    }
+
+    SECTION("null exception should be thrown when a null value is queried with a different type") {
+        bool thrown = false;
+        try {
+            auto conf = config::parse_string("object : null");
+            conf->get_int("object");
+        } catch (const config_exception& e) {
+            thrown = true;
+            REQUIRE_STRING_CONTAINS(e.what(), "Configuration key \"object\" is null");
+        }
+        REQUIRE(thrown);
+    }
+
+    SECTION("wrong type exception should be thrown when a value cannot be converted to the queried type") {
+        bool thrown = false;
+        try {
+            auto conf = config::parse_string("object : { key : value }");
+            conf->get_string("object");
+        } catch (const config_exception& e) {
+            thrown = true;
+            REQUIRE_STRING_CONTAINS(e.what(), "object could not be converted");
+        }
+        REQUIRE(thrown);
+    }
+
+    SECTION("not resolved error message should display when config has not been resolved") {
+        bool thrown = false;
+        try {
+            auto conf = config::parse_string("a : b\nc : ${a}");
+            conf->get_string("c");
+        } catch (const config_exception& e) {
+            thrown = true;
+            REQUIRE_STRING_CONTAINS(e.what(), "c has not been resolved");
+        }
+        REQUIRE(thrown);
+    }
+}