[REQ][Qt5] Make response objects queueable

[luStopai]

Is your feature request related to a problem?

No, it's an improvement

Please describe.

I think it would be great if we could run requests in a QThread, but right now it's not possible because the returned objects cannot be queueable (signals&slots) from one thread to a different thread.
This is in order to be able to run requests in background without freezing the UI.

Describe the solution you'd like

I think if the response objects where pointers to qobjects instead of regular objects it should work. Notice that they should be registered with qRegisterMetaType() ( https://doc.qt.io/qt-5/qt.html#ConnectionType-enum )

Describe alternatives you've considered

Right now the alternative is to manually create a proxy class that makes use of the generated client to make the request, receive the response and then convert the response objects to some pointers to custom qobjects, which are queueable. So the purpose of this suggestion is to avoid this workaround

Additional context

[etherealjoy]

Please do this this

background thread creates the api and calls

OAIPetApi* api
api->findPetsByStatus(status); // Non blocking call

UI thread --> slot is present in object on the receiver thread.

connect(api, &OAIPetApi::findPetsByStatusSignal, this, validator, Qt::QueuedConnection);

Qt::QueuedConnection instead of default Qt::AutoConnection

[luStopai]

This doesn't work.
When connecting two objects living in different threads, Qt automatically uses Qt::QueuedConnection so forcing it doesn't make a change.

The output error is the same
QObject::connect: Cannot queue arguments of type 'QList<OAIPet>'

This is because the autogenerated class OAIPet can't be used in a Qt::QueuedConnection because it doesn't fit the requirements stated here https://doc.qt.io/qt-5/qt.html#ConnectionType-enum

[etherealjoy]

Yes this is needed

qRegisterMetaType<OAIPet>( "OAIPet" );

Registering this is easy.
I wonder if we need to register all the types returned by the signals even though QList and QMap are already registered by default.

[luStopai]

Sorry, but even after registering the class it still doesn't work, I must be doing something wrong if it's working for you.

By the way, I'm using openapi-generator-cli 4.0.0 to generate the code if it makes any difference.

This is the code I'm trying, using this docker https://hub.docker.com/r/openapitools/openapi-petstore as a server. I skipped the code related to QThread since it's enought to make it work with Qt::QueuedConnection

TestCaller::TestCaller(QObject *parent) : QObject(parent)
{

}

void TestCaller::threadCall()
{
    qRegisterMetaType<OpenAPI::OAIPet>( "OAIPet" );

    OpenAPI::OAIPetApi* api = new OpenAPI::OAIPetApi("http://localhost/", "v3");

    api->defaultHeaders.insert("Authorization", "Bearer f41febfa-f591-4e34-bfc3-eee5c83a6b97");

//This doesn't work
    connect(api,
            &OpenAPI::OAIPetApi::findPetsByStatusSignal,
            this,
            &TestCaller::loaded,
            //Qt::AutoConnection --> THIS WORKS
            Qt::QueuedConnection //THIS DOESN't WORK
            );

//This works
//    connect(api,
//            &OpenAPI::OAIPetApi::findPetsByStatusSignal,
//            this,
//            &ThreadCaller::loaded,
//            Qt::AutoConnection
//            );


    QList<QString> status;
    status << "available";
    api->findPetsByStatus(status);
}

void TestCaller::loaded(QList<OpenAPI::OAIPet> list)
{
    qDebug() << "It Worked!";
}

[etherealjoy]

I will try it out and let you know

[etherealjoy]

QDEBUG : PetApiTests::findPetsByStatusTest() Creating request in  7012
QDEBUG : PetApiTests::findPetsByStatusTest() Connect in  7017
QDEBUG : PetApiTests::findPetsByStatusTest() Got Callback 7012

Seems like it does not work for me. The callback arrives in the thread where the API was created and where the event loop is running.

[luStopai]

I just noticed we were missing something else:
With the following code, the request runs in the main thread and the callback runs in the thread, because methods always run in the calling thread.

void TestCaller::threadCall()
{
    qRegisterMetaType<OpenAPI::OAIPet>( "OAIPet" );

    QThread* t = new QThread();
    OpenAPI::OAIPetApi* api = new OpenAPI::OAIPetApi("http://localhost/", "v3");
    api->moveToThread(t);
    t->start();

    api->defaultHeaders.insert("Authorization", "Bearer c3268a60-be3e-43f0-8f58-17fcd3533d51");

    connect(api,
            &OpenAPI::OAIPetApi::findPetsByStatusSignal,
            this,
            &TestCaller::loaded
            );

    connect(api, &OpenAPI::OAIPetApi::findPetsByStatusSignal, t, &QThread::quit);
    connect(t, &QThread::finished, t, &QThread::deleteLater);
    connect(t, &QThread::finished, api, &OpenAPI::OAIPetApi::deleteLater);


    QList<QString> status;
    status << "available";
    api->findPetsByStatus(status);
}

To make the request also run in the thread, findPetsByStatus should be a slot and not a regular method. This way we can connect a signal to this method as shown in the following snippet.

Notice that registering QList< QString > is needed. Edit: it's not need if Qt::QueuedConnection is forced

void TestCaller::threadCall()
{
    qRegisterMetaType<OpenAPI::OAIPet>( "OAIPet" );


    QThread* t = new QThread();
    OpenAPI::OAIPetApi* api = new OpenAPI::OAIPetApi("http://localhost/", "v3");
    api->moveToThread(t);
    t->start();

    api->defaultHeaders.insert("Authorization", "Bearer c3268a60-be3e-43f0-8f58-17fcd3533d51");

    connect(api,
            &OpenAPI::OAIPetApi::findPetsByStatusSignal,
            this,
            &TestCaller::loaded
            );

    connect(api, &OpenAPI::OAIPetApi::findPetsByStatusSignal, t, &QThread::quit);
    connect(t, &QThread::finished, t, &QThread::deleteLater);
    connect(t, &QThread::finished, api, &OpenAPI::OAIPetApi::deleteLater);

    QList<QString> status;
    status << "available";

    qRegisterMetaType<QList<QString>>( "A_List_Of_Strings" ); //Without it an error ocurrs: "Make sure 'QList<String>' is registered using qRegisterMetaType()"

    connect(this, &TestCaller::myFindPetsByStatusSignal, api, &OpenAPI::OAIPetApi::findPetsByStatus);
    emit myFindPetsByStatusSignal(status);
}

Either way, the callback method cannot queue OAIPet back to the main thread. The same would happen the other way around if we wanted to make addPet to run in the thread.

[etherealjoy]

Notice that registering QList< QString > is needed.

Yes this is needed.

Either way, the callback method cannot queue OAIPet back to the main thread. The same would happen the other way around if we wanted to make addPet to run in the thread.

Indeed, how the objects are moved around however is outside this api client. so I am not sure how to proceed.

[luStopai]

Ok, I'll try to explain:

To be able to make requests in a thread and get the response back to the main thread, we need to use the signals&slots mechanism with queued connections.
So to be able to achieve this without using the api inside a custom auxiliary class, the signals and slots of the api itself should be queueable. Which means the parameters of the signals and slots of the api should be registered as metatypes, and this implies they have to fit the metatype's requirements (which I think they are: public default constructor, a public copy constructor and a public destructor)

So to sum up, I think there are 3 things to be done:

1. Convert the public methods of the api classes to public slots, to be able to connect signals to them from a different thread.
2. Change the definition of the generated objects so the can be register as metatypes
3. Register the classes as metatypes in the generated code so it doesn't have to be done externally. Maybe by adding the Q_DECLARE_METATYPE macro in the class itself, so there's no need to call qRegisterMetaType() explicitly.

I'm not sure if it's clear what I'm trying to achieve. Please ask if needed

[etherealjoy]

It is clear what you are trying to do, I am just wondering how, without breaking the current compatibility for current users.
Need some thinking

[luStopai]

Ok great, I wasn't sure it was clear.

Also I've made some progress.
Registering OAIPet using qRegisterMetaType didn't work for me but using Q_DECLARE_METATYPE did.

I only needed to add Q_DECLARE_METATYPE(OpenAPI::OAIPet) to OAIPet header, and force the connection to Qt::QueuedConnection as you mentioned earlier.
There's no need to make the methods public slots because since new Qt signal and slots syntax "It is possible to connect to any member function of QObject, not only slots."

So maybe the only change needed is to register all generated clases as metatypes by adding Q_DECLARE_METATYPE at the bottom of the header, and this doesn't seem to have any side effects that could affect backwards compatibility.

Hope it helps

[luStopai]

Just noticed that error signals contain a parameter
QString &error_str
which doesn't work for queued connections

Changing to either
const QString &error_str
or
QString error_str
worked for me

[etherealjoy]

I think the parameters in the signals should not be reference anyway. because they are created on stack

[etherealjoy]

I created the PR. I think the default copy constructor is good enough.

[luStopai]

Great, Thank you!