From 3a5c33b0056a4ff50812593c9f7a7ca9e1b6a564 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 9 Nov 2020 15:13:02 -0800 Subject: [PATCH] Rework JsonUtils' optional handling to let Converters see null (#8175) The JsonUtils changes in #8018 revealed that we need more robust, configurable optional handling. We learned that there's a class of values that was previously underrepresented in our API: _strings that have an explicit empty value_. The Settings model supports starting directory, icon, background image et al values that are empty. That emptiness _overrides_ a value set in a lower layer, so it is not sufficient to represent the empty value for any one of those fields as an unset optional. There are a couple other settings for which we've implemented a hand-rolled option type (for roughly the same reason): foreground, background, any color fields that override values from the color scheme _or_ the lower layer profile. These requirements are best fulfilled by better optional support in JsonUtils. Where the library would originally detect known types of optional and pre-filter them out during `GetValue` and `SetValue`, it will now defer to another conversion trait. This commit introduces a helper conversion trait and an "option oracle". The conversion trait will use the option oracle to detect emptiness, generate empty option values, and read values out of option types. In so doing, the trait is insulated from the implementation details of any specific option type. Any special logic for handling JSON null and option types has been stripped from GetValue. Due to this, there is an express change in behavior for some converters: * `GetValue(jsonNull)` where `T` is **not** an option type[1] has been upgraded from a silent no-op to an exception. Further, I took the opportunity to replace NullableSetting with std::optional>, which accurately represents "setting that the user might explicitly clear". I've added a test to JsonUtilsTests to make sure it can serialize/deserialize double optionals the way we expect it to. Tests (Local, Unit for TerminalApp/SettingsModel): Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0 [1]: Explicitly, if `T` is not an option type _and the converter does not support null_. --- .github/actions/spell-check/expect/expect.txt | 1 + doc/cascadia/Json-Utility-API.md | 17 +- .../LocalTests_SettingsModel/CommandTests.cpp | 13 +- .../KeyBindingsTests.cpp | 12 +- .../TerminalSettingsModel/IInheritable.h | 50 +++--- .../TerminalSettingsModel/JsonUtils.h | 164 ++++++++++++------ .../TerminalSettingsModel/Profile.cpp | 21 +-- src/cascadia/ut_app/JsonUtilsTests.cpp | 141 +++++++++++++-- 8 files changed, 279 insertions(+), 140 deletions(-) diff --git a/.github/actions/spell-check/expect/expect.txt b/.github/actions/spell-check/expect/expect.txt index f1513698f..55298a2bd 100644 --- a/.github/actions/spell-check/expect/expect.txt +++ b/.github/actions/spell-check/expect/expect.txt @@ -2277,6 +2277,7 @@ tcome tcommandline tcommands tcon +TDelegated TDP TEAMPROJECT tearoff diff --git a/doc/cascadia/Json-Utility-API.md b/doc/cascadia/Json-Utility-API.md index 465f3239a..422a550bf 100644 --- a/doc/cascadia/Json-Utility-API.md +++ b/doc/cascadia/Json-Utility-API.md @@ -8,19 +8,20 @@ return a JSON value coerced into the specified type. When reading into existing storage, it returns a boolean indicating whether that storage was modified. If the JSON value cannot be converted to the specified type, an exception will be generated. +For non-nullable type conversions (most POD types), `null` is considered to be an invalid type. ```c++ std::string one; std::optional two; JsonUtils::GetValue(json, one); -// one is populated or unchanged. +// one is populated or an exception is thrown. JsonUtils::GetValue(json, two); -// two is populated, nullopt or unchanged +// two is populated, nullopt or an exception is thrown auto three = JsonUtils::GetValue(json); -// three is populated or zero-initialized +// three is populated or an exception is thrown auto four = JsonUtils::GetValue>(json); // four is populated or nullopt @@ -225,14 +226,14 @@ auto v = JsonUtils::GetValue(json, conv); -|json type invalid|json null|valid -|-|-|- -`T`|❌ exception|🔵 unchanged|✔ converted +`T`|❌ exception|❌ exception|✔ converted `std::optional`|❌ exception|🟨 `nullopt`|✔ converted ### GetValue<T>() (returning) -|json type invalid|json null|valid -|-|-|- -`T`|❌ exception|🟨 `T{}` (zero value)|✔ converted +`T`|❌ exception|❌ exception|✔ converted `std::optional`|❌ exception|🟨 `nullopt`|✔ converted ### GetValueForKey(T&) (type-deducing) @@ -242,14 +243,14 @@ a "key not found" state. The remaining three cases are the same. val type|key not found|_json type invalid_|_json null_|_valid_ -|-|-|-|- -`T`|🔵 unchanged|_❌ exception_|_🔵 unchanged_|_✔ converted_ -`std::optional`|_🔵 unchanged_|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ +`T`|🔵 unchanged|_❌ exception_|_❌ exception_|_✔ converted_ +`std::optional`|🔵 unchanged|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ ### GetValueForKey<T>() (return value) val type|key not found|_json type invalid_|_json null_|_valid_ -|-|-|-|- -`T`|🟨 `T{}` (zero value)|_❌ exception_|_🟨 `T{}` (zero value)_|_✔ converted_ +`T`|🟨 `T{}` (zero value)|_❌ exception_|_❌ exception_|_✔ converted_ `std::optional`|🟨 `nullopt`|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ ### Future Direction diff --git a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp index b50dd6059..b3a0951f7 100644 --- a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp @@ -144,7 +144,6 @@ namespace SettingsModelLocalTests // of from keybindings. const std::string commands0String{ R"([ - { "name": "command0", "command": { "action": "splitPane", "split": null } }, { "name": "command1", "command": { "action": "splitPane", "split": "vertical" } }, { "name": "command2", "command": { "action": "splitPane", "split": "horizontal" } }, { "name": "command4", "command": { "action": "splitPane" } }, @@ -157,18 +156,8 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(0u, commands.Size()); auto warnings = implementation::Command::LayerJson(commands, commands0Json); VERIFY_ARE_EQUAL(0u, warnings.size()); - VERIFY_ARE_EQUAL(5u, commands.Size()); + VERIFY_ARE_EQUAL(4u, commands.Size()); - { - auto command = commands.Lookup(L"command0"); - VERIFY_IS_NOT_NULL(command); - VERIFY_IS_NOT_NULL(command.Action()); - VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action()); - const auto& realArgs = command.Action().Args().try_as(); - VERIFY_IS_NOT_NULL(realArgs); - // Verify the args have the expected value - VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle()); - } { auto command = commands.Lookup(L"command1"); VERIFY_IS_NOT_NULL(command); diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index 78ac3ada2..6f74fed4d 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -322,7 +322,6 @@ namespace SettingsModelLocalTests void KeyBindingsTests::TestSplitPaneArgs() { const std::string bindings0String{ R"([ - { "keys": ["ctrl+c"], "command": { "action": "splitPane", "split": null } }, { "keys": ["ctrl+d"], "command": { "action": "splitPane", "split": "vertical" } }, { "keys": ["ctrl+e"], "command": { "action": "splitPane", "split": "horizontal" } }, { "keys": ["ctrl+g"], "command": { "action": "splitPane" } }, @@ -335,17 +334,8 @@ namespace SettingsModelLocalTests VERIFY_IS_NOT_NULL(keymap); VERIFY_ARE_EQUAL(0u, keymap->_keyShortcuts.size()); keymap->LayerJson(bindings0Json); - VERIFY_ARE_EQUAL(5u, keymap->_keyShortcuts.size()); + VERIFY_ARE_EQUAL(4u, keymap->_keyShortcuts.size()); - { - KeyChord kc{ true, false, false, static_cast('C') }; - auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc); - VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action()); - const auto& realArgs = actionAndArgs.Args().try_as(); - VERIFY_IS_NOT_NULL(realArgs); - // Verify the args have the expected value - VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle()); - } { KeyChord kc{ true, false, false, static_cast('D') }; auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index c8c1ee1e4..539b21a54 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -72,11 +72,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // This is like std::optional, but we can use it in inheritance to determine whether the user explicitly cleared it template - struct NullableSetting - { - std::optional setting{ std::nullopt }; - bool set{ false }; - }; + using NullableSetting = std::optional>; } // Use this macro to quickly implement both getters and the setter for an @@ -93,7 +89,7 @@ public: \ bool Has##name() const \ { \ return _##name.has_value(); \ - }; \ + } \ \ /* Returns the resolved value for this setting */ \ /* fallback: user set value --> inherited value --> system set value */ \ @@ -101,19 +97,19 @@ public: \ { \ const auto val{ _get##name##Impl() }; \ return val ? *val : type{ __VA_ARGS__ }; \ - }; \ + } \ \ /* Overwrite the user set value */ \ void name(const type& value) \ { \ _##name = value; \ - }; \ + } \ \ /* Clear the user set value */ \ void Clear##name() \ { \ _##name = std::nullopt; \ - }; \ + } \ \ private: \ std::optional _##name{ std::nullopt }; \ @@ -137,7 +133,7 @@ private: \ \ /*no value was found*/ \ return std::nullopt; \ - }; + } // This macro is similar to the one above, but is reserved for optional settings // like Profile.Foreground (where null is interpreted @@ -148,51 +144,51 @@ public: \ /* Returns true if the user explicitly set the value, false otherwise*/ \ bool Has##name() const \ { \ - return _##name.set; \ - }; \ + return _##name.has_value(); \ + } \ \ /* Returns the resolved value for this setting */ \ /* fallback: user set value --> inherited value --> system set value */ \ winrt::Windows::Foundation::IReference name() const \ { \ const auto val{ _get##name##Impl() }; \ - if (val.set) \ + if (val) \ { \ - if (val.setting) \ + if (*val) \ { \ - return *val.setting; \ + return **val; \ } \ return nullptr; \ } \ return winrt::Windows::Foundation::IReference{ __VA_ARGS__ }; \ - }; \ + } \ \ /* Overwrite the user set value */ \ void name(const winrt::Windows::Foundation::IReference& value) \ { \ if (value) /*set value is different*/ \ { \ - _##name.setting = value.Value(); \ + _##name = std::optional{ value.Value() }; \ } \ else \ { \ - _##name.setting = std::nullopt; \ + /* note we're setting the _inner_ value */ \ + _##name = std::optional{ std::nullopt }; \ } \ - _##name.set = true; \ - }; \ + } \ \ /* Clear the user set value */ \ void Clear##name() \ { \ - _##name.set = false; \ - }; \ + _##name = std::nullopt; \ + } \ \ private: \ NullableSetting _##name{}; \ NullableSetting _get##name##Impl() const \ { \ /*return user set value*/ \ - if (Has##name()) \ + if (_##name) \ { \ return _##name; \ } \ @@ -201,12 +197,12 @@ private: \ /*iterate through parents to find a value*/ \ for (auto parent : _parents) \ { \ - auto val{ parent->_get##name##Impl() }; \ - if (val.set) \ + if (auto val{ parent->_get##name##Impl() }) \ { \ return val; \ } \ } \ + \ /*no value was found*/ \ - return { std::nullopt, false }; \ - }; + return std::nullopt; \ + } diff --git a/src/cascadia/TerminalSettingsModel/JsonUtils.h b/src/cascadia/TerminalSettingsModel/JsonUtils.h index 2ed3f1bde..13b77637a 100644 --- a/src/cascadia/TerminalSettingsModel/JsonUtils.h +++ b/src/cascadia/TerminalSettingsModel/JsonUtils.h @@ -60,36 +60,34 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils const std::string_view zeroCopyString{ begin, gsl::narrow_cast(end - begin) }; return zeroCopyString; } - - template - struct DeduceOptional - { - using Type = typename std::decay::type; - static constexpr bool IsOptional = false; - }; - - template - struct DeduceOptional> - { - using Type = typename std::decay::type; - static constexpr bool IsOptional = true; - }; - - template - struct DeduceOptional<::winrt::Windows::Foundation::IReference> - { - using Type = typename std::decay::type; - static constexpr bool IsOptional = true; - }; - - template<> - struct DeduceOptional<::winrt::hstring> - { - using Type = typename ::winrt::hstring; - static constexpr bool IsOptional = true; - }; } + template + struct OptionOracle + { + template // universal parameter + static constexpr bool HasValue(U&&) + { + return true; + } + }; + + template + struct OptionOracle> + { + static constexpr std::optional EmptyV() { return std::nullopt; } + static constexpr bool HasValue(const std::optional& o) { return o.has_value(); } + static constexpr auto&& Value(const std::optional& o) { return *o; } + }; + + template + struct OptionOracle<::winrt::Windows::Foundation::IReference> + { + static constexpr ::winrt::Windows::Foundation::IReference EmptyV() { return nullptr; } + static constexpr bool HasValue(const ::winrt::Windows::Foundation::IReference& o) { return static_cast(o); } + static constexpr auto&& Value(const ::winrt::Windows::Foundation::IReference& o) { return o.Value(); } + }; + class DeserializationError : public std::runtime_error { public: @@ -177,13 +175,27 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils // Leverage the wstring converter's validation winrt::hstring FromJson(const Json::Value& json) { + if (json.isNull()) + { + return winrt::hstring{}; + } return winrt::hstring{ til::u8u16(Detail::GetStringView(json)) }; } Json::Value ToJson(const winrt::hstring& val) { + if (val == winrt::hstring{}) + { + return Json::Value::nullSingleton(); + } return til::u16u8(val); } + + bool CanConvert(const Json::Value& json) + { + // hstring has a specific behavior for null, so it can convert it + return ConversionTrait::CanConvert(json) || json.isNull(); + } }; #endif @@ -419,6 +431,56 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils }; #endif + template::type>, typename TOpt = std::optional::type>> + struct OptionalConverter + { + using Oracle = OptionOracle; + TDelegatedConverter delegatedConverter{}; + + TOpt FromJson(const Json::Value& json) + { + if (!json && !delegatedConverter.CanConvert(json)) + { + // If the nested converter can't deal with null, emit an empty optional + // If it can, it probably has specific null behavior that it wants to use. + return Oracle::EmptyV(); + } + TOpt val{ delegatedConverter.FromJson(json) }; + return val; + } + + bool CanConvert(const Json::Value& json) + { + return json.isNull() || delegatedConverter.CanConvert(json); + } + + Json::Value ToJson(const TOpt& val) + { + if (!Oracle::HasValue(val)) + { + return Json::Value::nullSingleton(); + } + return delegatedConverter.ToJson(Oracle::Value(val)); + } + + std::string TypeDescription() const + { + return delegatedConverter.TypeDescription(); + } + }; + + template + struct ConversionTrait> : public OptionalConverter, std::optional> + { + }; + +#ifdef WINRT_Windows_Foundation_H + template + struct ConversionTrait<::winrt::Windows::Foundation::IReference> : public OptionalConverter, ::winrt::Windows::Foundation::IReference> + { + }; +#endif + template struct EnumMapper { @@ -586,31 +648,15 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValue(const Json::Value& json, T& target, Converter&& conv) { - if constexpr (Detail::DeduceOptional::IsOptional) + if (!conv.CanConvert(json)) { - // FOR OPTION TYPES - // - If the json object is set to `null`, then - // we'll instead set the target back to the empty optional. - if (json.isNull()) - { - target = T{}; // zero-construct an empty optional - return true; - } + DeserializationError e{ json }; + e.expectedType = conv.TypeDescription(); + throw e; } - if (json) - { - if (!conv.CanConvert(json)) - { - DeserializationError e{ json }; - e.expectedType = conv.TypeDescription(); - throw e; - } - - target = conv.FromJson(json); - return true; - } - return false; + target = conv.FromJson(json); + return true; } // GetValue, forced return type, manual converter @@ -654,7 +700,7 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValue(const Json::Value& json, T& target) { - return GetValue(json, target, ConversionTrait::Type>{}); + return GetValue(json, target, ConversionTrait::type>{}); } // GetValue, forced return type, with automatic converter @@ -662,7 +708,7 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils std::decay_t GetValue(const Json::Value& json) { std::decay_t local{}; - GetValue(json, local, ConversionTrait::Type>{}); + GetValue(json, local, ConversionTrait::type>{}); return local; // returns zero-initialized or value } @@ -670,14 +716,14 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValueForKey(const Json::Value& json, std::string_view key, T& target) { - return GetValueForKey(json, key, target, ConversionTrait::Type>{}); + return GetValueForKey(json, key, target, ConversionTrait::type>{}); } // GetValueForKey, forced return type, with automatic converter template std::decay_t GetValueForKey(const Json::Value& json, std::string_view key) { - return GetValueForKey(json, key, ConversionTrait::Type>{}); + return GetValueForKey(json, key, ConversionTrait::type>{}); } // Get multiple values for keys (json, k, &v, k, &v, k, &v, ...). @@ -696,15 +742,19 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template void SetValueForKey(Json::Value& json, std::string_view key, const T& target, Converter&& conv) { - // demand guarantees that it will return a value or throw an exception - *json.demand(&*key.cbegin(), (&*key.cbegin()) + key.size()) = conv.ToJson(target); + // We don't want to write any empty optionals into JSON (right now). + if (OptionOracle::HasValue(target)) + { + // demand guarantees that it will return a value or throw an exception + *json.demand(&*key.cbegin(), (&*key.cbegin()) + key.size()) = conv.ToJson(target); + } } // SetValueForKey, type-deduced, with automatic converter template void SetValueForKey(Json::Value& json, std::string_view key, const T& target) { - SetValueForKey(json, key, target, ConversionTrait::Type>{}); + SetValueForKey(json, key, target, ConversionTrait::type>{}); } }; diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index a38d40adc..7017f15e2 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -293,10 +293,10 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); // Core Settings - _Foreground.set = JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground.setting); - _Background.set = JsonUtils::GetValueForKey(json, BackgroundKey, _Background.setting); - _SelectionBackground.set = JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground.setting); - _CursorColor.set = JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor.setting); + JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); + JsonUtils::GetValueForKey(json, BackgroundKey, _Background); + JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); + JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); JsonUtils::GetValueForKey(json, ColorSchemeKey, _ColorSchemeName); // TODO:MSFT:20642297 - Use a sentinel value (-1) for "Infinite scrollback" @@ -320,18 +320,11 @@ void Profile::LayerJson(const Json::Value& json) // Padding was never specified as an integer, but it was a common working mistake. // Allow it to be permissive. - JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::PermissiveStringConverter{}); + JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::OptionalConverter>{}); JsonUtils::GetValueForKey(json, ScrollbarStateKey, _ScrollState); - // StartingDirectory is "nullable". But we represent a null starting directory as the empty string - // When null is set in the JSON, we empty initialize startDir (empty string), and set StartingDirectory to that - // Without this, we're accidentally setting StartingDirectory to nullopt instead. - hstring startDir; - if (JsonUtils::GetValueForKey(json, StartingDirectoryKey, startDir)) - { - _StartingDirectory = startDir; - } + JsonUtils::GetValueForKey(json, StartingDirectoryKey, _StartingDirectory); JsonUtils::GetValueForKey(json, IconKey, _Icon); JsonUtils::GetValueForKey(json, BackgroundImageKey, _BackgroundImagePath); @@ -341,7 +334,7 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, RetroTerminalEffectKey, _RetroTerminalEffect); JsonUtils::GetValueForKey(json, AntialiasingModeKey, _AntialiasingMode); - _TabColor.set = JsonUtils::GetValueForKey(json, TabColorKey, _TabColor.setting); + JsonUtils::GetValueForKey(json, TabColorKey, _TabColor); JsonUtils::GetValueForKey(json, BellStyleKey, _BellStyle); } diff --git a/src/cascadia/ut_app/JsonUtilsTests.cpp b/src/cascadia/ut_app/JsonUtilsTests.cpp index efaac0da0..7dbaf9fd2 100644 --- a/src/cascadia/ut_app/JsonUtilsTests.cpp +++ b/src/cascadia/ut_app/JsonUtilsTests.cpp @@ -101,6 +101,38 @@ JSON_FLAG_MAPPER(JsonTestFlags) }; }; +struct hstring_like +{ + std::string value; +}; +template<> +struct ConversionTrait +{ + hstring_like FromJson(const Json::Value& json) + { + return { ConversionTrait{}.FromJson(json) }; + } + + bool CanConvert(const Json::Value& json) + { + return json.isNull() || ConversionTrait{}.CanConvert(json); + } + + Json::Value ToJson(const hstring_like& val) + { + if (val.value == "") + { + return Json::Value::nullSingleton(); + } + return val.value; + } + + std::string TypeDescription() const + { + return "string"; + } +}; + namespace TerminalAppUnitTests { class JsonUtilsTests @@ -119,6 +151,11 @@ namespace TerminalAppUnitTests TEST_METHOD(FlagMapper); TEST_METHOD(NestedExceptionDuringKeyParse); + + TEST_METHOD(SetValueHStringLike); + TEST_METHOD(GetValueHStringLike); + + TEST_METHOD(DoubleOptional); }; template @@ -136,9 +173,8 @@ namespace TerminalAppUnitTests //// 1.a. Type Invalid - Exception //// VERIFY_THROWS_SPECIFIC(GetValue(object), DeserializationError, _ReturnTrueForException); - //// 1.b. JSON NULL - Zero Value //// - std::string zeroValueString{}; - VERIFY_ARE_EQUAL(zeroValueString, GetValue(Json::Value::nullSingleton())); + //// 1.b. JSON NULL - Exception //// + VERIFY_THROWS_SPECIFIC(GetValue(Json::Value::nullSingleton()), DeserializationError, _ReturnTrueForException); //// 1.c. Valid - Valid //// VERIFY_ARE_EQUAL(expected, GetValue(object)); @@ -167,9 +203,8 @@ namespace TerminalAppUnitTests VERIFY_THROWS_SPECIFIC(GetValue(object, outputRedHerring), DeserializationError, _ReturnTrueForException); VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged - //// 1.b. JSON NULL - Unchanged //// - VERIFY_IS_FALSE(GetValue(Json::Value::nullSingleton(), output)); // FALSE = storage not modified! - VERIFY_ARE_EQUAL("sentinel", output); // unchanged + //// 1.b. JSON NULL - Exception //// + VERIFY_THROWS_SPECIFIC(GetValue(Json::Value::nullSingleton(), output), DeserializationError, _ReturnTrueForException); //// 1.c. Valid //// VERIFY_IS_TRUE(GetValue(object, output)); @@ -208,14 +243,14 @@ namespace TerminalAppUnitTests //// 1.a. Type Invalid - Exception //// VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key), DeserializationError, _ReturnTrueForException); - //// 1.b. JSON NULL - Zero Value //// - std::string zeroValueString{}; - VERIFY_ARE_EQUAL(zeroValueString, GetValueForKey(object, nullKey)); + //// 1.b. JSON NULL - Exception //// + VERIFY_THROWS_SPECIFIC(GetValueForKey(object, nullKey), DeserializationError, _ReturnTrueForException); //// 1.c. Valid - Valid //// VERIFY_ARE_EQUAL(expected, GetValueForKey(object, key)); //// 1.d. Not Found - Zero Value //// + std::string zeroValueString{}; VERIFY_ARE_EQUAL(zeroValueString, GetValueForKey(object, invalidKey)); //// 2. Optional //// @@ -253,8 +288,7 @@ namespace TerminalAppUnitTests VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged //// 1.b. JSON NULL - Unchanged //// - VERIFY_IS_FALSE(GetValueForKey(object, nullKey, output)); // FALSE = storage not modified! - VERIFY_ARE_EQUAL("sentinel", output); // unchanged + VERIFY_THROWS_SPECIFIC(GetValueForKey(object, nullKey, output), DeserializationError, _ReturnTrueForException); //// 1.c. Valid //// VERIFY_IS_TRUE(GetValueForKey(object, key, output)); @@ -505,4 +539,89 @@ namespace TerminalAppUnitTests VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key), DeserializationError, CheckKeyInException); } + void JsonUtilsTests::SetValueHStringLike() + { + // Terminal has a string type (hstring) where null/"" are the same, and + // we want to make sure that optionals of that type serialize "properly". + hstring_like first{ "" }; + hstring_like second{ "second" }; + std::optional third{ { "" } }; + std::optional fourth{ { "fourth" } }; + std::optional fifth{}; + + Json::Value object{ Json::objectValue }; + + SetValueForKey(object, "first", first); + SetValueForKey(object, "second", second); + SetValueForKey(object, "third", third); + SetValueForKey(object, "fourth", fourth); + SetValueForKey(object, "fifth", fifth); + + VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["first"]); // real empty value serializes as null + VERIFY_ARE_EQUAL("second", object["second"].asString()); // serializes as a string + VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["third"]); // optional populated with real empty value serializes as null + VERIFY_ARE_EQUAL("fourth", object["fourth"].asString()); // serializes as a string + VERIFY_IS_FALSE(object.isMember("fifth")); // does not serialize + } + + void JsonUtilsTests::GetValueHStringLike() + { + Json::Value object{ Json::objectValue }; + object["string"] = "string"; + object["null"] = Json::Value::nullSingleton(); + // object["nonexistent"] can't be set, clearly, to continue not existing + + hstring_like v; + VERIFY_IS_TRUE(GetValueForKey(object, "string", v)); + VERIFY_ARE_EQUAL("string", v.value); // deserializes as string + VERIFY_IS_TRUE(GetValueForKey(object, "null", v)); + VERIFY_ARE_EQUAL("", v.value); // deserializes as real value, but empty + VERIFY_IS_FALSE(GetValueForKey(object, "nonexistent", v)); // does not deserialize + + std::optional optionalV; + // deserializes as populated optional containing string + VERIFY_IS_TRUE(GetValueForKey(object, "string", optionalV)); + VERIFY_IS_TRUE(optionalV.has_value()); + VERIFY_ARE_EQUAL("string", optionalV->value); + + optionalV = std::nullopt; + // deserializes as populated optional containing real empty value + VERIFY_IS_TRUE(GetValueForKey(object, "null", optionalV)); + VERIFY_IS_TRUE(optionalV.has_value()); + VERIFY_ARE_EQUAL("", optionalV->value); + + optionalV = std::nullopt; + // does not deserialize; optional remains nullopt + VERIFY_IS_FALSE(GetValueForKey(object, "nonexistent", optionalV)); + VERIFY_ARE_EQUAL(std::nullopt, optionalV); + } + + void JsonUtilsTests::DoubleOptional() + { + const std::optional> first{ std::nullopt }; // no value + const std::optional> second{ std::optional{ std::nullopt } }; // outer has a value, inner is "no value" + const std::optional> third{ std::optional{ 3 } }; // outer has a value, inner is "no value" + + Json::Value object{ Json::objectValue }; + + SetValueForKey(object, "first", first); + SetValueForKey(object, "second", second); + SetValueForKey(object, "third", third); + + VERIFY_IS_FALSE(object.isMember("first")); + VERIFY_IS_TRUE(object.isMember("second")); + VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["second"]); + VERIFY_ARE_EQUAL(Json::Value{ 3 }, object["third"]); + + std::optional> firstOut, secondOut, thirdOut; + VERIFY_IS_FALSE(GetValueForKey(object, "first", firstOut)); + VERIFY_IS_TRUE(GetValueForKey(object, "second", secondOut)); + VERIFY_IS_TRUE(static_cast(secondOut)); + VERIFY_ARE_EQUAL(std::nullopt, *secondOut); // should have come back out as null + + VERIFY_IS_TRUE(GetValueForKey(object, "third", thirdOut)); + VERIFY_IS_TRUE(static_cast(thirdOut)); + VERIFY_IS_TRUE(static_cast(*thirdOut)); + VERIFY_ARE_EQUAL(3, **thirdOut); + } }