Use strict json encoder in REST API v2 endpoints

[algorandskiy]

Summary

As #1699 demonstrates, api v2 uses JSONHandle that incorrectly encodes maps with numeric fields. Switching to JSONStrictHandle fixes the issue since it was specially created for this purpose.

Test Plan

A test added

[algorandskiy]

This is a single-line fix but with a quite large test.

[tsachiherman]

I'm not opposing to use strict json formatting, since the non-strict variation doesn't create a valid json on integer keyed map; but I am a bit concerned that this change would create other issues. Two potential issues -

1. Does this change affect only block requests ? If not, it might be an issue.
2. What about the msgptool -d ? It being used to decode blocks as well. Do we need to change it as well ? I.e. it make the decompress/recompress non canonical.

[brianolson]

Based on the difference in protocol/codec.go where the only difference is adding MapKeyAsString, I feel good about using this for API return values, which I'm pretty sure is the only time algod uses JSON (no internal storage of JSON like Indexer).

[winder]

Thanks for bringing this up again. This would only affect endpoints that have a format option. Since msgpack is available to fetch the canonical data, I think we can ignore any slight differences that this change may cause. These are the affected endpoints:
/v2/transactions/pending
/v2/transactions/pending/{txid}
/v2/blocks/{round}
/v2/accounts/{address}

The TealDryrun command protocol.JSONHandle for reading the request, do you think that should use the strict encoder?

[tsachiherman]

What would happen if we would follow the same pattern used in msgpacktool, and add ":int" to the key's string when encoding it as JSON ? this would allow the reverse operation to perform that symmetrically. ( similar to ":b64" )
i.e. I'm not afraid of "breaking" the JSON formatted output here, since it's already "broken" for that use case.

[algorandskiy]

Thank you for reviewing the PR.

What about the msgptool -d ? It being used to decode blocks as well. Do we need to change it as well?

It has -strict option to use JSONStrictHandle so it is all set.

The TealDryrun command protocol.JSONHandle for reading the request, do you think that should use the strict encoder?

It does not matter (added a test to protocol/codec_test.go) but I changed it to be in sync with rest of the code.

Updated the PR and fixed TODO in rest-proof-endpoint test.

[algorandskiy]

What would happen if we would follow the same pattern used in msgpacktool, and add ":int" to the key's string when encoding it as JSON ? this would allow the reverse operation to perform that symmetrically. ( similar to ":b64" )
i.e. I'm not afraid of "breaking" the JSON formatted output here, since it's already "broken" for that use case.

This looks overkill, although :b64 or :addr is useful as encoding hints. Let's simply emit valid json here - JSONHandle and JSONStrictHandle are interchangable for encoding-decoding in Go code so there is no risk.

[tsachiherman]

What would happen if we would follow the same pattern used in msgpacktool, and add ":int" to the key's string when encoding it as JSON ? this would allow the reverse operation to perform that symmetrically. ( similar to ":b64" )
i.e. I'm not afraid of "breaking" the JSON formatted output here, since it's already "broken" for that use case.

This looks overkill, although :b64 or :addr is useful as encoding hints. Let's simply emit valid json here - JSONHandle and JSONStrictHandle are interchangable for encoding-decoding in Go code so there is no risk.

I agree that we need to emit valid jsons here; and for most of the endpoints, it would not be an issue. However, when it comes for some, it does. Two examples:

• when requesting the pending transactions, the caller might want to recalculate the transaction ids. In order to do so, the caller would need to msgp these - which would require a canonical msgp->json->msgp path. ( no maps here, but we do have few ":b64" )
• when requesting a block, the caller might want to perform a validation to ensure that the block header matches the payset. This would not work unless we have a canonical msgp->json->msgp.

by fixing it as is now, we're hosing ourselves since it would require us to break backward compatibility in the future.

[algorandskiy]

The caller needs to stick to msgp in these scenarios. Or we can implement "cjson" (canonical json) format later if needed.

[winder]

The caller needs to stick to msgp in these scenarios. Or we can implement "cjson" (canonical json) format later if needed.

I like the cjson idea for a backwards-compatible way to add this if/when we need to support such a format.

[tsachiherman]

The cjson sounds like a "hey, we messed up before - let's do it right this time" approach. Given that this is a new feature, I'd like to make sure we're not making any stupid mistakes here.

[codecov-commenter]

Codecov Report

Merging #2611 (cee70b3) into master (58fd804) will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
+ Coverage   46.97%   47.01%   +0.03%     
==========================================
  Files         348      348              
  Lines       55716    55723       +7     
==========================================
+ Hits        26172    26197      +25     
+ Misses      26600    26585      -15     
+ Partials     2944     2941       -3     

Impacted Files	Coverage Δ
daemon/algod/api/server/v2/handlers.go	0.00% <0.00%> (ø)
daemon/algod/api/server/v2/utils.go	16.81% <0.00%> (ø)
daemon/algod/api/server/v2/test/helpers.go	77.09% <100.00%> (+1.29%)	⬆️
ledger/blockqueue.go	81.03% <0.00%> (-4.03%)	⬇️
catchup/service.go	69.53% <0.00%> (-0.79%)	⬇️
network/wsNetwork.go	60.92% <0.00%> (ø)
ledger/acctupdates.go	62.66% <0.00%> (+0.33%)	⬆️
cmd/tealdbg/debugger.go	73.86% <0.00%> (+1.00%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️
crypto/merkletrie/node.go	93.48% <0.00%> (+1.86%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58fd804...cee70b3. Read the comment docs.

[algorandskiy]

Adding ":b64" or ":int" suffixes to REST API json a bit bizarre for me. Our SDKs have REST API models, can load json data and then serialize them into mspg if needed. We do have models generator for Java and Go, and need to extend it for Python/JS to keep it in sync with REST automatically.

[brianolson]

I think I'd like a policy that msgpack is always canonical and json is always best-effort and for convenience where API consumers find json easier than msgpack.

[tsachiherman]

Would this handle correctly a non-utf8 map keys ?
i.e.

 v.Map["a\xc5z"] = "def"

I think that the root of the issue here is that the objects we're trying to return via the REST API were not designed to be returned using that interface. For the cases that we did design ( i.e. we == @winder ), we have no issues.

[zeldovich]

Looks reasonable to me. I believe we always use msgpack for the authoritative version of any data (blocks, transactions, account state, etc), when storing or signing things, so the question for this change is just about the clients of this REST API. Seems good to emit valid JSON, if we can deal with existing clients (if any) that are expecting integer map keys.

I agree with Pavel that adding :int suffixes is a bit weird in this context. The reason I added a :b64 suffix in msgpacktool is because msgpacktool has no schema for the data it's transforming, and it has to guess the type when converting JSON back to msgpack. This is not the case for REST API responses.

[jasonpaulos]

This looks like a good change to me. I remember users have ran into this issue before, and I believe we've told them to use the msgpack response as a workaround. This PR fixes the underlying issue, which is great.