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

On GUI side, handle Core REST API responses without data #7334

Merged

Conversation

kozlovsky
Copy link
Contributor

@kozlovsky kozlovsky commented Mar 24, 2023

This PR fixes #7333:

  1. It adds the handling of QNeworkReply errors mentioned in Empty results in reply.readAll() when receiving response from Core REST API #7335 by checking the QNetworkReply error code before reading the response data;
  2. If there is an error or if the response data is empty, now Request does not call the on_success callback (renamed from on_finish);
  3. In case of an error, it displays QNetworkReply errors in debug window as a request status code.

@kozlovsky kozlovsky force-pushed the fix/request_manager_key_error branch from ad13303 to a9cb07c Compare March 24, 2023 16:04
@kozlovsky kozlovsky force-pushed the fix/request_manager_key_error branch from a9cb07c to 6cda1bc Compare March 28, 2023 17:02
@kozlovsky kozlovsky marked this pull request as ready for review March 28, 2023 17:42
@kozlovsky kozlovsky requested review from a team and xoriole and removed request for a team March 28, 2023 17:42
@kozlovsky kozlovsky changed the title Fix/request manager key error On GUI side, handle Core REST API responses without data Mar 28, 2023
@kozlovsky kozlovsky requested a review from drew2a March 28, 2023 18:10
@kozlovsky kozlovsky force-pushed the fix/request_manager_key_error branch from 7ccb956 to 1ce0aeb Compare March 28, 2023 18:10
@kozlovsky kozlovsky force-pushed the fix/request_manager_key_error branch from 1ce0aeb to 2f2da8f Compare March 29, 2023 06:30
@kozlovsky kozlovsky requested a review from xoriole March 29, 2023 06:30
@kozlovsky kozlovsky merged commit c64313d into Tribler:release/7.13 Mar 29, 2023
@kozlovsky kozlovsky deleted the fix/request_manager_key_error branch March 29, 2023 08:14
@xoriole
Copy link
Contributor

xoriole commented Aug 9, 2023

Related to #7558

By checking the QNetworkReply error code before reading the response data, I noticed that it is masking the HTTP error response like 413 HTTPRequestEntityTooLarge because the error code according to QT is not zero in such case. It returns error code: 299 (UnknownContentError).

Therefore, in my opinion, it would be best to check for the HTTP status code first and process the response (including empty and error responses). If there is no HTTP status code present then the QT error code be processed.

@kozlovsky
Copy link
Contributor Author

kozlovsky commented Aug 9, 2023

@xoriole, in my opinion, we should check the QNetworkReply error code before the HTTP error code because it covers a wider range of possible errors, like QNetworkReply::ConnectionRefusedError or QNetworkReply::UnknownNetworkError. It does not look correct to check the HTTP code if no HTTP response was received.

But we should allow further processing of responses with QNetworkReply::UnknownContentError, as in this case, we successfully got the response from the server, and the error is content-related and most probably explained by the HTTP error code and JSON error message.

So I think we should change

if error_code != QNetworkReply.NoError:
    ...

to

if error_code not in (QNetworkReply.NoError, QNetworkReply.UnknownContentError):
    ...

and allow processing of responses with QNetworkReply.UnknownContentError.

xoriole added a commit to xoriole/tribler that referenced this pull request Aug 10, 2023
Previously, QNetworkReply errors were checked and the response was
processed afterwards. With this PR, it prioritises the HTTP status
code processing over QT network error codes.

See: Tribler#7334 (comment)
xoriole added a commit to xoriole/tribler that referenced this pull request Aug 14, 2023
Previously, QNetworkReply errors were checked and the response was
processed afterwards. With this PR, it prioritises the HTTP status
code processing over QT network error codes.

See: Tribler#7334 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants