utility: new utility method to convert proto value to string #36334

wbpcode · 2024-09-25T10:41:26Z

Commit Message: utility: new utility method to convert proto value to string
Additional Description:

New utility method to convert the proto value to json. This could work even the ENVOY_ENABLE_YAML is not set and is exception free.

Risk Level: low.
Testing: unit test.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode · 2024-09-25T10:42:00Z

This method will be used in the access log formatter first.

wbpcode · 2024-09-25T10:47:33Z

source/common/protobuf/utility.cc

+  switch (value.kind_case()) {
+  case ProtobufWkt::Value::KIND_NOT_SET:
+  case ProtobufWkt::Value::kNullValue:
+    streamer.addNull();
+    break;
+  case ProtobufWkt::Value::kNumberValue:
+    streamer.addNumber(value.number_value());
+    break;
+  case ProtobufWkt::Value::kStringValue:
+    streamer.addString(value.string_value());
+    break;
+  case ProtobufWkt::Value::kBoolValue:
+    streamer.addBool(value.bool_value());
+    break;
+  case ProtobufWkt::Value::kStructValue: {
+    auto map = streamer.makeRootMap();
+    structValueToJson(value.struct_value(), *map);
+    break;
+  }
+  case ProtobufWkt::Value::kListValue:
+    auto arr = streamer.makeRootArray();
+    listValueToJson(value.list_value(), *arr);
+    break;


there are some duplicated code in the ValueUtil::toJsonString and valueToJson. If the Streamer could provides addMap() and addArray(), then we can deduplicate these code by template. But it's up to @jmarantz

I'm not following what's duplicated yet; can you describe more If there's a small refactor to the streamer can you propose that as a separate PR just to make it go faster?

void valueToJson(const ProtobufWkt::Value& value, JsonStringStreamer::Level& level) { switch (value.kind_case()) { case ProtobufWkt::Value::KIND_NOT_SET: case ProtobufWkt::Value::kNullValue: level.addNull(); break; case ProtobufWkt::Value::kNumberValue: level.addNumber(value.number_value()); break; case ProtobufWkt::Value::kStringValue: level.addString(value.string_value()); break; case ProtobufWkt::Value::kBoolValue: level.addBool(value.bool_value()); break; case ProtobufWkt::Value::kStructValue: { auto map = level.addMap(); structValueToJson(value.struct_value(), *map); break; } case ProtobufWkt::Value::kListValue: { auto array = level.addArray(); listValueToJson(value.list_value(), *array); break; } } }

vs

void ValueUtil::appendJsonToString(const ProtobufWkt::Value& value, std::string& dest) { JsonStringStreamer streamer(dest); switch (value.kind_case()) { case ProtobufWkt::Value::KIND_NOT_SET: case ProtobufWkt::Value::kNullValue: streamer.addNull(); break; case ProtobufWkt::Value::kNumberValue: streamer.addNumber(value.number_value()); break; case ProtobufWkt::Value::kStringValue: streamer.addString(value.string_value()); break; case ProtobufWkt::Value::kBoolValue: streamer.addBool(value.bool_value()); break; case ProtobufWkt::Value::kStructValue: { auto map = streamer.makeRootMap(); structValueToJson(value.struct_value(), *map); break; } case ProtobufWkt::Value::kListValue: auto arr = streamer.makeRootArray(); listValueToJson(value.list_value(), *arr); break; } }

@jmarantz will address other comments first. Also create a minor refactoring PR. But it completely up to you.

jmarantz

lgtm at high level.

jmarantz · 2024-09-26T12:39:42Z

source/common/protobuf/utility.cc

+    break;
+  case ProtobufWkt::Value::kStructValue: {
+    auto map = level.addMap();
+    structValueToJson(value.struct_value(), *map);


nit: you could drop the temp here and just call this with *level.addMap(), so you wouldn't need the new scope. Up to you.

If you decide to keep the temp then please don't use auto for this case per style guide.

*level.addMap() looks good.

jmarantz · 2024-09-26T12:41:47Z

source/common/protobuf/utility.cc

+    break;
+  }
+  case ProtobufWkt::Value::kListValue: {
+    auto array = level.addArray();


jmarantz · 2024-09-26T12:42:44Z

source/common/protobuf/utility.cc

+  }
+  // Sort the keys to make the output deterministic.
+  std::sort(sorted_fields.begin(), sorted_fields.end(),
+            [](auto a, auto b) { return a.get().first < b.get().first; });


explicit types per style guide

source/common/protobuf/utility.cc

jmarantz · 2024-09-26T12:44:43Z

source/common/protobuf/utility.cc

@@ -852,6 +911,34 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector<ProtobufWkt::Value>& v
  return val;
 }

+void ValueUtil::toJsonString(const ProtobufWkt::Value& value, std::string& dest) {


Naming this toJsonString makes me think it will return a string. Should we call this appendJsonToString?

jmarantz · 2024-09-26T12:46:41Z

source/common/protobuf/utility.cc

+  switch (value.kind_case()) {
+  case ProtobufWkt::Value::KIND_NOT_SET:
+  case ProtobufWkt::Value::kNullValue:
+    streamer.addNull();
+    break;
+  case ProtobufWkt::Value::kNumberValue:
+    streamer.addNumber(value.number_value());
+    break;
+  case ProtobufWkt::Value::kStringValue:
+    streamer.addString(value.string_value());
+    break;
+  case ProtobufWkt::Value::kBoolValue:
+    streamer.addBool(value.bool_value());
+    break;
+  case ProtobufWkt::Value::kStructValue: {
+    auto map = streamer.makeRootMap();
+    structValueToJson(value.struct_value(), *map);
+    break;
+  }
+  case ProtobufWkt::Value::kListValue:
+    auto arr = streamer.makeRootArray();
+    listValueToJson(value.list_value(), *arr);
+    break;


I'm not following what's duplicated yet; can you describe more If there's a small refactor to the streamer can you propose that as a separate PR just to make it go faster?

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

source/common/protobuf/utility.cc

jmarantz · 2024-09-26T17:22:46Z

source/common/protobuf/utility.cc

+    structValueToJson(value.struct_value(), *streamer.makeRootMap());
+    break;
+  case ProtobufWkt::Value::kListValue:
+    listValueToJson(value.list_value(), *streamer.makeRootArray());


I'm not seeing how renaming makeRootArray to addArray in the streamer here would eliminate duplication; can you explain?

If we rename the makeRoot* to add*, then we can update our two function to following to eliminate the duplicated code:

template <class StreamerOrLevel> void valueToJson(const ProtobufWkt::Value& value, StreamerOrLevel& streamer_or_level) { switch (value.kind_case()) { case ProtobufWkt::Value::KIND_NOT_SET: case ProtobufWkt::Value::kNullValue: streamer_or_level.addNull(); break; case ProtobufWkt::Value::kNumberValue: streamer_or_level.addNumber(value.number_value()); break; case ProtobufWkt::Value::kStringValue: streamer_or_level.addString(value.string_value()); break; case ProtobufWkt::Value::kBoolValue: streamer_or_level.addBool(value.bool_value()); break; case ProtobufWkt::Value::kStructValue: structValueToJson(value.struct_value(), *streamer_or_level.addMap()); break; case ProtobufWkt::Value::kListValue: listValueToJson(value.list_value(), *streamer_or_level.addArray()); break; } }

void ValueUtil::appendJsonToString(const ProtobufWkt::Value& value, std::string& dest) { JsonStringStreamer streamer(dest); valueToJson(value, streamer); }

You can compare this with #36334 (comment)

I see, OK. I think I am OK with the way you have it in this PR with a duplicated switch. I can see the benefit of typing less code, but I feel like it creates an external dependency that Json::StreamerBase and Json::StreamerBase::Level have to have the same API, and if we are going to do that I think they should probably implement the same interface. I think it's probably not worth it to save this duplicated switch.

I am really a big fan of C++in most ways but one that would be nice is a language feature for describing an interface that a class passed into a template must implement.

Concept? Haha.

jmarantz · 2024-09-26T17:24:12Z

test/common/protobuf/utility_test.cc

  repeated.Add()->set_value(20);
  EXPECT_THAT(RepeatedPtrUtil::debugString(repeated),
-              testing::ContainsRegex("\\[.*[\n]*value:\\s*10\n,.*[\n]*value:\\s*20\n\\]"));
+              testing::ContainsRegex("\\[value: 10\n, value: 20\n\\]"));


does this test work without this expectation change?

at least cannot work at my local env. I can revert it to see whether will our ci say ok.

fine, our CI say it ok

jmarantz · 2024-09-26T17:28:28Z

test/common/protobuf/utility_test.cc

+    ValueUtil::toJsonString(v, json_string);
+    EXPECT_EQ(json_string, "null");
+    json_string.clear();
+


I would add a test-method helper that takes a Value as an arg and returns the json string, with these 4 lines. Then you can structure tests like

ProtobufWkt::Value v; EXPECT_EQ("null", toJson(v)); v.set_null_value(ProtobufWkt::NULL_VALUE)); EXPECT_EQ("null", toJson(v));

I think this will make tests more information-dense and easier to read.

jmarantz · 2024-09-26T17:28:49Z

/wait

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

jmarantz

flushing some more comments.

/wait

jmarantz · 2024-09-30T13:17:52Z

source/common/protobuf/utility.cc

@@ -10,6 +10,7 @@
 #include "source/common/common/assert.h"
 #include "source/common/common/documentation_url.h"
 #include "source/common/common/fmt.h"
+#include "source/common/json/json_streamer.h"


can we move the new utility that depends on JsonStreamer into the json library? I think it's confusing, at the directory level, to have json depend on proto and vice versa. I don't think it's hard to break the cycle by moving the new utility.

jmarantz · 2024-09-30T13:20:22Z

source/common/protobuf/utility.cc

@@ -106,6 +107,60 @@ absl::Status validateDurationAsMillisecondsNoThrow(const ProtobufWkt::Duration&
  return validateDurationNoThrow(duration, kMaxInt64Nanoseconds);
 }

