microsoft / CCF

Confidential Consortium Framework
https://microsoft.github.io/CCF/
Apache License 2.0
777 stars 211 forks source link

Accept plain strings for string query parameters of C++ endpoints #2153

Closed letmaik closed 3 years ago

letmaik commented 3 years ago

Currently, it is /network/nodes?host="123.456..." because everything is JSON, but this is clearly undesirable and also conflicts with the generated OpenAPI schema.

achamayou commented 3 years ago

Yes, this aspect of the json wrapper is a (the last?) leftover from the jsonrpc days, it clearly needs removing now.

eddyashton commented 3 years ago

I took another look at this, convinced myself it was a trivial fix, and instantly hit the "oh no this doesn't work" wall.

My attempted fix was just "if parsing as JSON fails, keep the string". So in get_params_from_query,

catch (const std::exception& e)
{
  throw UrlQueryParseError(fmt::format(
    "Unable to parse URL query value: {} ({})", query, e.what()));
}

became

catch (const nlohmann::json::parse_error& e)
{
  params[std::string(key)] = value;
}

Easy peasy. But!

If you remove the quotes from ?host=1.2.3.4, then our attempt to parse it succeeds in the wrong way - it reads a JSON number, not a string!

So this 2-step "try and find a JSON type" now butts against the "I expected you to find the right JSON type for my conversion" later. I don't see an escape hatch here, and I think the answer is to drop any JSON magic conversion for query parameters. We can build a string->string map for you, but from there the parsing is up to the app.

achamayou commented 3 years ago

I think the answer is to drop any JSON magic conversion for query parameters. We can build a string->string map for you, but from there the parsing is up to the app.

I completely agree.