Refactor webserver handlers

[leszko]

What does this pull request do? Explain your changes. (required)
Move all endpoint handlers from webserver.go to handlers.go and create unit tests for them.

The endpoints should work exactly the same as before with the following exceptions:

• Sometimes different HTTP codes are returned (before we often returned 200 even if there was an error)
• Fail fast if ETH client or DB is missing
• If we want to return an empty object as JSON, now the proper null is returned (before it was either empty string or {})
• Error message in the response body (made it more consistent)

Non-goals for this PR:

• Improve our REST API
• Improve messages on the Livepeer CLI side

Specific updates (required)

• Moved all handlers to handlers.go
• Reuse common code mustHaveClient(), mustHaveDb(), etc.
• Add unit tests for almost handlers

How did you test each of these updates (required)

Played round with CLI to see if the commands work fine.

Does this pull request close any open issues?

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[yondonfu]

Looks good overall (note: I mainly skimmed since most of the code changes for the handlers were copy + pasting existing logic into dedicated handler functions). Just a few :small questions.

If we want to return an empty object as JSON, now the proper null is returned

Could this cause problems for any of the livepeer_cli functions that work with these objects if the return value is "null" instead of an empty string or empty object?

[yondonfu]

Should we keep this test case?

[leszko]

Do you mean the whole cliserver_test.go, right? It's kind-of integration/e2e test for the same what we test in handlers_test.go. I see some value in it, because if we, e.g., break something with the endpoints themselves, or in the REST server configuration of Livepeer, we would catch it here.

[yondonfu]

I specifically mean the code here under // test offchain which appears to be a test case for when the /EthChainID endpoint should return 0. I see that we kept the test case starting on L228 for when the /EthChainID endpoint returns 1. So, I am wondering if we should have both?

[leszko]

ahh, I see you point. No, I think we don't need it anymore. This test was actually in some sense wrong before. Because it tested the following scenario:

If DB is nil, then return 0 from /EthChainID.

But it's not a valid scenario, we never have a situation when DB is nil, we always create DB, even for the off-chain mode. So, in result // test offchain is exactly the same as // test onchain in this test case. That's why I removed it and consolidated it into one.

And the root cause why I actually removed it was that now I check in the handler if we have the DB object, so this test started to fail.

[yondonfu]

Got it thanks for the clarification!

[leszko]

If we want to return an empty object as JSON, now the proper null is returned

Could this cause problems for any of the livepeer_cli functions that work with these objects if the return value is "null" instead of an empty string or empty object?

No, it's not a problem, because we should (and we actually do) json.Umarshal() in livepeer_cli. Then, null is a correct value to unmarshal a json.

[leszko]

Thanks for the review @yondonfu Addressed both of your comments. PTAL

[yondonfu]

LGTM after resolving conflicts