Basic REST client

Jun 26, 2015 at 8:05 AM
Edited Jun 26, 2015 at 8:15 AM
I've been working on basic REST client; which simply fires GET/POST/PUT/DELETE call. At present following code is working fine but I'm facing challenges while achieving following things:
  1. Return the response body once the call is completed.
  2. Raise/throw the exceptions by looking at http error codes.
  3. Logging the request url, body and response.
  4. When should one use get() over wait() I do get that latter one is async; but not sure how that matters
Also let me know if I might have to take care of other things or implement things in different manner.

Thanks,
IK

Here goes the code...

Constructor of the class.
RESTClient::CRESTClient::CRESTClient(wstring basicUri, bool insecure, int timeout)
:m_httpClient(basicUri, _prepare_client_config(insecure, timeout))
{
}
Simply prepares the http_client_config object and returns the same, should this be implemented in different manner?
http_client_config RESTClient::CRESTClient::_prepare_client_config(bool insecure, int timeout)
{
    http_client_config httpClientConfig;
    httpClientConfig.set_validate_certificates(insecure);
    httpClientConfig.set_timeout(utility::seconds(timeout));
    return httpClientConfig;
}
This prepares the request body for authentication. Need to find a way to get token from response here. :(
void RESTClient::CRESTClient::authenticate(wstring username, wstring password, wstring tenant_name)
{
    m_szSessionToken = L"";
    value jAuth, jCred, jRequestBody, jResponseBody;
    jCred[ReqUsername] = value::string(username);
    jCred[ReqPassword] = value::string(password);
    jAuth[ReqCredentials] = jCred;
    jAuth[ReqTenantName] = value::string(tenant_name);
    jRequestBody[ReqAuth] = jAuth;
    Post(LogginSession, jRequestBody, jResponseBody);
    m_szUsername = username;
    m_szPassword = password;
    m_szTenantName = tenant_name;
}
The method which actually fires the request. Any scope of improvement?
task<void> RESTClient::CRESTClient::_request(wstring szUri, method httpMethod, value jRequestBody)
{
    return pplx::create_task([this, szUri, httpMethod, jRequestBody]
    {
        http_request request(httpMethod);

        request.set_request_uri(szUri);

        request.headers().add(header_names::content_type, ContentType);

        if (!m_szSessionToken.empty())
            request.headers().add(AuthToken, m_szSessionToken);

        if (jRequestBody != JSON_NULL)
            request.set_body(jRequestBody.to_string());

        return this->m_httpClient.request(request);
    }).then([this](http_response response)
    {
        if (response.status_code() >= status_codes::BadRequest)
        {
            // TODO: Handle different http failures
            return pplx::task_from_exception<value>(exception("Request failed!"));
        }
        return response.extract_json();
    }).then([=](pplx::task<value> previousTask)
    {
        try
        {
            previousTask.get();
        }
        catch (exception &e)
        {
            //return pplx::task_from_exception<void>(exception(e.what()));
            //std::cout << e.what();
            throw exception(e.what());
        }
        catch (...)
        {
            //return pplx::task_from_exception<void>(exception("Something failed!"));
            //std::cout << "Something failed!";
            throw exception("Something failed!");
        }
    });
}
Wraps the _request method; idea is to implement re-authenticate if and when token expires. Should it be implemented in different manner?
value RESTClient::CRESTClient::request(wstring szUri, method httpMethod, value jRequestBody)
{
    _request(szUri, httpMethod, jRequestBody).wait();
    // Need to get the JSON object from response
    return json::value();
}
Providing different actions. Is there a better way to implement this?
void RESTClient::CRESTClient::Get(wstring szUri, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::GET);
}

void RESTClient::CRESTClient::Post(wstring szUri, value jRequestBody, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::POST, jRequestBody);
}

void RESTClient::CRESTClient::Post(wstring szUri, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::POST);
}

void RESTClient::CRESTClient::Put(wstring szUri, value jRequestBody, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::PUT, jRequestBody);
}

void RESTClient::CRESTClient::Put(wstring szUri, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::PUT);
}

void RESTClient::CRESTClient::Delete(wstring szUri, value& jResponseBody)
{
    jResponseBody = request(szUri, http::methods::DEL);
}
Jun 26, 2015 at 7:40 PM
Hi ikhere,

You have a bit too many questions here so it is confusing to figure out exactly what you are looking for help on. Here are some comments:
  1. Inside CRESTClient::_request in your last task continuation just return previousTask.get(), it will return the json::value from extract_json above. Also change the function signature to return a task<json::value> instead of task<void>.
  2. You already are doing this right now, although you don't even need to create a task from the exception you could just directly throw the exception and allow it to escape out of the current task continuation.
  3. Leaving this up to you, you can have logging functions or print like you have commented out.
  4. All tasks are asynchronous until you go to actually wait or retrieve the value. task::wait() does what the name indicates it simply waits for the task to complete. task::get() waits for that task to complete and returned the result value from the task.
CRESTClient::CRESTClient
  • I find it confusing with the bool insecure variable. The way it is worded I expect 'true' to mean turn off certificate verification. However our API set_validate_certificates uses true to 'turn' on and 'false' to turn off.
CRESTClient::_request
  • No need to call pplx::create_task in the beginning the http_client::request(...) API is asynchronous and returns without performing any blocking operations. By calling create_task here you are creating an extra task that gets scheduled for no reason.
  • When checking the status code I would be careful about just doing greater than BadRequest. You are treating a lot of other status codes as success.
  • In your catch handlers why are you creating new exception types to throw? Just do a throw; to rethrow the original exception instead of losing the type and a bunch of information.
Not entirely sure what you want to do with the CRESTClient::GET, PUT, etc... functions. Do you want them to be asynchronous? If so they should return a task<json::value>.

General:
  • You are passing around many arguments by value incurring additional unnecessary copies of objects that aren't necessarily cheap to copy, strings, json::value objects, etc.. Most of these appear that they should be passed by const reference.
Steve
Jun 27, 2015 at 9:58 AM
Edited Jun 29, 2015 at 4:46 AM
Hi Steve,

Thanks for the reply. I considered the comments and the change is as follows.
  1. _request(...) method now doesn't create new task; instead it just relies on http_client::request(...).
  2. Status code is taken care; though need to figure out way to extract error message on failure.
  3. _request(...) now returns task<value> instead of task<void>and throws exception that was caught.
task<value> RESTClient::CRESTClient::_request(const wstring &request_uri, const method &http_method, const value &request_body)
{
    http_request request_obj(http_method);

    request_obj.set_request_uri(request_uri);

    request_obj.headers().add(header_names::content_type, ContentType);

    if (!m_szSessionToken.empty())
        request_obj.headers().add(AuthToken, m_szSessionToken);

    if (!request_body.is_null())
        request_obj.set_body(request_body.to_string());

    return m_httpClient.request(request_obj).then([this](http_response response_obj)
    {
        if (response_obj.status_code() >= status_codes::OK && response_obj.status_code() <= status_codes::Accepted)
        {
            return response_obj.extract_json();
        }

        // TODO: Handle different http failures
        throw exception("Request failed!");
    }).then([=](pplx::task<value> previous_task)
    {
        try
        {
            return previous_task.get();
        }
        catch (exception &e)
        {
            std::cout << e.what();
            throw;
        }
        catch (...)
        {
            std::cout << "Something failed!";
            throw;
        }
    });
}
task<value> RESTClient::CRESTClient::request(const wstring &request_uri, const method &http_method, const value &request_body)
{
    return _request(request_uri, http_method, request_body);
}
CRESTClient::Get, Put et all; is meant to take care of request and simply return the response so that upper stack need not have to worry about the logic to fetch the response and all.
void RESTClient::CRESTClient::Get(const wstring &uri, value &response_body)
{
    response_body = request(uri, http::methods::GET).get();
}
Have following queries now:
  • Exception handling isn't working as expected. I'm hitting following error in case request fails. I think I'm missing something basic here.
Unhandled exception at 0x000007FEFD91A49D in RESTClientTest.exe: Microsoft C++ exception: std::exception at memory location 0x00000000002AF5E0.
  • What if I want to fetch the response body while using wait(); as you have noticed that now I'm using get() to fetch response body.
Jun 29, 2015 at 9:40 PM
Hi ikhere,

Your code as it stands re throws the exception out of the task returned from the _request method. You then later on in CRESTClient::Get are calling task::get() which will throw the escaped exception from your task.

I recommend you take a look at the following for more information about programming with tasks, in particular how exceptions are managed and handled.
https://msdn.microsoft.com/en-us/library/dd492427(v=vs.110).aspx
wait()/get() are exactly the same, they both block waiting until the task has been completed. The only difference is one returns the resulting value from the task. The other doesn't.

Steve
Marked as answer by ikhere on 6/30/2015 at 12:12 AM
Jun 30, 2015 at 8:14 AM
Thanks for the help Steve! Did manage to get things in working state.