Skip to content

Commit

Permalink
Rework JsonUtils' optional handling to allow Converters access to nul…
Browse files Browse the repository at this point in the history 
…l values
  • Loading branch information
@DHowett
DHowett committed Nov 6, 2020
1 parent 4eeaddc commit dfea59b
Show file tree
Hide file tree
Showing 4 changed files with 205 additions and 73 deletions.
17 changes: 9 additions & 8 deletions doc/cascadia/Json-Utility-API.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> 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<std::string>(json);
// three is populated or zero-initialized
// three is populated or an exception is thrown

auto four = JsonUtils::GetValue<std::optional<std::string>>(json);
// four is populated or nullopt
Expand Down Expand Up @@ -225,14 +226,14 @@ auto v = JsonUtils::GetValue<int>(json, conv);
-|json type invalid|json null|valid
-|-|-|-
`T`|❌ exception|🔵 unchanged|✔ converted
`T`|❌ exception|❌ exception|✔ converted
`std::optional<T>`|❌ exception|🟨 `nullopt`|✔ converted
### GetValue&lt;T&gt;() (returning)
-|json type invalid|json null|valid
-|-|-|-
`T`|❌ exception|🟨 `T{}` (zero value)|✔ converted
`T`|❌ exception|❌ exception|✔ converted
`std::optional<T>`|❌ exception|🟨 `nullopt`|✔ converted
### GetValueForKey(T&) (type-deducing)
Expand All @@ -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<T>`|_🔵 unchanged_|_❌ exception_|_🟨 `nullopt`_|_✔ converted_
`T`|🔵 unchanged|_❌ exception_|_❌ exception_|_✔ converted_
`std::optional<T>`|🔵 unchanged|_❌ exception_|_🟨 `nullopt`_|_✔ converted_
### GetValueForKey&lt;T&gt;() (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<T>`|🟨 `nullopt`|_❌ exception_|_🟨 `nullopt`_|_✔ converted_
### Future Direction
Expand Down
140 changes: 95 additions & 45 deletions src/cascadia/TerminalSettingsModel/JsonUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,29 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
}

template<typename T>
struct DeduceOptional
struct OptionOracle
{
using Type = typename std::decay<T>::type;
static constexpr bool IsOptional = false;
template<typename U> // universal parameter
static constexpr bool HasValue(U&&)
{
return true;
}
};

template<typename TOpt>
struct DeduceOptional<std::optional<TOpt>>
struct OptionOracle<std::optional<TOpt>>
{
using Type = typename std::decay<TOpt>::type;
static constexpr bool IsOptional = true;
static constexpr std::optional<TOpt> EmptyV() { return std::nullopt; }
static constexpr bool HasValue(const std::optional<TOpt>& o) { return o.has_value(); }
static constexpr auto&& Value(const std::optional<TOpt>& o) { return *o; }
};

template<typename TOpt>
struct DeduceOptional<::winrt::Windows::Foundation::IReference<TOpt>>
{
using Type = typename std::decay<TOpt>::type;
static constexpr bool IsOptional = true;
};

template<>
struct DeduceOptional<::winrt::hstring>
struct OptionOracle<::winrt::Windows::Foundation::IReference<TOpt>>
{
using Type = typename ::winrt::hstring;
static constexpr bool IsOptional = true;
static constexpr ::winrt::Windows::Foundation::IReference<TOpt> EmptyV() { return nullptr; }
static constexpr bool HasValue(const ::winrt::Windows::Foundation::IReference<TOpt>& o) { return static_cast<bool>(o); }
static constexpr auto&& Value(const ::winrt::Windows::Foundation::IReference<TOpt>& o) { return o.Value(); }
};
}

Expand Down Expand Up @@ -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<std::wstring>::CanConvert(json) || json.isNull();
}
};
#endif

Expand Down Expand Up @@ -419,6 +431,56 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
};
#endif

template<typename T, typename TDelegatedConverter = ConversionTrait<T>, typename TOptional = std::optional<T>>
struct OptionalConverter
{
using Oracle = Detail::OptionOracle<TOptional>;
TDelegatedConverter delegatedConverter{};

TOptional 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();
}
TOptional val{ delegatedConverter.FromJson(json) };
return val;
}

bool CanConvert(const Json::Value& json)
{
return json.isNull() || delegatedConverter.CanConvert(json);
}

Json::Value ToJson(const TOptional& val)
{
if (!Oracle::HasValue(val))
{
return Json::Value::nullSingleton();
}
return delegatedConverter.ToJson(Oracle::Value(val));
}

std::string TypeDescription() const
{
return delegatedConverter.TypeDescription();
}
};

template<typename T>
struct ConversionTrait<std::optional<T>> : public OptionalConverter<T, ConversionTrait<T>, std::optional<T>>
{
};

#ifdef WINRT_Windows_Foundation_H
template<typename T>
struct ConversionTrait<::winrt::Windows::Foundation::IReference<T>> : public OptionalConverter<T, ConversionTrait<T>, ::winrt::Windows::Foundation::IReference<T>>
{
};
#endif

template<typename T, typename TBase>
struct EnumMapper
{
Expand Down Expand Up @@ -586,31 +648,15 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
template<typename T, typename Converter>
bool GetValue(const Json::Value& json, T& target, Converter&& conv)
{
if constexpr (Detail::DeduceOptional<T>::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
Expand Down Expand Up @@ -654,30 +700,30 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
template<typename T>
bool GetValue(const Json::Value& json, T& target)
{
return GetValue(json, target, ConversionTrait<typename Detail::DeduceOptional<T>::Type>{});
return GetValue(json, target, ConversionTrait<typename std::decay<T>::type>{});
}

// GetValue, forced return type, with automatic converter
template<typename T>
std::decay_t<T> GetValue(const Json::Value& json)
{
std::decay_t<T> local{};
GetValue(json, local, ConversionTrait<typename Detail::DeduceOptional<T>::Type>{});
GetValue(json, local, ConversionTrait<typename std::decay<T>::type>{});
return local; // returns zero-initialized or value
}

// GetValueForKey, type-deduced, with automatic converter
template<typename T>
bool GetValueForKey(const Json::Value& json, std::string_view key, T& target)
{
return GetValueForKey(json, key, target, ConversionTrait<typename Detail::DeduceOptional<T>::Type>{});
return GetValueForKey(json, key, target, ConversionTrait<typename std::decay<T>::type>{});
}

// GetValueForKey, forced return type, with automatic converter
template<typename T>
std::decay_t<T> GetValueForKey(const Json::Value& json, std::string_view key)
{
return GetValueForKey<T>(json, key, ConversionTrait<typename Detail::DeduceOptional<T>::Type>{});
return GetValueForKey<T>(json, key, ConversionTrait<typename std::decay<T>::type>{});
}

// Get multiple values for keys (json, k, &v, k, &v, k, &v, ...).
Expand All @@ -696,15 +742,19 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
template<typename T, typename Converter>
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 (Detail::OptionOracle<T>::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<typename T>
void SetValueForKey(Json::Value& json, std::string_view key, const T& target)
{
SetValueForKey(json, key, target, ConversionTrait<typename Detail::DeduceOptional<T>::Type>{});
SetValueForKey(json, key, target, ConversionTrait<typename std::decay<T>::type>{});
}
};

Expand Down
11 changes: 2 additions & 9 deletions src/cascadia/TerminalSettingsModel/Profile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::wstring>{});
JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::OptionalConverter<hstring, JsonUtils::PermissiveStringConverter<std::wstring>>{});

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);
Expand Down
Loading

1 comment on commit dfea59b

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New misspellings found, please review:

  • TDelegated
  • TOptional
To accept these changes, run the following commands 
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"Bopomofo CParams GENERATEPROJECTPRIFILE hhhh renamer rgus SGRXY Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/dfea59b24e5d2f39a2c7a451d2d5abd81017a47c.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"Renamer TDelegated TOptional unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/dfea59b24e5d2f39a2c7a451d2d5abd81017a47c.txt is added to your repository...'
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Please sign in to comment.