Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

any JSON object should be allowed; another ctor for response object #250

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

abhisharmab
Copy link

  1. It was not correct to allow only JSON object type to be serialized like that since other things are also valid JSON.

  2. Since you mention people can use rapidJSON you need a way for them to pass std::string but still set the header content-type to be JSON for client applications.

@abhisharmab
Copy link
Author

@ipkn - I think the missing documentation project really needs some attention. Let me know if you need help.

@ipkn
Copy link
Owner

ipkn commented Sep 23, 2017

It seems that current response code has a bug at handling json value. Some constructors save json_value only and some others serialize to the body right away. This problem should be handled first.

I think that the best way is remove json_obj from response class and add a static method named response_from_json_string. Add another constructor with bool argument seems error-prone to me.

And I also want to add a customizable `response serialization' class with enable_if. Then rapidJSON or nlohmann json can works well with response class. Code will be something like this:

// http_response.h

template <typename T, typename = std::enable_if<is_response_serializable<T>::value>::type>
response::response(const T& t) : body(crow::serialize_response_from(t)) {}

// from user code
namespace crow
{
  template <>
  struct serialize<UserType>
  {
    std::string operator()(const UserType&) const { ... }
  }
}

Any documentation help is always welcome. I'm still busy and deeply appreciate for any help.

@abhisharmab
Copy link
Author

@ipkn - I follow you argument here. Although I think it un-necessarily complicates things for the end-user of crow. The std::enable_if is not trivial for most people to follow. Why don't we just do this:

  • remove the json_value constructor all together and just have one constructor with std::string
  • then we can do response::response(std::string b) : body(std::move(b))
  • for setting content-type; just either make it a first-class argument itself or let user supply that explicitly. there are 6-7 other content types anyways.

Let me know what you think.

@ipkn
Copy link
Owner

ipkn commented Sep 26, 2017

The thought about having a constructor with json value was:

If we can construct the response from a json object, we can write a handler just with a json return value and without a response argument.

CROW_ROUTE(...)([]{
    return some_json_type{{"version", 3}, {"another-key", "another-value"}};
});

This looks good for writing a simple rest server.


But maybe giving headers as a constructor argument could be better. Many other web server implementations create a response obj from [status code, body, headers].

I will do some more experiments later and make suggestions again.

@yurenzhen
Copy link

CROW_ROUTE(app, "/api/mutil_json")
([]{
crow::json::wvalue x;

    x["Code"] = 1;
    x["Msg"] = "success";
    x["Data"]["Code"] = 1;
    x["Data"]["Remark"] = "very good";

    return x;
});

/*
will response like this:
{
"Code": 1,
"Data": {
"Remark": "very good",
"Code": 1
},
"Msg": "success"
}
*/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants