Rapidjson parity #205

ccaraman · 2016-11-09T22:34:41Z

No description provided.

mattklein123 · 2016-11-09T23:04:34Z

source/common/json/json_loader.cc

-                                error.line, error.column));
+  char buffer[4096];
+
+  FILE* fp = fopen(file_path.c_str(), "r");


You are leaking here. Can you use C++ iostream?

if not, you can use CSmartPtr and supply fclose not to leak fd.

Was able to switch to iostream.

mattklein123 · 2016-11-09T23:06:00Z

source/common/json/json_loader.cc

+    throw Exception(fmt::format("File:{} doesn't exist.", file_path));
+  }
+
+  rapidjson::FileReadStream fs(fp, buffer, sizeof(buffer));


what is buffer used for here? Why does it have to be specified? Why 4K? Seems odd there is no API to just deal with buffering internally.

Removed. Switched to using iostream.

mattklein123 · 2016-11-09T23:06:20Z

source/common/json/json_loader.cc

+  rapidjson::FileReadStream fs(fp, buffer, sizeof(buffer));
+  if (document_.ParseStream(fs).HasParseError()) {
+    throw Exception(fmt::format("Error(offset {}): {}\n",
+                                static_cast<unsigned>(document_.GetErrorOffset()),


What is offset? Does it include line number?

wondering, why casting to unsigned_int?

mattklein123 · 2016-11-09T23:07:21Z

source/exe/CMakeLists.txt

@@ -20,6 +20,7 @@ target_link_libraries(envoy rt)
 target_link_libraries(envoy dl)

 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})
+include_directories(SYSTEM ${ENVOY_RAPIDJSON_INCLUDE_DIR})


why is this needed?

Removed. It was due to the initial integration of rapidjson. No longer needed now that rapidjson headers are only in the .cc file.

mattklein123 · 2016-11-09T23:07:29Z

source/server/CMakeLists.txt

@@ -24,6 +24,7 @@ include_directories(SYSTEM ${ENVOY_HTTP_PARSER_INCLUDE_DIR})
 include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR})
+include_directories(SYSTEM ${ENVOY_RAPIDJSON_INCLUDE_DIR})


why is this needed?

It made the compiler happy, back when I was playing with it. I don't think this will be necessary once I find a way to get rid of the refernce to rapidjson/document.h in the json_loader.h

mattklein123 · 2016-11-09T23:07:50Z

source/common/json/json_loader.h

@@ -128,20 +136,10 @@ class Object {
  bool hasObject(const std::string& name) const;

 protected:
-  Object() : name_("root") {}
+  Object() : name_("root"), document_() {}


default initializer not needed

mattklein123 · 2016-11-09T23:08:08Z

source/common/json/json_loader.h

+  }
+
+  // Empty Object
+  Object(const std::string& name) : name_(name), document_() {}


default initializer not needed

mattklein123 · 2016-11-09T23:08:24Z

source/common/json/json_loader.h

@@ -25,7 +28,12 @@ typedef std::function<bool(const std::string&, const Object&)> ObjectCallback;
 */
 class Object {
 public:
-  Object(json_t* json, const std::string& name) : json_(json), name_(name) {}
+  Object(const rapidjson::Value& value, const std::string& name) : name_(name), document_() {


default initializer not needed

mattklein123 · 2016-11-09T23:09:05Z

source/common/json/json_loader.h

-struct json_t;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wold-style-cast"
+#include "rapidjson/document.h"


Do not leak this in header. Find a way around this even if you need to expose an interface.

mattklein123 · 2016-11-09T23:11:43Z

source/common/json/json_loader.cc

@@ -172,7 +160,7 @@ void Object::iterate(const ObjectCallback& callback) {
 }

 bool Object::hasObject(const std::string& name) const {
-  return json_object_get(json_, name.c_str()) != nullptr;
+  return document_.HasMember(name.c_str()) && document_.IsObject();


Don't know API but seems wrong to be calling document_.IsObject() here.

mattklein123 · 2016-11-09T23:13:12Z

source/common/json/json_loader.cc

  }

  return string_array;
 }

 double Object::getDouble(const std::string& name) const {
-  json_t* real = json_object_get(json_, name.c_str());
-  if (!real || !json_is_real(real)) {
+  if (!document_.HasMember(name.c_str()) || !document_[name.c_str()].IsDouble()) {


can you avoid multiple hash lookups here and elsewhere

Removed multiple hash lookups.

RomanDzhabarov · 2016-11-10T00:08:43Z

source/common/json/json_loader.cc

 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wold-style-cast"
-#include "jansson.h"
+#include <rapidjson/error/en.h>


minor: s/</"

RomanDzhabarov · 2016-11-21T23:52:05Z

include/envoy/json/json_object.h

+
+  /**
+   * Get a boolean value by name.
+   * @param name supplies the  key name.


nit: two spaces :-)

RomanDzhabarov · 2016-11-21T23:55:19Z

source/common/json/json_loader.cc


-Object::EmptyObject::~EmptyObject() { json_decref(json_); }
+    std::vector<AbstractObjectPtr> object_array;


nit, i'd reserve vector capacity upfront as we know the Size.

Fixed here and in json_loader.h

RomanDzhabarov · 2016-11-22T00:10:58Z

source/common/json/json_loader.cc

-  for (size_t i = 0; i < json_array_size(json_); i++) {
-    object_array.emplace_back(json_array_get(json_, i), name_ + " (array item)");
+  bool getBoolean(const std::string& name) const override {
+    if (!document_[name.c_str()].IsBool()) {


not to familiar with api, but can we

auto &blah = document_[name.c_str()];
and query internal map or something once.

if (!blah.IsBool()) {
}

return blah.GetBool();

mattklein123 · 2016-11-22T02:06:03Z

docs/install/requirements.rst

@@ -14,10 +14,11 @@ Envoy has the following requirements:
 * `libevent <http://libevent.org/>`_ (last tested with 2.0.22)
 * `tclap <http://tclap.sourceforge.net/>`_ (last tested with 1.2.1)
 * `gperftools <https://github.com/gperftools/gperftools>`_ (last tested with 2.5.0)
-* `jansson <https://github.com/akheron/jansson>`_ (last tesed with 2.7)
-* `openssl <https://www.openssl.org/>`_ (last tesed with 1.0.2i)
+* `jansson <https://github.com/akheron/jansson>`_ (last tested with 2.7)


delete all references to jansson from docs, cmake, etc.

I will do that clean up in Jira 615.

mattklein123 · 2016-11-22T02:06:29Z

include/envoy/json/json_object.h

+  /**
+   * Convert a generic object into an array of objects. This is useful for dealing with arrays
+   * of arrays.
+   * @return std::vector<Object> the converted object.


mattklein123 · 2016-11-22T02:06:43Z

include/envoy/json/json_object.h

+   * Get a sub-object by name.
+   * @param name supplies the key name.
+   * @param allow_empty supplies whether to return an empty object if the key does not exist.
+   * @return Object the sub-object.


mattklein123 · 2016-11-22T02:06:50Z

include/envoy/json/json_object.h

+  /**
+   * Get an array by name.
+   * @param name supplies the key name.
+   * @return std::vector<Object> the array of JSON objects.


mattklein123 · 2016-11-22T02:07:25Z

include/envoy/json/json_object.h

+  virtual double getDouble(const std::string& name, double default_value) const PURE;
+
+  /**
+   * Iterate Object and call callback on key-value pairs


wrong type / please fix grammar

mattklein123 · 2016-11-22T02:09:45Z

include/envoy/json/json_object.h

+  /**
+   * @return a vector of the AbstractObject that are members of the currect AbstractObject.
+   */
+  virtual std::vector<AbstractObjectPtr> getMembers() const PURE;


can't this be implemented in terms of iterate? Why is this needed?

Or get rid of iterate() from the public interface

mattklein123 · 2016-11-22T02:12:32Z

source/common/json/json_loader.cc

+class AbstractObjectImpl : public AbstractObject {
+public:
+  AbstractObjectImpl(const rapidjson::Document& document, const std::string& name) : name_(name) {
+    document_.CopyFrom(document, document_.GetAllocator());


We should not be copying anything. If needed a reference to the parent document should be used.

mattklein123 · 2016-11-22T02:13:08Z

source/common/json/json_loader.cc

-  if (!json_) {
-    throw Exception(fmt::format("json parsing error: {}", error.text));
+  AbstractObjectImpl(const rapidjson::Value& value, const std::string& name) : name_(name) {
+    document_.CopyFrom(value, document_.GetAllocator());


mattklein123 · 2016-11-22T02:14:26Z

test/common/json/json_loader_test.cc

@@ -122,11 +124,6 @@ TEST(JsonLoaderTest, Integer) {
    EXPECT_EQ(std::numeric_limits<int64_t>::max(), json.getInteger("max"));
    EXPECT_EQ(std::numeric_limits<int64_t>::min(), json.getInteger("min"));
  }
-
-  {


what was this test testing and why is it no longer failing

This was testing Max and Min Integer. Unfortunately, rapidjson doesn't throw an exception during Parsing. We should push this validation to the schema. :(

mattklein123 · 2016-11-22T02:14:45Z

source/common/json/json_loader.h

 };

 /**
 * Loads JSON from a string.
 */
 class StringLoader : NonCopyable, public Object {
 public:
-  StringLoader(const std::string& json);
-  ~StringLoader();
+  StringLoader(const std::string& json) { abstract_object_ = Factory::LoadFromString(json); }


initializer list

mattklein123 · 2016-11-22T02:14:50Z

source/common/json/json_loader.h

 };

 /**
 * Loads a JSON file into memory.
 */
 class FileLoader : NonCopyable, public Object {
 public:
-  FileLoader(const std::string& file_path);
-  ~FileLoader();
+  FileLoader(const std::string& file_path) { abstract_object_ = Factory::LoadFromFile(file_path); }


initializer list

mattklein123 · 2016-11-22T02:15:56Z

source/common/json/json_loader.h

+
+  std::vector<Object> getObjectArray(const std::string& name) const {
+    std::vector<AbstractObjectPtr> abstract_object_array = abstract_object_->getObjectArray(name);
+    std::vector<Object> return_vector;


reserve capacity ahead of time

mattklein123 · 2016-11-22T02:16:10Z

source/common/json/json_loader.h

+  }
+
+  Object getObject(const std::string& name, bool allow_empty = false) const {
+    AbstractObjectPtr abstract_object = abstract_object_->getObject(name, allow_empty);


local var not needed

removed extra local var.

mattklein123 · 2016-11-22T02:16:20Z

source/common/json/json_loader.h

+
+  std::vector<Object> asObjectArray() const {
+    std::vector<AbstractObjectPtr> abstract_object_array = abstract_object_->asObjectArray();
+    std::vector<Object> return_vector;


mattklein123 · 2016-11-22T02:19:26Z

source/common/json/json_loader.h

+  Object() {}
+
+  Object(AbstractObjectPtr&& abstract_object_ptr) {
+    abstract_object_ = std::move(abstract_object_ptr);


initializer list

mattklein123 · 2016-11-22T02:19:38Z

source/common/json/json_loader.cc

-    Object object_value(value, "root");
-    std::string object_key(key);
+private:
+  std::string name_;


mattklein123 · 2016-11-30T01:15:09Z

source/common/json/json_loader.h

 namespace Json {
+class Object;


split object out into public include and only define the factory stuff in this file.

mattklein123 · 2016-11-30T01:20:38Z

test/common/json/json_loader_test.cc

-  }
-
-  {
-    EXPECT_THROW(StringLoader("{\"val\":9223372036854775808}"), Exception);


What actually happens here? We shouldn't delete this test.

…#205)

* worker: provide removeFilterChain interface (envoyproxy#10528) Signed-off-by: Yuchen Dai <[email protected]> * cherry-pick in place filter chain update Signed-off-by: Yuchen Dai <[email protected]> * fix conficts Signed-off-by: Yuchen Dai <[email protected]>

Signed-off-by: Jose Nino [email protected] Description: previously the CI runs would only build the demos. However we did not run them, or check for liveliness. This PR adds running all the Android demos in the emulator for Mac CI. Risk Level: low - improved CI Testing: tested locally and on CI Fixes #117 Signed-off-by: JP Simard <[email protected]>

Co-authored-by: Matteo Pace <[email protected]>

mattklein123 reviewed Nov 9, 2016

View reviewed changes

RomanDzhabarov reviewed Nov 10, 2016

View reviewed changes

ccaraman force-pushed the rapidjson-parity branch from b92feb0 to bac2199 Compare November 21, 2016 22:09

RomanDzhabarov reviewed Nov 22, 2016

View reviewed changes

mattklein123 reviewed Nov 22, 2016

View reviewed changes

ccaraman added 16 commits November 29, 2016 16:35

initial

131eabe

two

a6c4233

building!

a96e8b8

cleanish

0934352

fix headers

0304a73

clean

d5f629a

destructor

7759382

fmt format

dd52b40

documentation

9d21f6a

reference

4e43247

shared ptr

25ed966

address prr

84f0976

comments

9065001

push

ba21f10

finally

c93eca2

merge

424f6db

ccaraman force-pushed the rapidjson-parity branch from f311925 to 424f6db Compare November 30, 2016 01:04

mattklein123 reviewed Nov 30, 2016

View reviewed changes

one more

64bf2db

mattklein123 approved these changes Nov 30, 2016

View reviewed changes

mattklein123 merged commit 88ed42a into master Nov 30, 2016

mattklein123 deleted the rapidjson-parity branch November 30, 2016 03:04

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

add mixer_client_config_proto build target for istio/api. (envoyproxy…

b2d9149

…#205)

arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024

Fix usage examples in README. (envoyproxy#205)

f5f890a

Co-authored-by: Matteo Pace <[email protected]>


		Object::EmptyObject::~EmptyObject() { json_decref(json_); }
		std::vector<AbstractObjectPtr> object_array;

Rapidjson parity #205

Rapidjson parity #205

Conversation

ccaraman commented Nov 9, 2016

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

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

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

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