Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adds HTTP/1.1 support on cleos requests and improves response handling #9349

Merged
merged 3 commits into from
Aug 18, 2020

Conversation

leordev
Copy link
Contributor

@leordev leordev commented Jul 30, 2020

Change Description

  • Upgrades cleos http 1.0 to 1.1: in more advanced cloud stacks cleos is being rejected by gateways or proxies with HTTP status 426:

The HTTP 426 Upgrade Required client error response code indicates that the server refuses to perform the request using the current protocol but might be willing to do so after the client upgrades to a different protocol. The server sends an Upgrade header with this response to indicate the required protocol(s).

  • Removes unnecessary json parsing if it's an error code
  • Straight dump responses for --print-response if unable to convert to variant

Change Type

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

if( print_response ) {
std::cerr << "RESPONSE:" << std::endl
<< "---------------------" << std::endl
<< fc::json::to_pretty_string( response_result ) << std::endl
<< re << std::endl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the pretty print of the response? The point is to make this human readable. Do you need to parse this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this does remove the forced pretty-printing of the json and instead relies on the service endpoints choice of formatting which may be very efficient and less readable as a result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason was because we don't know if it was a successful response so far and we might just dump the response if it's a useful plain text error...
For example, for the PR root problem the status code 426 does not give any json... I imagine there are other handful errors that could either not be a json and have a plain text or have no response at all.

Do you think we should do some checks and prettify if it's a valid json?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that fc::json::to_pretty_string will print invalid json just fine. Did you find something where information was lost?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was the call to fc::json::from_string can throw an exception and prevent the response from being printed out.

One option is to still try the from_string anyway, catch any exception, and in the case of exception print a non-pretty response, otherwise it can make response_result pretty and print that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leordev Can you update this PR as @arhag suggests.

Copy link
Contributor

@heifner heifner Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leordev I went ahead and made @arhag suggested changes. @arhag can you review now please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thank you @heifner 💯💯💯 -- I was going to go back here and do that as soon as I have a chance but I got stuck in other high priority items!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heifner Change looks fine to me.

Copy link
Contributor

@arhag arhag Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heifner

Hmm. Actually, under some status codes, this now could either return a null variant or it would throw an alternative exception as it tries to convert the null variant to some other type.

Do we want to rethrow the original exception after printing the response if it is null?

@leordev leordev requested review from heifner, b1bart and arhag August 12, 2020 18:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants