config: replace dom parser with sax parser #972

danielhochman · 2017-05-16T00:06:48Z

The main benefit of using the SAX parser is that we get line numbers in error messages by using a custom stream during parsing.

New error:

JSON at lines 4-6 does not conform to schema.
 Schema violation: pattern
 Offending document key: #/name

The diff is not fun to look through given the changes to some widely used variable names. Pretty much all meaningful code changes are here: https://github.com/lyft/envoy/pull/972/files#diff-33ef08aee21cc1f90b7a1ff26051aa6b

TODO:

fix remaining tests (failing tests are asserting on the old error message format, want to make sure everyone is happy with it first)
added more tests on exception message format

ccaraman · 2017-05-16T00:10:45Z

source/common/json/json_loader.h

 #include <string>

 #include "envoy/json/json_object.h"

+// Do not let RapidJson leak outside json_loader.


I guess this can't be moved into a header/cc file. Sorry I didn't see the rapidjson::Document in your member variables earlier.

Please move this back into json_loader.cc.

ccaraman · 2017-05-16T00:11:37Z

source/common/json/json_loader.cc

 #include "spdlog/spdlog.h"

-// Do not let RapidJson leak outside of this file.
+// Do not let RapidJson leak outside of json_loader.


The old comment is correct, but to make it more clear, you can change it to json_loader.cc

RomanDzhabarov · 2017-05-16T04:32:37Z

source/common/json/json_loader.h

+      bool boolean_value_;
+      double double_value_;
+      int64_t integer_value_;
+      std::map<const std::string, FieldPtr> object_value_;


why not std::unordered_map?

htuch · 2017-05-16T14:29:34Z

source/common/json/json_loader.h

+   * Internal representation of Object.
+   */
+  class Field;
+  typedef std::shared_ptr<Field> FieldPtr;


FieldSharedPtr

htuch · 2017-05-16T14:39:21Z

source/common/json/json_loader.h

+  class Field;
+  typedef std::shared_ptr<Field> FieldPtr;
+
+  class Field : public Object, public std::enable_shared_from_this<Field> {


General preference for not having large nested classes (and series of nested classes). It becomes hard to track the outer class. Consider using a namespace (anonymous if inside .cc).

ah cool was not aware of this feature. new to the language. will make that change.

htuch · 2017-05-16T14:40:59Z

source/common/json/json_loader.h

+    bool isType(Type type) const { return type == type_; }
+    void checkType(Type type) const {
+      if (!isType(type)) {
+        throw Exception("Field access in JSON with correct type.");


Nit: incorrect type

Also, is there a test for this? Can we add the type name to the error string?

htuch

Thanks for the improved error messages!

htuch · 2017-05-16T14:41:24Z

source/common/json/json_loader.h

+      }
+      return ret;
+    }
+    uint64_t getLineNumber() { return line_number_; }


htuch · 2017-05-16T14:42:18Z

source/common/json/json_loader.h

+    bool isType(Type type) const { return type == type_; }
+    void checkType(Type type) const {
+      if (!isType(type)) {
+        throw Exception("Field access in JSON with correct type.");


Also, is there a test for this? Can we add the type name to the error string?

htuch · 2017-05-16T14:48:00Z

source/common/json/json_loader.cc

+  } else {
+    return default_value;
+  }
+}


These getters are all kind of boiler platish, consider a template function implementation.

htuch · 2017-05-16T14:49:16Z

source/common/json/json_loader.cc

+  std::vector<std::string> string_array;
+  string_array.reserve(array.size());
+  for (const auto& element : array) {
+    if (!element > isType(Type::String)) {


Nit: this expression is hard to read, there's negation, inequality and on the RHS a predicate. Can this be made clearer?

htuch · 2017-05-16T14:53:16Z

source/common/json/json_loader.cc

+  array->setLineNumberStart(stream_.getLineNumber());
+
+  switch (state_) {
+  case expectValueOrStartObjectArray:


Can some of this be refactored with StartObject?

ccaraman · 2017-05-16T16:31:07Z

include/envoy/json/json_object.h

@@ -12,7 +12,7 @@
 namespace Json {
 class Object;

-typedef std::unique_ptr<Object> ObjectPtr;
+typedef std::shared_ptr<Object> ObjectPtr;


nit: ObjectSharedPtr

ccaraman · 2017-05-16T17:19:32Z

source/common/json/json_loader.h

+        throw Exception("Field access in JSON with correct type.");
+      }
+    }
+    // value rtype funcs


rtype = return type?

RomanDzhabarov · 2017-05-16T18:25:52Z

source/common/json/json_loader.h

+    // Ch is typdef in parent class to handle character encoding.
+  public:
+    LineCountingStringStream(const Ch* src) : rapidjson::StringStream(src), line_number_(1) {}
+    Ch Take() {


is that an override of method from rapidjson::StringStream? (if so Ch Take() override)

only virtual member functions can be marked 'override'

yeah, that's why i asked if Take is defined on the rapidjson::StringStream as virtual.
if no, nvm

ccaraman · 2017-05-16T18:33:17Z

source/common/json/json_loader.h

+    }
+
+    rapidjson::Document asRapidJsonDocument() const;
+    static void build(const Field& field, rapidjson::Value& value,


Can you add a comment here on what this does?

danielhochman · 2017-05-16T20:56:18Z

@htutch

Can some of [StartArray] be refactored with StartObject?

Not seeing a way to do this without making the code less clear than it is now. The differences are there enough to make it worth keeping the impl separate. Open to suggestions though.

These getters are all kind of boiler platish, consider a template function implementation.

I based the getter functions on bson_impl which @mattklein123 wrote which favors repetition over templates.

mattklein123 · 2017-05-16T21:34:22Z

test/tools/router_check/router.cc

@@ -102,7 +102,7 @@ bool RouterCheckTool::compareEntriesInJson(const std::string& expected_route_jso
    }

    if (validate->hasObject("header_fields")) {
-      for (const Json::ObjectPtr& header_field : validate->getObjectArray("header_fields")) {
+      for (const Json::ObjectSharedPtr& header_field : validate->getObjectArray("header_fields")) {


per offline convo, I would global find/replace Json::ObjectSharedPtr& with Json::ObjectSharedPtr for safety.

ccaraman · 2017-05-16T22:37:14Z

source/common/json/json_loader.cc

  }
+  default:
+    throw Exception("Json has a non-object or non-array at the root.");


EnvoyException instead of Exception

this is a Json::Exception

Nvm I forgot about the Json::Exception.

mattklein123

Looks great. Per offline convo, please make sure there is a test for a partial but valid object. Also, did we fix the case where the JSON is just an array? I think that was broken in old code.

mattklein123 · 2017-05-16T23:09:00Z

source/common/json/json_loader.cc

-      throw Exception(fmt::format("'{}' is not an array", name_));
-    }
+  // Container factories for handler.
+  static FieldSharedPtr createObject() { return FieldSharedPtr{new Field(Type::Object)}; }


let's use std::make_shared here and everywhere else we are making new shared_ptr inline.

mattklein123 · 2017-05-16T23:16:16Z

source/common/json/json_loader.cc

+  Value value_;
+};
+
+/*


same elsewhere

mattklein123 · 2017-05-16T23:18:05Z

source/common/json/json_loader.cc

+
+  uint64_t line_number_start_;
+  uint64_t line_number_end_;
+  Type type_;


can this be const?

mattklein123 · 2017-05-16T23:20:59Z

source/common/json/json_loader.cc

+  return std::hash<std::string>{}(buffer.GetString());
+}
+
+bool Field::getBoolean(const std::string& name) const {


Can we add TODO here to add line number info to all these errors (or just do now)

mattklein123 · 2017-05-16T23:30:29Z

source/common/json/json_loader.cc

+    state_ = expectKeyOrEndObject;
+    return true;
+  default:
+    return false;


per offline convo, lets do NOT_REACHED here since this should never happen and is a programming error (same elsewhere). I would add comment also on why.

Same elsewhere if this applies.

mattklein123 · 2017-05-17T00:32:24Z

Per offline convo there are more error messages that can have line numbers so those will get added also.

RomanDzhabarov · 2017-05-17T01:00:55Z

source/common/json/json_loader.cc

@@ -521,7 +523,7 @@ bool ObjectHandler::StartObject() {
    state_ = expectKeyOrEndObject;
    return true;
  default:


we should delete default completely from the switch() as it must never happen.

the compiler will enforce that all types of state_ are present in the case statements. (could place NOT_REACHED at the very end after switch)

there are some other entries like that

not all states are in the case statement. the NOT_REACHED will only be hit if something unexpected happens in the state machine (e.g. due to unexpected sequence of calls from rapidjson).

the idea of NOT_REACHED is to make the compiler happy in cases we know something will not happen rather than detect cases like that.

In this case, it feels more like InvalidStateException or something like that (i do not think we really have a variety of exception types in our code base), so throwing Json::Exception might make sense, and specify state_ as part of the exception message.

This case must crash the process, so throwing a Json::Exception, or any exception, is not appropriate. Especially if it cannot be tested without modifying the rapidjson code. Using NOT_REACHED is fine in this case.

mattklein123

LGTM. We can merge tomorrow in case anyone has any final comments. This is great!

RomanDzhabarov · 2017-05-17T17:18:32Z

+1

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Daniel Hochman added 6 commits May 15, 2017 14:19

move to SAX parser

d894af2

Merge branch 'master' of github.com:lyft/envoy into rapidjson-sax

21e7e53

use Json::Exception

c92473e

null support, error improvements

2f7460c

reorganize into header

564b2b4

comment and exception fixes

db9d15e

ccaraman reviewed May 16, 2017

View reviewed changes

RomanDzhabarov reviewed May 16, 2017

View reviewed changes

htuch reviewed May 16, 2017

View reviewed changes

ccaraman reviewed May 16, 2017

View reviewed changes

move code back to .cc in unnamed namespace

d2ef882

RomanDzhabarov reviewed May 16, 2017

View reviewed changes

ccaraman reviewed May 16, 2017

View reviewed changes

Daniel Hochman added 4 commits May 16, 2017 12:02

most feedback

0f49edd

fix tests and add new ones

34c6bae

rename ObjectPtr to ObjectSharedPtr

c19d386

Merge branch 'master' of github.com:lyft/envoy into rapidjson-sax

6583dc4

Daniel Hochman added 2 commits May 16, 2017 14:16

add default for gcc

d41e88e

use NOT_REACHED instead of default

25a6767

mattklein123 reviewed May 16, 2017

View reviewed changes

never use reference to ObjectSharedPtr

c53f5e1

ccaraman reviewed May 16, 2017

View reviewed changes

some more test coverage

f9a52b5

mattklein123 reviewed May 16, 2017

View reviewed changes

line numbers in all error messages

5b64409

more test coverage

449247f

RomanDzhabarov reviewed May 17, 2017

View reviewed changes

fix new test with missing EXPECT

93ff5cd

mattklein123 approved these changes May 17, 2017

View reviewed changes

ccaraman approved these changes May 17, 2017

View reviewed changes

mattklein123 merged commit 3511758 into master May 17, 2017

mattklein123 deleted the rapidjson-sax branch May 17, 2017 17:23

mattklein123 mentioned this pull request May 17, 2017

config: make error messages less bad #686

Closed

jpsim pushed a commit that referenced this pull request Nov 28, 2022

release: v0.3.1 (#972)

b28a412

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

release: v0.3.1 (#972)

2c023cf

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

config: replace dom parser with sax parser #972

config: replace dom parser with sax parser #972

Conversation

danielhochman commented May 16, 2017 • edited Loading

ccaraman May 16, 2017 • edited Loading

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

htuch 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

RomanDzhabarov May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhochman commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhochman May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 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

mattklein123 commented May 17, 2017

RomanDzhabarov May 17, 2017 • edited Loading

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

mattklein123 left a comment

Choose a reason for hiding this comment

RomanDzhabarov commented May 17, 2017

danielhochman commented May 16, 2017 •

edited

Loading

ccaraman May 16, 2017 •

edited

Loading

RomanDzhabarov May 16, 2017 •

edited

Loading

danielhochman May 16, 2017 •

edited

Loading

RomanDzhabarov May 17, 2017 •

edited

Loading