cli: cleanup logic for HTTP error codes #18323

pull niftynei wants to merge 1 commits into bitcoin:master from niftynei:nifty/http-nit changing 1 files +3 −1

niftynei commented at 10:21 PM on March 11, 2020: contributor
bit of a nit. removes redundant logic check and make more readable.

summary of changes:
```
old: status >= 400 && status != 400
new: status > 400
```
fanquake added the label Utils/log/libs on Mar 11, 2020
fanquake renamed this:
~~http nit~~
cli: cleanup logic for HTTP error codes
on Mar 11, 2020

fanquake commented at 10:53 PM on March 11, 2020: member

Can you fixup the linter issue:

This diff appears to have added new lines with tab characters instead of spaces.
The following changes were suspected:
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
@@ -399 +399,3 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
+		    && response.status != HTTP_NOT_FOUND
+		    && response.status != HTTP_INTERNAL_SERVER_ERROR)
^---- failure generated from test/lint/lint-whitespace.sh

cli: cleanup logic for HTTP error codes
remove redundant check and make more readable
0798949584
niftynei force-pushed on Mar 11, 2020
niftynei commented at 11:35 PM on March 11, 2020: contributor

linter is now passing.
practicalswift commented at 1:03 AM on March 12, 2020: contributor

Welcome as a Bitcoin Core contributor @niftynei. Thanks for all the great c-lightning contributions you've done!

in src/bitcoin-cli.cpp:400 in 0798949584

 395 | @@ -396,7 +396,9 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 396 |          } else {
 397 |              throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
 398 |          }
 399 | -    } else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR)
 400 | +    } else if (response.status > HTTP_BAD_REQUEST // HTTP_BAD_REQUEST is 400
 401 | +                    && response.status != HTTP_NOT_FOUND

jonatack commented at 1:48 AM on March 12, 2020:

nit: 5 spaces of extra indentation?

niftynei commented at 3:49 AM on March 12, 2020:

are wrapped if's usually lined up? this seems to odd to me but i'm not familiar with bitcoind's formatting conventions.

jonatack commented at 4:01 AM on March 12, 2020:

the linter didn't complain, so there's that, in theory it's https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

jonatack commented at 2:02 AM on March 12, 2020: member

ACK on hoisting the magic value to a constant. The original logic began by checking that there is an HTTP error (either Client 4xx or Server 5xx), and then enumerating the 3 exceptions; I suppose this was done to separate the general error class check from the HTTP_BAD_REQUEST check. Maybe good to keep this separation?
niftynei commented at 3:51 AM on March 12, 2020: contributor

I suppose this was done to separate the general error class check from the HTTP_BAD_REQUEST check. Maybe good to keep this separation?

This is why I added the comment. If we keep the constant but revert the logic, I don't think that's going to make what it's doing any clearer.
niftynei commented at 3:56 AM on March 12, 2020: contributor

the CI job failure appears to be spurious

https://travis-ci.org/github/bitcoin/bitcoin/jobs/661311499
jonatack commented at 4:07 AM on March 12, 2020: member
I surmise the intent was if status >= HTTP_ERROR && (specific exceptions we want to handle differently) so if the exceptions change it seems good to keep the general check separate along the lines of:
```
    } else if (response.status >= HTTP_ERROR &&
               (response.status != HTTP_BAD_REQUEST &&
                response.status != HTTP_NOT_FOUND &&
                response.status != HTTP_INTERNAL_SERVER_ERROR)) {
```
but that's only one opinion :)
fanquake requested review from laanwj on Mar 13, 2020
promag commented at 11:11 PM on March 15, 2020: member

The change is correct but FWIW I prefer the current one.
Empact commented at 11:33 PM on March 18, 2020: member

I also prefer the current code - if 400 is "the value that identifies all errors", then what follows are exceptions to the general error condition.
laanwj commented at 3:05 PM on March 19, 2020: member

The change is correct but FWIW I prefer the current one.

I also prefer the current code - if 400 is "the value that identifies all errors", then what follows are exceptions to the general error condition.

Same. I like that this splits the condition over multiple lines, but I don't like the >= to > change as it makes things less explicit. There's no need for this kind of manual logic optimization, wouldn't be surprised if the compiled code is exactly the same.

I also don't think replacing the 400 with a constant is good. ">= 400" is used as shortcut for "4xx or 5xx" here, which are the HTTP error codes that are fatal.
laanwj commented at 5:49 PM on March 26, 2020: member

I don't think there's enough agreement to make this change. Closing, sorry.
laanwj closed this on Mar 26, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Review Requested
laanwj

Labels

Utils/log/libs