Remove vast::json class

[ngrodzitski]

📔 Description

Remove vast::json from vast.

📝 Checklist

• All user-facing changes have changelog entries.
• The changes are reflected on docs.tenzir.com/vast, if necessary.
• The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

[ngrodzitski]

There is a ambiguity in how to interpret a given json values as types.
This is example from integration test (which should fail currently):

{ "a": null, "b": true, "c": false, "d": 0, "e": -0, "f": -42, "g": 4.2, "h": 42.0, "i": 42}
-type json = record{a: none, b: bool, c: bool, d: real, e: real, f: int, g: real, h: count, i: count}
+type json = record{a: none, b: bool, c: bool, d: int, e: int, f: int, g: real, h: real, i: int}

[dominiklohmann]

That's just the nature of vast::data being more powerful than JSON. I'll discuss with the team which conversions are preferred.

[dominiklohmann]

After looking at the source code some more, I think the way you implemented this is absolutely correct. Please update the integration test accordingly.

[ngrodzitski]

I didn't delete vast/concept/printable/vast/json.hpp header and reuse its printer as a second option for vast::data printer:

auto text_str = to_string(data_object); // default to_string(data) rendering
auto json_str = to_string(data_object, print_rendering::json); // explicitly set json rendering

If the idea of such extended to_string(data, rendering) is ok, then json_data_printer can be moved here entirely.

[dominiklohmann]

For YAML we use this:

/// Prints data as YAML.
/// @param x The data instance.
/// @returns The YAML representation of *x*, or an error.
caf::expected<std::string> to_yaml(const data& x);

/// Parses YAML into a data.
/// @param str The string containing the YAML content
/// @returns The parsed YAML as data, or an error.
caf::expected<data> from_yaml(std::string_view str);

So I think you can just name the function to_json and have it return a caf::expected<std::string>.

[ngrodzitski]

Those 3 convert functions are quite asymmetric to many other convert function in vast, which return bool to manifest the result of conversion. while if (convert(x,y)) is test on success for return-bool-based convert functions it is a test on fail for return-caf::error-based convert function.

[dominiklohmann]

Yes, they were overlooked in a past refactoring. Generally, convert overloads must return something that is explicitly convertible to bool, but just returning bool is preferred since the errors aren't checked anyways.

[dominiklohmann]

Thanks for the PR; this will take while for me to go through it, but I will do a first pass sometime today and then we can hopefully merge this early next week to avoid merge conflicts as much as possible.

[dominiklohmann]

Looking really good so far.

• Please use to_json over to_string(data, tag_type).
• There are some leftover integration test file with simdjson in their name.

[ngrodzitski]

Thinking of these to_string/to_json/to_yaml family of function and having fmt already in vast I'd like to share a quick observation:

I think the whole engine of printable things can be rebuilt with fmt (thankfully vast printable machinery is very similar to the one in fmt), thus reducing maintenance costs in a long run (a wide community is interested to have FMT a bug-free and performant) and be more 3rd-party friendly (e.g. plugins authors would have a kind of "standard" approach to do printings with vast types). Also it would be more expressive and flexible to have possible rendering formats as options, something like the following:

auto d = vast::data{ make_something()};
auto text = fmt::format("{}", d);
auto json = fmt::format("{j}", d);
auto yaml = fmt::format("{y}", d);

[mavam]

This is great observation. We actually had the very same discussion last week: what's the future of our concepts, in particular Printable, Parseable, Hashable. It's no question that FMT is a major win and probably our best for the implementation of what we call Printable right now.

A while ago, we wanted to have to<yaml>, to<json>, etc. for rendering a type in a specific way. (The to overloads return an expected<T> instead of a T.) But we are moving away from that. Now that json is gone, we're actually going in a different direction.

The underlying question is: what's the most flexible mechanism to print one type in different ways? By treating FMT a first-class library in the code base, your proposal makes a lot of sense. The only thing I wish we had is a symmetric API for parsing. I'm not sure if these are realistic expectations, though.

[dominiklohmann]

We've had the discussion before @mavam, and I feel like forcing APIs to be symmetric will just hold us back.

For now, I think to_json is fine, but I agree with @ngrodzitski that a format option for {fmt} would be much more friendly to plugin authors.

[dominiklohmann]

I'll merge this into another branch now to do some cleanup and to write a changelog entry and documentation. Thanks for your work on this @ngrodzitski! 🙏