Skip to content

[REQ] [cpprestsdk] ApiException is not used in the way it is generated #3462

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

Open
CyrilleBenard opened this issue Jul 25, 2019 · 3 comments
Open

Comments

@CyrilleBenard
Copy link

CyrilleBenard commented Jul 25, 2019

Is your feature request related to a problem? Please describe.

The generator produces code that uses ApiException not in the way it has been intended. Ok, this is not exactly true but let me show what I mean thanks to the PetApi example :

In the file PetApi.cpp line 136 we can see one error treatment throwing an ApiException

if (localVarResponse.status_code() >= 400)
        {
            throw ApiException(localVarResponse.status_code()
                , utility::conversions::to_string_t("error calling addPet: ") + localVarResponse.reason_phrase()
                , std::make_shared<std::stringstream>(localVarResponse.extract_utf8string(true).get()));
        }

The 3rd parameter of the ApiException is set with a std::make_shared<std::stringstream> whereas the ApiException shows the corresponding API with a std::shared_ptr<std::istream>

class  ApiException
    : public web::http::http_exception
{
public:
    ApiException( int errorCode
        , const utility::string_t& message
        , std::shared_ptr<std::istream> content = nullptr );
../..
}

Of course this is allowed because a std::stringstream IS A std::istream but when I (the developer) want to handle an ApiException in my application, I retrieve "'only" an std::istream that is not really convenient to use whereas at the origin, the ApiException has been raised with a std::stringstream.

Describe the solution you'd like

What about modifying the ApiException and replace the use of std::istream with std::stringstream ?

Describe alternatives you've considered

N/A

Additional context

N/A

@etherealjoy
Copy link
Contributor

etherealjoy commented Jul 26, 2019

@CyrilleBenard
This is by design like this, so that different exceptions can be thrown and the handler can process the derived object properly.

In your case please do something like this

    auto reqTask = api->findPetsByStatus(req)
            .then([=](std::vector<std::shared_ptr<Pet>> pets)
            {
                std::cout << "found pet successfully" << std::endl;
            });
    try{
        reqTask.wait();
    }
    catch(const ApiException& ex){
        std::cout << ex.what() << std::endl << std::flush;
        auto ss = std::dynamic_pointer_cast<std::stringstream>( ex.getContent() );
        if( nullptr != ss )
        {
            std::cout << ss->str() << std::endl << std::flush;
        }
        std::string err(ex.what());
    }

perform std::dynamic_pointer_cast and you will get back the std::shared_ptr<std::stringstream>

In this case I can see the error like this for example

error calling findPetsByStatus: Not Found
{"code":404,"type":"unknown","message":"java.lang.NumberFormatException: 
For input string: \"findByStatus\""}

@etherealjoy
Copy link
Contributor

@CyrilleBenard
Please update when you have time and if this hint resolve your issue.

@CyrilleBenard
Copy link
Author

CyrilleBenard commented Jul 27, 2019

@etherealjoy Thanks for your answer and proposal but I did not opened a Bug issue "only" a Feature request :)
I also write this kind of treatment in my code and it worked well but what I pointed is the restrictive use of istream instead of using stringstream along all the Exception treatment.

Throw ApiException using stringstream and catch it using stringstream. No restrictive type inside ApiException class and so, no cast to transform the type in the catch treatment and best performance also.

We are also able to write code without a cast and so without any check of nullptr like :

auto reqTask = api->findPetsByStatus(req)
            .then([=](std::vector<std::shared_ptr<Pet>> pets)
            {
                std::cout << "found pet successfully" << std::endl;
            });
    try{
        reqTask.wait();
    }
    catch(const ApiException& ex){
        std::cout << ex.what() << std::endl ;
        std::stringbuf sbuf ;
        e.getContent()->get(sbuf) ;

        std::cout << sbuf.str() << std::endl ;

    }

But I still think that it's not as simple as if we only use stringstream.
This was just a Feature Request to optionally improve the generated code and the developer's life 😄

NOTE:
I saw you use std::flush behind std::endl but if I'm not wrong, std::endl already flush the stream (source).

greetings

EDIT: fix the code alternative I wrote previously

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

No branches or pull requests

2 participants