Support SAX-style unpack API

[jpetso]

It would be super neat if I could use msgpack as a pure wire format reader/visitor without having to create a result msgpack::object.

For instance, my class msgpack_from_json_handler can be passed to rapidjson::Reader::Parse() as a parameter and will be called back on every element in the JSON tree.

With unpack_reference_func(), I can avoid copying all string data with a zone. However, I still have to let msgpack generate a msgpack::object tree, with allocations depending on the non-leaf objects / depth of the tree. In some situations, a simple visitor pattern would work better.

Here are two example use cases:

• Converting msgpack to a custom type/variant without allocations ("use case 1")

Elvis's library deals mostly with JSON data. Internally he stores much of his data as RapidJSON variant values. He decides he also wants to support serialialization to and from msgpack. He needs to process lots of data and packing/unpacking is a hot code path, so it needs to perform as fast as possible. For serializing to msgpack, he can use pack()/pack_map()/pack_array() functions and json-msgpack. He also wants to be able to read directly from a msgpack buffer into a RapidJSON variant, skipping the intermediate step of the msgpack::object because his variant can represent the same data in a format that the rest of his code uses.

[Note that instead of unpacking into JSON, one could also make use cases for msgpack::type::variant, known user-defined message objects, etc.]

• Checking msgpack contents without unpacking them ("use case 2")

Jakob wants to implement a SQLite extension function that checks if a serialized msgpack buffer (stored in a column of a SQLite table) contains a certain string. He wants to issue SQL queries such as, SELECT * FROM table WHERE msgpack_array_contains(msgpack_column, id) and implement the msgpack_column() extension function in the most efficient way, because it will be executed for a lot of rows. He would like to write a visitor class that gets called on each item and aborts unpacking when an array item equals id (after setting a found flag to true), or when the top-level msgpack type is something other than ARRAY. He does not care about a return type, neither msgpack::object nor any other variant.

I think RapidJSON's SAX interface (passed to rapidjson::Reader::Parse()) would be a pretty good starting point, with minor adaptations required to suit msgpack's object model. Here's an initial draft of a (template) interface for C++ classes implementing a msgpack visitor, and a suitable unpack() overload:

struct unpack_visitor {
    bool visit_nil();
    bool visit_boolean(bool);
    bool visit_positive_integer(uint64_t);
    bool visit_negative_integer(int64_t);
    bool visit_float(double);
    bool visit_str(const char*, uint32_t size, bool needs_copy); // still not sure why you're not using size_t
    bool visit_bin(const char*, uint32_t size, bool needs_copy);
    bool visit_ext(int8_t type, const char* data, uint32_t size, bool needs_copy);
    bool start_array(uint32_t num_elements);
    bool end_array();
    bool start_map(uint32_t num_kv_pairs);
    bool start_map_key(); // this one could be void, but since all others are bool, might as well keep this one too
    bool end_map_key();
    bool end_map();
    void parse_error(size_t parsed_offset, size_t error_offset);
    void insufficient_bytes(size_t parsed_offset, size_t error_offset);
}

template <typename UnpackVisitor>
bool unpack(const char* data, size_t len, size_t& off, UnpackVisitor&);

[Edit: changed begin_*() to start_*() as RapidJSON, Qt and the Java SAX API all use start as well.]

I prepended visit_ to single-value functions so we don't get issues with keywords (bool float(double);) or the Apple nil define, and msgpack's lower-case naming can still be employed.

The bool return value of all of those represents a successful parse (true for "continue parsing", false for "parse error"). The unpack() function is specified in a way that an exception-less interface can be built with it and all current unpack() functions can be implemented with this unpack() overload and a visitor:

• The zone and stack of result msgpack objects can be members of the visitor object, which uses/writes/manages them.
• Stream-level errors (parse_error and insufficient_bytes) are called as error functions in the visitor, which can decide to either throw or store them as state in the visitor object (and do custom error handling afterwards).
  • For use case 2: A caller who does avoids throwing exceptions can distinguish between legitimate parse errors and "found, now abort" early return situations by determining whether error functions have been called. If I know that my object is valid msgpack, an early return will save me some reads and processing time.
  • I chose to include offset parameters for the error functions, because it could be useful for error reporting (invalid bytes range from parsed_offset to error_offset). error_offset is the same value as the off parameter in unpack(). When used within an unpacker, parsed_offset equals parsed_size() and error_offset equals parsed_size() + nonparsed_size() (assuming I understand the purpose of these values correctly).
• unpack_limit and size_overflow errors can be thrown by the visitor functions themselves.
• unpack_reference_func can be called by visit_str(), visit_bin() and visit_ext().
  • user_data can be a member of the visitor class.
• unpacker can't be implemented on top of this overload because unpack() doesn't take a parse context object. (The visitor object likely contains a stack of msgpack objects, but does not tell the calling parser about the stack depth. That's a good thing because it makes the visitor object more lightweight and easier to implement if no stack information is necessary.) However, it should be possible to use the same visitor object for unpacker as for regular unpack() overloads, assuming that insufficient_bytes() doesn't throw.
• The off result parameter might be important for error messages. It's required here in addition to the visitor's error functions because if false is returned from a visit_*()/start_*()/end_*() function, the visitor itself does not know the current offset so it can't store it as state. Hence unpack() itself must maintain and return it.

Personally, I find this interface a lot more approachable and easier to understand than unpack_reference_func and unpack_limit. Even though it is simpler in concept, it is also more flexible. I hope something like this can be supported by msgpack-c.

[redboltz]

@jpetso , it seems to be a nice idea :)

The visitor interface could be a low-level public unpacking interface, and current unpack() implementation could be build on the visitor interface as you mentioned. I prefer to the design.

I answer the following comment:

bool visit_str(const char*, uint32_t size, bool needs_copy); // still not sure why you're not using size_t

I choose uint32_t intentionally because the maximum length of STR in msgpack format is 32bit.
https://github.com/msgpack/msgpack/blob/master/spec.md#formats-str

The size of size_t is implementation defined. I want to make sure the maximum size of STR, so I choose uint32_t. I had considered choosing uint_fast32_t but some environment don't support that.

I have some questions:

• Why do start_map_key and end_map_key exist? After calling start_map, two msgpack objects, key and value, are expected. A client can store a counter in a visitor class. If you want to avoid that and provide easier interface, why don't you provide start_map_value and end_map_value?

The bool return value of all of those represents a successful parse (true for "continue parsing", false for "parse error").

• What does "parse error" mean in the context? The value of the msgpack object is unexpected?

The off result parameter might be important for error messages. It's required here in addition to the visitor's error functions because if false is returned from a visit__()/start__()/end_*() function, the visitor itself does not know the current offset so it can't store it as state. Hence unpack() itself must maintain and return it.

• if false is returned from a visit__()/start__()/end_*() function, shouldn't off be updated? Current unpack() manages off as https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/unpack.hpp#L1776, https://github.com/msgpack/msgpack-c/blob/master/src/unpack.c#L538.

[jpetso]

I choose uint32_t intentionally because the maximum length of STR in msgpack format is 32bit.
https://github.com/msgpack/msgpack/blob/master/spec.md#formats-str

Okay, thanks for explaining!

Why do start_map_key and end_map_key exist? After calling start_map, two msgpack objects, key and value, are expected. A client can store a counter in a visitor class. If you want to avoid that and provide easier interface, why don't you provide start_map_value and end_map_value?

You're right, it feels a bit inconsistent and I'm not totally happy with that part of my current proposal. The background is that RapidJSON has a Key() function which is really the same as String() but called for object (map) keys rather than values. In contrast, any type can be a key in msgpack, including complex types, so having separate functions or extra parameters for all of the types doesn't seem like the best kind of solution.

Counting the position in the map is certainly possible. I thought maybe an explicit function can help reduce the effort of keeping track, although I haven't fully thought through what a visitor with nested keys and/or values would look like and how it would keep track of its state. My initial thinking was that if you write a visitor, you can determine whether you're in the key or value part by having state that ends up in a boolean of whether you're currently in the key part or not. If not, then it's a value.

My thinking is that conceptually, key and value probably count as separate "attributes" so having start/end visitors is the safe/conservative approach. Empty inlined functions will get optimized out by compilers at even minimal optimization settings, so the only thing this adds is a few extra functions that might or might not be used. I think the interface should rather err on the "convenient but not always used" side and include start/stop_map_key(), and for consistency, as you point out, it doesn't hurt much (other than requiring more functions) to add start/stop_map_value().

What does "parse error" mean in the context? The value of the msgpack object is unexpected?

Yes. "Parse error" in this context meant any error that's not related to the wire format itself. What exactly is considered a parse error here will depend on the visitor and can include:

• Allocation errors
• Limits on the number or size of elements
• For custom visitors, semantic errors in the data format, e.g. "unexpected key 'foobar'", or "expected string type, found positive integer instead"

To avoid confusion, I guess this kind of error should not be called "parse error" because that already describes a parse error in the wire format itself. Let's perhaps stick to "unpack errors" in this case, which includes all of the above but does not overload the meaning of "parse error"?

if false is returned from a visit_()/start_()/end_*() function, shouldn't off be updated? Current unpack() manages off as https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/unpack.hpp#L1776, https://github.com/msgpack/msgpack-c/blob/master/src/unpack.c#L538.

Yes, I think off should be managed internally by unpack() and updated on false returns but not exposed to the visitor otherwise (or only to the visitor's error functions). It should be done in the same way that it is managed now - functions such as unpack_uint8() or unpack_str() are the equivalent to the visitor functions, and they also don't have knowledge of the offset, but it still gets updated by the wire format parser itself. Sorry if that was unclear.

I also have to admit that I didn't check how and when exactly off gets advanced, but either way I think its value should be exactly the same as for current unpack() functions, otherwise things would get more confusing.

[redboltz]

@jpetso, sorry for my late reply. I had been tied up with preparation of the version 2.0 and other issues. As you pointed out, MessagePack allows any MessagePack object as a key of map.

I think that introducing start/stop_map_value() makes things simpler. After introducing that, there are two candidates.

Approach A

start_map();
start_map_key();
...
stop_map_key();
start_map_value();
...
start_map_value();
stop_map();

Approach B

start_map_key();
...
stop_map_key();
start_map_value();
...
start_map_value();

I think that Approach A is redundant. So I prefer to Approach B. What do you think?

I come up with another question. It's about needs_copy.

bool visit_str(const char*, uint32_t size, bool needs_copy);

You said as follows:

unpack_reference_func can be called by visit_str(), visit_bin() and visit_ext().

I think that unpack-core, visitor caller, cannot pass the parameter needs_copy. Requiring copy or not is decided by the result of unpack_reference_func at least current implementation (creating msgpack::object).

[redboltz]

I realized that I misunderstood start/stop_map(). Now I think that Approach A is better. Sorry for the mix-up.

[redboltz]

I started implementing the feature.

[jpetso]

I think that unpack-core, visitor caller, cannot pass the parameter needs_copy. Requiring copy or not is decided by the result of unpack_reference_func at least current implementation (creating msgpack::object).

Yes, that's what I meant: In order to implement the current unpack() implementation on top of the visitor interface, the implementation of the visitor interface can determine needs_copy by calling the user-provided unpack_reference_func. I can then use this to create the msgpack::object accordingly. I didn't intend to say that the visitor implementation can come up with needs_copy by itself, only that visit_str(), visit_bin() and visit_ext() can make use of unpack_reference_func to implement the existing interface.

Thank you for spending time on this!

[jpetso]

Oh, and I am in favor of Approach A as well... assuming there are no misunderstandings on my side 😃

[redboltz]

I've implemented almost all functionalities. It works well on my local environment. I come up with a question.

    void parse_error(size_t parsed_offset, size_t error_offset);
    void insufficient_bytes(size_t parsed_offset, size_t error_offset);

What are parsed_offset and error_offset? I think that a parse error only happens when the iterator encounters 0xc1. See https://github.com/msgpack/msgpack/blob/master/spec.md

Let's see the following case:

0x93, 0x01, 0xc1, 0x03

It is an array that contains three elements. And the second element is bad. What are the appropriate values for parsed_offset and error_offset?

Candidates are

parsed_offset	error_offset
0	0
0	2
1	2

In other expressions

parsed_offset	error_offset
off	off
off	bad point
one byte before bad point	bad point

Note: off is the original offset value.

I think that the third one is meaningful.

How about insufficient_bytes?

[redboltz]

I come up with another question.

bool unpack(const char* data, size_t len, size_t& off, UnpackVisitor&);

What does bool return value mean? I think that it can be void. Because caller can store any return values in UnpackVisitor. Maybe caller can get the unpacked situation using len, off, and UnpackVisitor.

[jpetso]

bool unpack(const char* data, size_t len, size_t& off, UnpackVisitor&);

What does bool return value mean?

My thinking was that the return value can be similar to QXmlSimpleReader::parse():
"Returns true if the parsing was successful; otherwise returns false."

You're right in that the UnpackVisitor can always store data about errors, so strictly speaking the bool return value wouldn't be necessary. Its intention is to make life easier for the caller. These are the two main thoughts behind having the return value:

1. Without return value, the UnpackVisitor implementation must set the error in each of the visitor methods and parse_error()/insufficient_bytes(), making it unreasonably hard to have a simple centralized check for whether or not parsing succeeded.
2. A return value allows a single-line unpack-and-check statement:

MyVisitor visitor;
if (!msgpack::unpack(data, length, offset, visitor)) {
    std::cerr << visitor.error_message();
}

Different libraries have different approaches to enable 2.:

• RapidJSON's Reader::Parse() returns a boolean like this one. In addition, their Reader object has additional information about the error offset, error code and message. My API draft was based on their API and so it uses/returns the same values, only I changed it to be a free function (without class/object) because unpack() has been msgpack-c's main low-level parsing primitive.
• Qt's QXMLSimpleReader::parse() returns a similar bool result value. For detailed error reporting, one can set a QXMLErrorHandler that will get called with warning(), error() and/or fatalError() methods by the reader. This is a pretty intriguing design because it allows the caller to check whether an XML document is well-formed by simply calling parse() without setting either a visitor or an error handler, and the caller can add more handling/complexity if desired.
• Java's XMLReader.parse() doesn't have a return value, but throws a SAXException or an IOException on error. That way they can put parse() inside a try/catch block, which would be the idiomatic Java version for handling parse errors. For the msgpack API, I wanted to avoid exceptions on the lowest level so that low-featured embedded environments can use this unpack() overload without requiring RTTI/unwinding.

An alternative to the proposed API would be to call a single error method on the UnpackVisitor so the caller only needs to set the parsing state in a single location:

enum class /*msgpack::*/unpack_errc { // unpack_error is already taken :-P
    parse_error,
    insufficient_bytes,
    user_error // when returning false from a visitor method
};

// in UnpackVisitor, instead of parse_error() and insufficient_bytes():
void error(msgpack::unpack_errc, size_t parsed_offset, size_t error_offset);

This would solve 1. from above, but 2. is still questionable, and the user still has to define a boolean such as has_parse_error inside the visitor. Given that the unpack() overload (I just saw that you named it unpack_visit()) already has all the information and only needs to return unpack_visit_imp() == UNPACK_SUCCESS;, it seems like that's a low price to pay for some extra convenience.

[jpetso]

Ah yes, I was going to say that an alternative to the bool return value for a single-line unpack-and-check statement could be this:

template <typename UnpackVisitor>
UnpackVisitor& unpack_visit(const char* data, size_t len, size_t& off, UnpackVisitor&);

This would allow the caller to define access to a member variable or function, so they could do this:

struct MyVisitor {
    bool has_parse_error = false;
    operator bool() { return !has_parse_error; }
    // (...all the other methods...)
};

MyVisitor visitor;
if (!msgpack::unpack_visit(data, length, offset, visitor)) { ... }
// or like this:
if (msgpack::unpack_visit(data, length, offset, visitor).has_parse_error) { ... }

Together with the single error() function, I could see this being workable. However, it appears like this would require more abstract thinking from the caller and will neither save code in the visitor nor in the unpack_visit() implementation. So I am not convinced that this is better than the simpler bool return value.

Regarding parsed_offset and error_offset, let me get back to you on that after having another look. As described in the initial comment, I mostly included them because they mirror the parsed_size() and nonparsed_size()/message_size() values that unpacker provides, and those seemed useful for producing error output. I feel that offset is clearer than size in this respect, especially error_offset seems clearer than nonparsed_size() or message_size() in this context.

I'll have to check what the actual values for parsed_size() and message_size() are right now, but they should be the same as the values passed as parsed_offset and error_offset, otherwise it gets even more confusing.

[redboltz]

This would solve 1. from above, but 2. is still questionable, and the user still has to define a boolean such as has_parse_error inside the visitor. Given that the unpack() overload (I just saw that you named it unpack_visit()) already has all the information and only needs to return unpack_visit_imp() == UNPACK_SUCCESS;, it seems like that's a low price to pay for some extra convenience.

Ah yes I named unpack_visit() because there are many overloads. It contains not only v2::unpack() but also v1::unpack() dut to ADL. I can control those overloads using enable_if but introducing the new name unpack_visit() is simpler.

I will implement the bool return type version. You mentioned unpack_visit_imp() == UNPACK_SUCCESS;, I think that UNPACK_SUCCESS is mapped to true too. It is obvious. How about UNPACK_EXTRA_BYTES? It means data contains a complete msgpack formatted data and extra bytes. I think that it shoud be mapped true too.

I'll have to check what the actual values for parsed_size() and message_size() are right now, but they should be the same as the values passed as parsed_offset and error_offset, otherwise it gets even more confusing.

FYI: I removed a m_pased updating code by mistake on my branch and just fixed it.

Using a visitor with the unpacker, parsed_offset and error_offset can be mapped to parsed_size() and message_size(). But using a visitor with unpack_visit(), there is no concept corresponding to parsed_size() and message_size(). Because unpack() functions such as unpack_visit() unpack one complete message pack formatted data in one go. So I think that they both always zero. Any ideas?

[redboltz]

Using a visitor with the unpacker, parsed_offset and error_offset can be mapped to parsed_size() and message_size(). But using a visitor with unpack_visit(), there is no concept corresponding to parsed_size() and message_size(). Because unpack() functions such as unpack_visit() unpack one complete message pack formatted data in one go. So I think that they both always zero. Any ideas?

I realized that it's wrong. I can get the value corresponding to parsed_size() on unpack() function and that's not always zero. I will implement it.

I will consider error_offset and message_size() again.