Inconsistent response from JSON-RPC GET method

[inglkruiz]

Inconsistent response from JSON-RPC GET method

Description

While testing the JSON-RPC endpoints I decided to run

curl -X GET -H 'content-type: application/json' http://YOUR_NODE_URL

I was expecting a JSON response in a middleware I built but what I got was a plain text (not parseable to a JSON object), I went directly to repo's code and found this > https://github.com/0xPolygon/polygon-edge/blob/develop/jsonrpc/jsonrpc.go#L248-L252

This is not a big Issue but I'd suggest standardizing the response, and document it here > https://docs.polygon.technology/docs/edge/get-started/json-rpc-commands

Your environment

• OS and version: Kubernetes GCP/EKS
• version of the Polygon Edge: v0.4.1
• branch that causes this issue: develop

Steps to reproduce

• Create a network with 1 Validator Node.
• From a terminal run

curl -X GET -H 'content-type: application/json' http://YOUR_NODE_URL

You will see the response is a String that is not parseable to a JSON Object.

Expected behaviour

If the Content-Type is set to application/json > https://github.com/0xPolygon/polygon-edge/blob/develop/jsonrpc/jsonrpc.go#L237 I would expect a JSON Object.

Actual behaviour

The response is just a plain text.

Logs

NA

Proposed solution

I'd suggest standardizing the response, and document it here > https://docs.polygon.technology/docs/edge/get-started/json-rpc-commands

[inglkruiz]

Just to enrich the conversation. I think you could consider not handling HTTP GET method because in fact JSON-RPC expects a data object, meaning that it must be an HTTP POST.

Here's the spec https://www.jsonrpc.org/specification just in case you want to review it, but I'm sure you know it very well.

[zivkovicmilos]

Hey @inglkruiz,

Thank you for opening up the issue.

Would you find it useful if the GET request to the root JSON-RPC url returns a JSON response that includes the name of your blockchain network (the one you set in the genesis.json)?

[inglkruiz]

Hey @inglkruiz,

Thank you for opening up the issue.

Would you find it useful if the GET request to the root JSON-RPC url returns a JSON response that includes the name of your blockchain network (the one you set in the genesis.json)?

Hi @zivkovicmilos

Yes, I would. Possibly, other details might come handy, like the node's version. TBH I'm fine with anything as long as the response is a parseable JSON Object because the Header Content-Type is set to application/json.

[zivkovicmilos]

Got it @inglkruiz, thank you for the quick reply.

I've talked with the team and decided that this makes sense - we've added it to the development pipeline.

Letting you know here as soon as we have it up on a PR 🙏

[mrwillis]

GET / is a very useful standard endpoint for health checks for load balances and downstream services. Is it really necessary to remove it?

[zivkovicmilos]

Hey @mrwillis,

Not necessary at all to remove it. Seeing as we can't reach a conclusion on the PR, I will keep the GET endpoint, but make sure it returns a proper JSON response, instead of a string. This option requires some code gymnastics, but @ZeljkoBenovic also expressed concerns on removing it prematurely so it's worth looking into it

[inglkruiz]

@mrwillis I would not say that GET / is a standard endpoint for health checks. As a node operator in the K8s space, I've found multiple services implement /healthz as the health check endpoint and mostly it goes on a different port.

As a reference, you could check Hyperledger Fabric common library and here the healthz module > https://github.com/hyperledger/fabric-lib-go/tree/main/healthz Also, here are the docs > https://github.com/hyperledger/fabric/blob/v2.4.4/docs/source/operations_service.rst#health-checks and for getting some inspiration on how they use it you can check Fabric's codebase > https://github.com/hyperledger/fabric/