+using JsonStringStreamer = Json::StreamerBase<Json::StringOutput>;


Please move this to the end of json_streamer.h, next to the nickname for the buffered streamer. You can name it StringStreamer since it will be in the 'Json' namespace.

We should do a follow-up PR to rename Json::Streamer to Json::BufferStreamer for consistency.

jmarantz · 2024-09-30T13:23:57Z

source/common/protobuf/utility.cc

+    structValueToJson(value.struct_value(), *streamer.makeRootMap());
+    break;
+  case ProtobufWkt::Value::kListValue:
+    listValueToJson(value.list_value(), *streamer.makeRootArray());


I see, OK. I think I am OK with the way you have it in this PR with a duplicated switch. I can see the benefit of typing less code, but I feel like it creates an external dependency that Json::StreamerBase and Json::StreamerBase::Level have to have the same API, and if we are going to do that I think they should probably implement the same interface. I think it's probably not worth it to save this duplicated switch.

I am really a big fan of C++in most ways but one that would be nice is a language feature for describing an interface that a class passed into a template must implement.

jmarantz · 2024-09-30T13:26:27Z

test/common/protobuf/utility_test.cc

-    ValueUtil::toJsonString(v, json_string);
-    EXPECT_EQ(json_string, "true");
-    json_string.clear();
+    EXPECT_EQ(toJson(v), "true");


this is a lot nicer. Do we need to declare a new ProtobufWkt::Value in each section? Or would this have the same effect:

ProtobufWkt::Value v; EXPECT_EQ(toJson(v), "null"); v.set_bool_value(true); EXPECT_EQ(toJson(v), "null"); v.set_number_value(1); EXPECT_EQ(toJson(v), "1"); ...

alternatively you could put ProtobufWkt::Value value_; in the test fixture and have every one of these stanzas be a separate 1 or 2-line TEST_F.

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode · 2024-10-01T09:24:03Z

/retest

…alue-to-string

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

…alue-to-string

jmarantz

nice, thanks!

wbpcode · 2024-10-02T12:31:51Z

Defer this to @alyssawilk for a senior maintainer review. :)

alyssawilk

sorry for the delay, was out sick
LGTM assuming you're going to use this in a follow-up (which from PR description looks like). Can we convert both NO_YAML and YAML builds to this and remove legacy code?

wbpcode · 2024-10-07T14:29:46Z

sorry for the delay, was out sick
LGTM assuming you're going to use this in a follow-up (which from PR description looks like). Can we convert both NO_YAML and YAML builds to this and remove legacy code?

Sadly, only part of positions could be optimized with this PR for now.

…oxy#36334) Commit Message: utility: new utility method to convert proto value to string Additional Description: New utility method to convert the proto value to json. This could work even the `ENVOY_ENABLE_YAML` is not set and is exception free. Risk Level: low. Testing: unit test. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wangbaiping <wangbaiping@bytedance.com> Signed-off-by: Steven Jin Xuan <sjinxuan@microsoft.com>

utility: new utility method to convert proto value to string

071d0cd

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode assigned jmarantz Sep 25, 2024

wbpcode commented Sep 25, 2024

View reviewed changes

jmarantz reviewed Sep 26, 2024

View reviewed changes

wbpcode mentioned this pull request Sep 26, 2024

WIP: rename the makeRoot* to add* #36350

Closed

address some comments

cf2dc20

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

jmarantz reviewed Sep 26, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 26, 2024

address comments

2a5c8ca

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

repokitteh-read-only bot removed the waiting label Sep 28, 2024

jmarantz reviewed Sep 30, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 30, 2024

jmarantz reviewed Sep 30, 2024

View reviewed changes

address comments

4494012

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

repokitteh-read-only bot removed the waiting label Oct 1, 2024

wbpcode added 4 commits October 1, 2024 21:58

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-v…

f5ab533

…alue-to-string

clean up unnecessary changes

36c477a

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

clean up more unnecessary changes

71b86aa

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-v…

44016e8

…alue-to-string

jmarantz approved these changes Oct 2, 2024

View reviewed changes

wbpcode assigned alyssawilk Oct 2, 2024

alyssawilk approved these changes Oct 7, 2024

View reviewed changes

wbpcode merged commit 68b4559 into envoyproxy:main Oct 7, 2024
21 checks passed

wbpcode deleted the dev-value-to-string branch October 7, 2024 14:30

utility: new utility method to convert proto value to string #36334

utility: new utility method to convert proto value to string #36334

Conversation

wbpcode commented Sep 25, 2024

wbpcode commented Sep 25, 2024 • edited Loading

wbpcode Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Sep 26, 2024

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Oct 1, 2024

jmarantz left a comment

Choose a reason for hiding this comment

wbpcode commented Oct 2, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

wbpcode commented Oct 7, 2024

wbpcode commented Sep 25, 2024 •

edited

Loading

wbpcode Sep 25, 2024 •

edited

Loading

wbpcode Sep 26, 2024 •

edited

Loading

wbpcode Sep 27, 2024 •

edited

Loading