Merge remote-tracking branch 'origin/topic/awelzel/no-rapidjson-include-in-headers'

* origin/topic/awelzel/no-rapidjson-include-in-headers:
  formatters/JSON: Make JSON::NullDoubleWriter use zeek::json::detail version
  formatters/JSON: Remove surrounding_braces
  formatters/JSON: Prepare to remove rapidjson from installed Zeek headers

CHANGES

@@ -1,3 +1,36 @@
+6.1.0-dev.115 | 2023-06-21 17:45:35 -0700
+
+  * formatters/JSON: Make JSON::NullDoubleWriter use zeek::json::detail version (Arne Welzel, Corelight)
+
+    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.
+
+  * formatters/JSON: Remove surrounding_braces (Arne Welzel, Corelight)
+
+    This seems to have become unused 4 years ago with 9b76e8faf44e90c41f33f24b18900a50f0840c5a,
+    remove it.
+
+  * formatters/JSON: Prepare to remove rapidjson from installed Zeek headers (Arne Welzel, Corelight)
+
+    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
+
 6.1.0-dev.110 | 2023-06-21 15:36:32 -0700
 
   * Update zeekjs submodule (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-6.1.0-dev.110
+6.1.0-dev.115

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:

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,
+static void BuildJSON(json::detail::NullDoubleWriter& writer, Val* val, bool only_loggable = false,
-                      bool only_loggable = false, RE_Matcher* re = nullptr, const string& key = "")
+                      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, "");
 

src/threading/formatters/JSON.cc

@@ -16,22 +16,25 @@
 
 #include "zeek/Desc.h"
 #include "zeek/threading/MsgThread.h"
+#include "zeek/threading/formatters/detail/json.h"
 
 namespace zeek::threading::formatter
 	{
 
+// For deprecated NullDoubleWriter
+JSON::NullDoubleWriter::NullDoubleWriter(rapidjson::StringBuffer& stream)
+	: writer(std::make_unique<zeek::json::detail::NullDoubleWriter>(stream))
+	{
+	}
+
 bool JSON::NullDoubleWriter::Double(double d)
 	{
-	if ( rapidjson::internal::Double(d).IsNanOrInf() )
+	return writer->Double(d);
-		return rapidjson::Writer<rapidjson::StringBuffer>::Null();
-
-	return rapidjson::Writer<rapidjson::StringBuffer>::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() { }
@@ -39,7 +42,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 +71,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 +88,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);

src/threading/formatters/JSON.h

@@ -2,12 +2,21 @@
 
 #pragma once
 
+#include <memory>
+
 #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 <rapidjson/document.h>
 #include <rapidjson/writer.h>
 
 #include "zeek/threading/Formatter.h"
 
+namespace zeek::json::detail
+	{
+class NullDoubleWriter;
+	}
+
 namespace zeek::threading::formatter
 	{
 
@@ -38,18 +47,19 @@ public:
 	class NullDoubleWriter : public rapidjson::Writer<rapidjson::StringBuffer>
 		{
 	public:
-		NullDoubleWriter(rapidjson::StringBuffer& stream)
+		[[deprecated("Remove in v7.1 - This is an implementation detail.")]] NullDoubleWriter(
-			: rapidjson::Writer<rapidjson::StringBuffer>(stream)
+			rapidjson::StringBuffer& stream);
-			{
-			}
 		bool Double(double d);
+
+	private:
+		std::unique_ptr<json::detail::NullDoubleWriter> writer;
 		};
 
 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;
 	bool include_unset_fields;
 	};
 

src/threading/formatters/detail/json.h

@@ -0,0 +1,27 @@
+// Not installed - used by Val.cc and formatters/JSON.cc only.
+#pragma once
+
+#include <rapidjson/document.h>
+#include <rapidjson/internal/ieee754.h>
+#include <rapidjson/writer.h>
+
+namespace zeek::json::detail
+	{
+// A rapidjson Writer that writes null for inf or nan numbers.
+class NullDoubleWriter : public rapidjson::Writer<rapidjson::StringBuffer>
+	{
+public:
+	explicit NullDoubleWriter(rapidjson::StringBuffer& stream)
+		: rapidjson::Writer<rapidjson::StringBuffer>(stream)
+		{
+		}
+	bool Double(double d)
+		{
+		if ( rapidjson::internal::Double(d).IsNanOrInf() )
+			return rapidjson::Writer<rapidjson::StringBuffer>::Null();
+
+		return rapidjson::Writer<rapidjson::StringBuffer>::Double(d);
+		}
+	};
+
+	}