From 6efc696179cb8be8606c551222945ab0af397ac8 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Thu, 15 Jun 2023 12:16:13 +0200 Subject: [PATCH 1/3] formatters/JSON: Prepare to remove rapidjson from installed Zeek headers threading/formatters/JSON.h currently includes rapidjson headers for declaring the NullDoubleWriter. This appears mostly an internal detail, but results in the situation that 1) we need to ship rapidjson headers with the Zeek install tree and 2) taking care that external plugins are able to find these headers should they include formatters/JSON.h. There are currently no other Zeek headers that include rapidjson, so this seems very unfortunate and self-inflicted given it's not actually required. Attempt to hide this implementation detail with the goal to remove the rapidjson includes with v7.1 and then also stop bundling and exposing the include path to external plugins. The NullDoubleWriter implementation moves into a new formatters/detail/json.h header which is not installed. Closes #3128 --- src/CMakeLists.txt | 2 ++ src/Val.cc | 10 +++++----- src/threading/formatters/JSON.cc | 9 ++++++--- src/threading/formatters/JSON.h | 13 +++++++++++-- src/threading/formatters/detail/json.h | 27 ++++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 src/threading/formatters/detail/json.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 69e3ebc90c..f7fe8931bf 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -602,6 +602,8 @@ install( PATTERN "*.h" PATTERN "*.pac" PATTERN "3rdparty/*" EXCLUDE + # Headers used only during build + PATTERN "threading/formatters/detail" EXCLUDE # The "zeek -> ." symlink isn't needed in the install-tree REGEX "${escaped_include_path}$" EXCLUDE # FILES_MATCHING creates empty directories: diff --git a/src/Val.cc b/src/Val.cc index 102021c8a6..3cb1d08a63 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -39,7 +39,7 @@ #include "zeek/broker/Data.h" #include "zeek/broker/Manager.h" #include "zeek/broker/Store.h" -#include "zeek/threading/formatters/JSON.h" +#include "zeek/threading/formatters/detail/json.h" using namespace std; @@ -404,8 +404,8 @@ TableValPtr Val::GetRecordFields() // This is a static method in this file to avoid including rapidjson's headers in Val.h because // they're huge. -static void BuildJSON(threading::formatter::JSON::NullDoubleWriter& writer, Val* val, - bool only_loggable = false, RE_Matcher* re = nullptr, const string& key = "") +static void BuildJSON(json::detail::NullDoubleWriter& writer, Val* val, bool only_loggable = false, + RE_Matcher* re = nullptr, const string& key = "") { if ( ! key.empty() ) writer.Key(key); @@ -509,7 +509,7 @@ static void BuildJSON(threading::formatter::JSON::NullDoubleWriter& writer, Val* else { rapidjson::StringBuffer buffer; - threading::formatter::JSON::NullDoubleWriter key_writer(buffer); + json::detail::NullDoubleWriter key_writer(buffer); BuildJSON(key_writer, entry_key, only_loggable, re); string key_str = buffer.GetString(); @@ -612,7 +612,7 @@ static void BuildJSON(threading::formatter::JSON::NullDoubleWriter& writer, Val* StringValPtr Val::ToJSON(bool only_loggable, RE_Matcher* re) { rapidjson::StringBuffer buffer; - threading::formatter::JSON::NullDoubleWriter writer(buffer); + json::detail::NullDoubleWriter writer(buffer); BuildJSON(writer, this, only_loggable, re, ""); diff --git a/src/threading/formatters/JSON.cc b/src/threading/formatters/JSON.cc index 9c5825bdfd..25ba007596 100644 --- a/src/threading/formatters/JSON.cc +++ b/src/threading/formatters/JSON.cc @@ -16,10 +16,12 @@ #include "zeek/Desc.h" #include "zeek/threading/MsgThread.h" +#include "zeek/threading/formatters/detail/json.h" namespace zeek::threading::formatter { +// For deprecated NullDoubleWriter bool JSON::NullDoubleWriter::Double(double d) { if ( rapidjson::internal::Double(d).IsNanOrInf() ) @@ -39,7 +41,7 @@ JSON::~JSON() { } bool JSON::Describe(ODesc* desc, int num_fields, const Field* const* fields, Value** vals) const { rapidjson::StringBuffer buffer; - NullDoubleWriter writer(buffer); + zeek::json::detail::NullDoubleWriter writer(buffer); writer.StartObject(); @@ -68,7 +70,7 @@ bool JSON::Describe(ODesc* desc, Value* val, const std::string& name) const rapidjson::Document doc; rapidjson::StringBuffer buffer; - NullDoubleWriter writer(buffer); + zeek::json::detail::NullDoubleWriter writer(buffer); writer.StartObject(); BuildJSON(writer, val, name); @@ -85,7 +87,8 @@ Value* JSON::ParseValue(const std::string& s, const std::string& name, TypeTag t return nullptr; } -void JSON::BuildJSON(NullDoubleWriter& writer, Value* val, const std::string& name) const +void JSON::BuildJSON(zeek::json::detail::NullDoubleWriter& writer, Value* val, + const std::string& name) const { if ( ! name.empty() ) writer.Key(name); diff --git a/src/threading/formatters/JSON.h b/src/threading/formatters/JSON.h index c946ab7bf2..e76d3e3edf 100644 --- a/src/threading/formatters/JSON.h +++ b/src/threading/formatters/JSON.h @@ -3,11 +3,18 @@ #pragma once #define RAPIDJSON_HAS_STDSTRING 1 +// Remove in v7.1 when removing NullDoubleWriter below and also remove +// rapidjson include tweaks from CMake's dynamic_plugin_base target. #include #include #include "zeek/threading/Formatter.h" +namespace zeek::json::detail + { +class NullDoubleWriter; + } + namespace zeek::threading::formatter { @@ -38,7 +45,8 @@ public: class NullDoubleWriter : public rapidjson::Writer { public: - NullDoubleWriter(rapidjson::StringBuffer& stream) + [[deprecated("Remove in v7.1 - This is an implementation detail.")]] NullDoubleWriter( + rapidjson::StringBuffer& stream) : rapidjson::Writer(stream) { } @@ -46,7 +54,8 @@ public: }; private: - void BuildJSON(NullDoubleWriter& writer, Value* val, const std::string& name = "") const; + void BuildJSON(zeek::json::detail::NullDoubleWriter& writer, Value* val, + const std::string& name = "") const; TimeFormat timestamps; bool surrounding_braces; diff --git a/src/threading/formatters/detail/json.h b/src/threading/formatters/detail/json.h new file mode 100644 index 0000000000..7ae9f05c70 --- /dev/null +++ b/src/threading/formatters/detail/json.h @@ -0,0 +1,27 @@ +// Not installed - used by Val.cc and formatters/JSON.cc only. +#pragma once + +#include +#include +#include + +namespace zeek::json::detail + { +// A rapidjson Writer that writes null for inf or nan numbers. +class NullDoubleWriter : public rapidjson::Writer + { +public: + explicit NullDoubleWriter(rapidjson::StringBuffer& stream) + : rapidjson::Writer(stream) + { + } + bool Double(double d) + { + if ( rapidjson::internal::Double(d).IsNanOrInf() ) + return rapidjson::Writer::Null(); + + return rapidjson::Writer::Double(d); + } + }; + + } From 7c3b5553d84b763dfe4aece9499fc515124e36a6 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 16 Jun 2023 11:54:35 +0200 Subject: [PATCH 2/3] formatters/JSON: Remove surrounding_braces This seems to have become unused 4 years ago with 9b76e8faf44e90c41f33f24b18900a50f0840c5a, remove it. --- src/threading/formatters/JSON.cc | 3 +-- src/threading/formatters/JSON.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/threading/formatters/JSON.cc b/src/threading/formatters/JSON.cc index 25ba007596..5320b79d70 100644 --- a/src/threading/formatters/JSON.cc +++ b/src/threading/formatters/JSON.cc @@ -31,9 +31,8 @@ bool JSON::NullDoubleWriter::Double(double d) } JSON::JSON(MsgThread* t, TimeFormat tf, bool arg_include_unset_fields) - : Formatter(t), surrounding_braces(true), include_unset_fields(arg_include_unset_fields) + : Formatter(t), timestamps(tf), include_unset_fields(arg_include_unset_fields) { - timestamps = tf; } JSON::~JSON() { } diff --git a/src/threading/formatters/JSON.h b/src/threading/formatters/JSON.h index e76d3e3edf..5703216cd3 100644 --- a/src/threading/formatters/JSON.h +++ b/src/threading/formatters/JSON.h @@ -58,7 +58,6 @@ private: const std::string& name = "") const; TimeFormat timestamps; - bool surrounding_braces; bool include_unset_fields; }; From d892cac58c7853896aa8dc631d6ef21c865593f5 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Sat, 17 Jun 2023 14:09:03 +0200 Subject: [PATCH 3/3] formatters/JSON: Make JSON::NullDoubleWriter use zeek::json::detail version Not using inheritance and preferring composition to avoid including the detail/json.h header do an indirection via a unique_ptr and then just re-use the Double() implementation. --- src/threading/formatters/JSON.cc | 10 ++++++---- src/threading/formatters/JSON.h | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/threading/formatters/JSON.cc b/src/threading/formatters/JSON.cc index 5320b79d70..0a89db329d 100644 --- a/src/threading/formatters/JSON.cc +++ b/src/threading/formatters/JSON.cc @@ -22,12 +22,14 @@ namespace zeek::threading::formatter { // For deprecated NullDoubleWriter +JSON::NullDoubleWriter::NullDoubleWriter(rapidjson::StringBuffer& stream) + : writer(std::make_unique(stream)) + { + } + bool JSON::NullDoubleWriter::Double(double d) { - if ( rapidjson::internal::Double(d).IsNanOrInf() ) - return rapidjson::Writer::Null(); - - return rapidjson::Writer::Double(d); + return writer->Double(d); } JSON::JSON(MsgThread* t, TimeFormat tf, bool arg_include_unset_fields) diff --git a/src/threading/formatters/JSON.h b/src/threading/formatters/JSON.h index 5703216cd3..74bc89c8ae 100644 --- a/src/threading/formatters/JSON.h +++ b/src/threading/formatters/JSON.h @@ -2,6 +2,8 @@ #pragma once +#include + #define RAPIDJSON_HAS_STDSTRING 1 // Remove in v7.1 when removing NullDoubleWriter below and also remove // rapidjson include tweaks from CMake's dynamic_plugin_base target. @@ -46,11 +48,11 @@ public: { public: [[deprecated("Remove in v7.1 - This is an implementation detail.")]] NullDoubleWriter( - rapidjson::StringBuffer& stream) - : rapidjson::Writer(stream) - { - } + rapidjson::StringBuffer& stream); bool Double(double d); + + private: + std::unique_ptr writer; }; private: