Generalizing the JSON traits further

[Sp3EdeR]

The JSON traits are not yet advanced enough to support the RapidJSON library. This is mainly because RapidJSON parses into- and serializes from document objects. document objects hold ownership to the JSON tree. JSON values however are stored as value objects, and the document itself acts as such; but it cannot be copied into value objects.

I haven't gotten deep enough into the integration yet, but I suspect that the traits usage may also produce overhead, which might be lessened by using reference semantics in places.

[Sp3EdeR]

I intend to implement this, once the EdDSA feature gets merged (in order to avoid having to complicate the branching and integration).

[prince-chrismc]

With RapidJSON the biggest challenge is determining value_type for the traits. it depends how much schema validation you will be using IMO

You can use:

• rapidjson::Value
• rapidjson::Document (which is a value)

static bool parse(rapidjson::Value &val, string_type str)
{
    rapidjson::Document doc;
    d.Parse(str.data());
    if(doc.HasParserError())
        return false;

    val = std::move(doc);
    return true;
}

static bool parse(rapidjson::Document &val, string_type str)
{
    val.Parse(str.data());
    return !val.HasParserError();
}

Personally I think the document will be the better approach.

[Sp3EdeR]

Value cannot move from a document, the library has a specific check for it. I'll need to introduce the document_type type to the traits for sure. I'll probably put it together in a day or two as my time allows for it.

I would also like to avoid dumping performance considerations out the window while doing so. :)

[prince-chrismc]

I know rapidJSON will be the least well suited to the traits because of it's API, but I think the majority of JSON libraries will be easy to integrate.

I really think using the doc for the value is the best option.

[prince-chrismc]

I found this fork, https://github.com/Zebreus/jwt-cpp, which is expending the traits for QtJson + non std::string types

[Sp3EdeR]

@prince-chrismc, I will need to add further traits. Ones that I've found so far are:

• Conversion from string to value (value_type from_string(const std_string&)
• Handling for the document

I would like to affect existing traits the least amount and don't want to introduce bloat in them. But I don't see a very good solution for that right now. Since you seem to be the owner of the json traits feature, I'd like your input before I do too much work... :) Other than requiring all additional traits from all the libraries, my first idea is:

1. I could add a json_traits_common<> class
2. The class takes each of the defined types as its template arguments
3. The additional functions are defined by the json_traits_common as trivial functions
4. The actual trait definition inherits from the new common class.

Alternatively, the type traits could be separated from the function traits for each library's definition, which would look as follows:

struct picojson_type_traits
{
    using value_type = ...
};
struct picojson_traits : json_traits_common<pico_type_traits>
{ };

[prince-chrismc]

• Conversion from string to value (value_type from_string(const std_string&)

There's already parse and serialize methods on the traits. I would be open to changing the signature for a return and throw if it fails. That should not prevent any implementations

I've use RapidJSON very heavily in production for several years now, I would be shocked that the traits need to be modified.

In ur fork, you could add RapidJSON to the repo and try implementing a test for it like the Nlohmann example I had done.
I'd love to help an attempt to make it work!

[Sp3EdeR]

Sorry for not being clear. I didn't mean the parse and serialize methods. The following check:

		template <typename traits_type, typename value_type, typename string_type>
		struct supports_as_string {
			static constexpr auto value =
				std::is_constructible<value_type, string_type>::value &&
				is_detected<as_string_function, traits_type>::value &&
				std::is_function<as_string_function<traits_type>>::value &&
				is_as_string_signature<traits_type, value_type, string_type>::value;
		};

States in the first check, that value_type must be constructible from a string_type. In the rapidjson case, there are "const char*", StringRef and (string, allocator) (behind an ifdef) constructors available only. I haven't yet checked all of the jwt-cpp uses cases for this conversion yet, but I suspect that the StringRef would be good; however that constructs explicitly from std::string in order to hint that the string instance is a literal (to avoid temporary-string memory errors). So this fails the above check certainly.

But there are further issues as well; for example the builder object uses object instances to store header and payload information. rapidjson does not have a default constructor for objects. I believe that a value must be constructed instead, passing kObjectType to it to wrap an actual object. So a trait may be required to specify how to construct an empty object.

rapidjson is a large library, I don't think it is a good idea to include it in jwt-cpp. I could probably add a switch to the cmake build to add an external rapidjson dependency and only turn on the tests when that's defined. But at first I wanted to get to the point where something feasible looking at least compiles before starting a full-on test.

[prince-chrismc]

I wouldnt want it merged, just for a test! but find_package is fine for me

You would need to enable randjson with std::string, the internals here are very dependent on that. I was not able to break that dependency

if you wanted the most efficient usage with rapidjson, it would be far to specific for the traits

The object problem is a very interesting use case, we could probably just have a thin wrapper around value that provides the default ctor which is required

[Sp3EdeR]

I've come to the conclusion that there's no point trying to integrate rapidjson.

Traits would have to be specialized far too much to support rapidjson properly and there is no nice way to default common trait behaviour.
Creating wrappers for the rapidjson stuff would essentially create a whole new library, which defeats the original purpose for me.

So I'm closing this issue.