Display meaningful errors when JSON types don't match (#7241)

This pull request completes (and somewhat rewrites) the JsonUtils error
handling arc. Deserialization errors, no longer represented by trees of
exceptions that must be rethrown and caught, are now transformed at
catch time into a message explaining what we expected and where we
expected it.

Instead of exception trees, a deserialization failure will result in a
single type of exception with the originating JSON object from which we
can determine the contents and location of the failure.

Because most of the error message actually comes from the JSON schema
or the actual supported types, and the other jsoncpp errors are not
localized I've made the decision to **not** localize these messages.
This commit is contained in:
Dustin L. Howett 2020-08-11 12:50:13 -07:00 committed by GitHub
parent b07c1e49da
commit 7ccd1f6f1a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 193 additions and 84 deletions

View File

@ -632,6 +632,12 @@ namespace winrt::TerminalApp::implementation
hr = E_INVALIDARG;
_settingsLoadExceptionText = _GetErrorText(ex.Error());
}
catch (const ::TerminalApp::SettingsTypedDeserializationException& e)
{
hr = E_INVALIDARG;
std::string_view what{ e.what() };
_settingsLoadExceptionText = til::u8u16(what);
}
catch (...)
{
hr = wil::ResultFromCaughtException();

View File

@ -39,9 +39,17 @@ namespace TerminalAppUnitTests
namespace TerminalApp
{
class SettingsTypedDeserializationException;
class CascadiaSettings;
};
class TerminalApp::SettingsTypedDeserializationException final : public std::runtime_error
{
public:
SettingsTypedDeserializationException(const std::string_view description) :
runtime_error(description.data()) {}
};
class TerminalApp::CascadiaSettings final
{
public:

View File

@ -42,6 +42,54 @@ static constexpr std::string_view Utf8Bom{ u8"\uFEFF" };
static constexpr std::string_view SettingsSchemaFragment{ "\n"
R"( "$schema": "https://aka.ms/terminal-profiles-schema")" };
static std::tuple<size_t, size_t> _LineAndColumnFromPosition(const std::string_view string, ptrdiff_t position)
{
size_t line = 1, column = position + 1;
auto lastNL = string.find_last_of('\n', position);
if (lastNL != std::string::npos)
{
column = (position - lastNL);
line = std::count(string.cbegin(), string.cbegin() + lastNL + 1, '\n') + 1;
}
return { line, column };
}
static void _CatchRethrowSerializationExceptionWithLocationInfo(std::string_view settingsString)
{
std::string msg;
try
{
throw;
}
catch (const JsonUtils::DeserializationError& e)
{
static constexpr std::string_view basicHeader{ "* Line {line}, Column {column}\n{message}" };
static constexpr std::string_view keyedHeader{ "* Line {line}, Column {column} ({key})\n{message}" };
std::string jsonValueAsString{ "array or object" };
try
{
jsonValueAsString = e.jsonValue.asString();
}
catch (...)
{
// discard: we're in the middle of error handling
}
msg = fmt::format(" Have: \"{}\"\n Expected: {}", jsonValueAsString, e.expectedType);
auto [l, c] = _LineAndColumnFromPosition(settingsString, e.jsonValue.getOffsetStart());
msg = fmt::format((e.key ? keyedHeader : basicHeader),
fmt::arg("line", l),
fmt::arg("column", c),
fmt::arg("key", e.key.value_or("")),
fmt::arg("message", msg));
throw SettingsTypedDeserializationException{ msg };
}
}
// Method Description:
// - Creates a CascadiaSettings from whatever's saved on disk, or instantiates
// a new one with the default values. If we're running as a packaged app,
@ -90,16 +138,23 @@ std::unique_ptr<CascadiaSettings> CascadiaSettings::LoadAll()
needToWriteFile = true;
}
// See microsoft/terminal#2325: find the defaultSettings from the user's
// settings. Layer those settings upon all the existing profiles we have
// (defaults and dynamic profiles). We'll also set
// _userDefaultProfileSettings here. When we LayerJson below to apply the
// user settings, we'll make sure to use these defaultSettings _before_ any
// profiles the user might have.
resultPtr->_ApplyDefaultsFromUserSettings();
try
{
// See microsoft/terminal#2325: find the defaultSettings from the user's
// settings. Layer those settings upon all the existing profiles we have
// (defaults and dynamic profiles). We'll also set
// _userDefaultProfileSettings here. When we LayerJson below to apply the
// user settings, we'll make sure to use these defaultSettings _before_ any
// profiles the user might have.
resultPtr->_ApplyDefaultsFromUserSettings();
// Apply the user's settings
resultPtr->LayerJson(resultPtr->_userSettings);
// Apply the user's settings
resultPtr->LayerJson(resultPtr->_userSettings);
}
catch (...)
{
_CatchRethrowSerializationExceptionWithLocationInfo(resultPtr->_userSettingsString);
}
// After layering the user settings, check if there are any new profiles
// that need to be inserted into their user settings file.

View File

@ -69,52 +69,24 @@ namespace TerminalApp::JsonUtils
};
}
// These exceptions cannot use localized messages, as we do not have
// guaranteed access to the resource loader.
class TypeMismatchException : public std::runtime_error
class DeserializationError : public std::runtime_error
{
public:
TypeMismatchException() :
runtime_error("unexpected data type") {}
};
DeserializationError(const Json::Value& value) :
runtime_error("failed to deserialize"),
jsonValue{ value } {}
class KeyedException : public std::runtime_error
{
public:
KeyedException(const std::string_view key, std::exception_ptr exception) :
runtime_error(fmt::format("error parsing \"{0}\"", key).c_str()),
_key{ key },
_innerException{ std::move(exception) } {}
std::string GetKey() const
void SetKey(std::string_view newKey)
{
return _key;
if (!key)
{
key = newKey;
}
}
[[noreturn]] void RethrowInner() const
{
std::rethrow_exception(_innerException);
}
private:
std::string _key;
std::exception_ptr _innerException;
};
class UnexpectedValueException : public std::runtime_error
{
public:
UnexpectedValueException(const std::string_view value) :
runtime_error(fmt::format("unexpected value \"{0}\"", value).c_str()),
_value{ value } {}
std::string GetValue() const
{
return _value;
}
private:
std::string _value;
std::optional<std::string> key;
Json::Value jsonValue;
std::string expectedType;
};
template<typename T>
@ -123,6 +95,8 @@ namespace TerminalApp::JsonUtils
// Forward-declare these so the linker can pick up specializations from elsewhere!
T FromJson(const Json::Value&);
bool CanConvert(const Json::Value& json);
std::string TypeDescription() const { return "<unknown>"; }
};
template<>
@ -137,6 +111,11 @@ namespace TerminalApp::JsonUtils
{
return json.isString();
}
std::string TypeDescription() const
{
return "string";
}
};
template<>
@ -151,6 +130,11 @@ namespace TerminalApp::JsonUtils
{
return json.isString();
}
std::string TypeDescription() const
{
return "string";
}
};
#ifdef WINRT_BASE_H
@ -177,6 +161,11 @@ namespace TerminalApp::JsonUtils
{
return json.isBool();
}
std::string TypeDescription() const
{
return "true | false";
}
};
template<>
@ -191,6 +180,11 @@ namespace TerminalApp::JsonUtils
{
return json.isInt();
}
std::string TypeDescription() const
{
return "number";
}
};
template<>
@ -205,6 +199,11 @@ namespace TerminalApp::JsonUtils
{
return json.isUInt();
}
std::string TypeDescription() const
{
return "number (>= 0)";
}
};
template<>
@ -219,6 +218,11 @@ namespace TerminalApp::JsonUtils
{
return json.isNumeric();
}
std::string TypeDescription() const
{
return "number";
}
};
template<>
@ -233,6 +237,11 @@ namespace TerminalApp::JsonUtils
{
return json.isNumeric();
}
std::string TypeDescription() const
{
return "number";
}
};
template<>
@ -253,6 +262,11 @@ namespace TerminalApp::JsonUtils
const auto string{ Detail::GetStringView(json) };
return string.length() == 38 && string.front() == '{' && string.back() == '}';
}
std::string TypeDescription() const
{
return "guid";
}
};
// (GUID and winrt::guid are mutually convertible!)
@ -279,6 +293,11 @@ namespace TerminalApp::JsonUtils
const auto string{ Detail::GetStringView(json) };
return (string.length() == 7 || string.length() == 4) && string.front() == '#';
}
std::string TypeDescription() const
{
return "color (#rrggbb, #rgb)";
}
};
template<typename T, typename TBase>
@ -298,13 +317,22 @@ namespace TerminalApp::JsonUtils
}
}
throw UnexpectedValueException{ name };
DeserializationError e{ json };
e.expectedType = TypeDescription();
throw e;
}
bool CanConvert(const Json::Value& json)
{
return json.isString();
}
std::string TypeDescription() const
{
std::vector<std::string_view> names;
std::transform(TBase::mappings.cbegin(), TBase::mappings.cend(), std::back_inserter(names), [](auto&& p) { return p.first; });
return fmt::format("{}", fmt::join(names, " | "));
}
};
// FlagMapper is EnumMapper, but it works for bitfields.
@ -343,7 +371,9 @@ namespace TerminalApp::JsonUtils
(value == AllClear && newFlag != AllClear)))
{
// attempt to combine AllClear (explicitly) with anything else
throw UnexpectedValueException{ element.asString() };
DeserializationError e{ element };
e.expectedType = TypeDescription();
throw e;
}
value |= newFlag;
}
@ -377,6 +407,11 @@ namespace TerminalApp::JsonUtils
{
return true;
}
std::string TypeDescription() const
{
return "any";
}
};
// Method Description:
@ -407,7 +442,9 @@ namespace TerminalApp::JsonUtils
{
if (!conv.CanConvert(json))
{
throw TypeMismatchException{};
DeserializationError e{ json };
e.expectedType = conv.TypeDescription();
throw e;
}
target = conv.FromJson(json);
@ -435,10 +472,10 @@ namespace TerminalApp::JsonUtils
{
return GetValue(*found, target, std::forward<Converter>(conv));
}
catch (...)
catch (DeserializationError& e)
{
// Wrap any caught exceptions in one that preserves context.
throw KeyedException(key, std::current_exception());
e.SetKey(key);
throw; // rethrow now that it has a key
}
}
return false;

View File

@ -101,6 +101,8 @@ JSON_ENUM_MAPPER(::TerminalApp::CloseOnExitMode)
{
return EnumMapper::CanConvert(json) || json.isBool();
}
using EnumMapper::TypeDescription;
};
// This specialization isn't using JSON_ENUM_MAPPER because we need to have a different
@ -151,6 +153,8 @@ struct ::TerminalApp::JsonUtils::ConversionTrait<::winrt::Windows::UI::Text::Fon
{
return BaseEnumMapper::CanConvert(json) || json.isUInt();
}
using EnumMapper::TypeDescription;
};
JSON_ENUM_MAPPER(::winrt::Windows::UI::Xaml::ElementTheme)
@ -231,6 +235,11 @@ struct ::TerminalApp::JsonUtils::ConversionTrait<::TerminalApp::LaunchPosition>
{
return json.isString();
}
std::string TypeDescription() const
{
return "x, y";
}
};
// Possible Direction values

View File

@ -33,6 +33,8 @@ struct ConversionTrait<StructWithConverterSpecialization>
{
return value.isInt();
}
std::string TypeDescription() const { return ""; }
};
// Converts a JSON string value to an int and multiplies it by a specified factor
@ -49,6 +51,8 @@ struct CustomConverter
{
return true;
}
std::string TypeDescription() const { return ""; }
};
enum class JsonTestEnum : int
@ -130,7 +134,7 @@ namespace TerminalAppUnitTests
//// 1. Bare Value ////
//// 1.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValue<int>(object), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<int>(object), DeserializationError, _ReturnTrueForException);
//// 1.b. JSON NULL - Zero Value ////
std::string zeroValueString{};
@ -141,7 +145,7 @@ namespace TerminalAppUnitTests
//// 2. Optional ////
//// 2.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValue<std::optional<int>>(object), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<std::optional<int>>(object), DeserializationError, _ReturnTrueForException);
//// 2.b. JSON NULL - nullopt ////
VERIFY_ARE_EQUAL(std::nullopt, GetValue<std::optional<std::string>>(Json::Value::nullSingleton()));
@ -160,7 +164,7 @@ namespace TerminalAppUnitTests
std::string output{ "sentinel" }; // explicitly not the zero value
//// 1.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValue(object, outputRedHerring), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue(object, outputRedHerring), DeserializationError, _ReturnTrueForException);
VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged
//// 1.b. JSON NULL - Unchanged ////
@ -176,7 +180,7 @@ namespace TerminalAppUnitTests
std::optional<std::string> optionalOutput{ "sentinel2" }; // explicitly not nullopt
//// 2.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValue(object, optionalOutputRedHerring), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue(object, optionalOutputRedHerring), DeserializationError, _ReturnTrueForException);
VERIFY_ARE_EQUAL(6, optionalOutputRedHerring); // unchanged
//// 2.b. JSON NULL - nullopt ////
@ -202,7 +206,7 @@ namespace TerminalAppUnitTests
//// 1. Bare Value ////
//// 1.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValueForKey<int>(object, key), KeyedException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValueForKey<int>(object, key), DeserializationError, _ReturnTrueForException);
//// 1.b. JSON NULL - Zero Value ////
std::string zeroValueString{};
@ -216,7 +220,7 @@ namespace TerminalAppUnitTests
//// 2. Optional ////
//// 2.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValueForKey<std::optional<int>>(object, key), KeyedException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValueForKey<std::optional<int>>(object, key), DeserializationError, _ReturnTrueForException);
//// 2.b. JSON NULL - nullopt ////
VERIFY_ARE_EQUAL(std::nullopt, GetValueForKey<std::optional<std::string>>(object, nullKey));
@ -245,7 +249,7 @@ namespace TerminalAppUnitTests
std::string output{ "sentinel" }; // explicitly not the zero value
//// 1.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key, outputRedHerring), KeyedException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key, outputRedHerring), DeserializationError, _ReturnTrueForException);
VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged
//// 1.b. JSON NULL - Unchanged ////
@ -267,7 +271,7 @@ namespace TerminalAppUnitTests
std::optional<std::string> optionalOutput{ "sentinel2" }; // explicitly not nullopt
//// 2.a. Type Invalid - Exception ////
VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key, optionalOutputRedHerring), KeyedException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key, optionalOutputRedHerring), DeserializationError, _ReturnTrueForException);
VERIFY_ARE_EQUAL(6, optionalOutputRedHerring); // unchanged
//// 2.b. JSON NULL - nullopt ////
@ -321,12 +325,12 @@ namespace TerminalAppUnitTests
TryBasicType(testGuid, testGuidString);
VERIFY_THROWS_SPECIFIC(GetValue<GUID>({ "NOT_A_GUID" }), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<GUID>({ "{too short for a guid but just a bit}" }), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<GUID>({ "NOT_A_GUID" }), DeserializationError, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<GUID>({ "{too short for a guid but just a bit}" }), DeserializationError, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<GUID>({ "{proper length string not a guid tho?}" }), std::exception, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<til::color>({ "#" }), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<til::color>({ "#1234567890" }), TypeMismatchException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<til::color>({ "#" }), DeserializationError, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<til::color>({ "#1234567890" }), DeserializationError, _ReturnTrueForException);
}
void JsonUtilsTests::BasicTypeWithCustomConverter()
@ -366,7 +370,7 @@ namespace TerminalAppUnitTests
// Unknown value should produce something?
Json::Value stringUnknown{ "unknown" };
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestEnum>(stringUnknown), UnexpectedValueException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestEnum>(stringUnknown), DeserializationError, _ReturnTrueForException);
}
void JsonUtilsTests::FlagMapper()
@ -401,17 +405,17 @@ namespace TerminalAppUnitTests
Json::Value arrayNoneFirst{ Json::arrayValue };
arrayNoneFirst.append({ "none" });
arrayNoneFirst.append({ "first" });
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(arrayNoneFirst), UnexpectedValueException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(arrayNoneFirst), DeserializationError, _ReturnTrueForException);
// Stacking Any + None (Exception; same as above, different order)
Json::Value arrayFirstNone{ Json::arrayValue };
arrayFirstNone.append({ "first" });
arrayFirstNone.append({ "none" });
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(arrayFirstNone), UnexpectedValueException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(arrayFirstNone), DeserializationError, _ReturnTrueForException);
// Unknown flag value?
Json::Value stringUnknown{ "unknown" };
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(stringUnknown), UnexpectedValueException, _ReturnTrueForException);
VERIFY_THROWS_SPECIFIC(GetValue<JsonTestFlags>(stringUnknown), DeserializationError, _ReturnTrueForException);
}
void JsonUtilsTests::NestedExceptionDuringKeyParse()
@ -420,20 +424,10 @@ namespace TerminalAppUnitTests
Json::Value object{ Json::objectValue };
object[key] = Json::Value{ "string" };
auto CheckKeyedException = [](const KeyedException& k) {
try
{
k.RethrowInner();
}
catch (TypeMismatchException&)
{
// This Keyed exception contained the correct inner.
return true;
}
// This Keyed exception did not contain the correct inner.
return false;
auto CheckKeyInException = [](const DeserializationError& k) {
return k.key.has_value();
};
VERIFY_THROWS_SPECIFIC(GetValueForKey<int>(object, key), KeyedException, CheckKeyedException);
VERIFY_THROWS_SPECIFIC(GetValueForKey<int>(object, key), DeserializationError, CheckKeyInException);
}
}