Remove omitempty from *pb.go by tac0turtle · Pull Request #3889 · tendermint/tendermint

tac0turtle · 2019-08-08T10:10:13Z

remove omitempty from *pb.go files
added command to makefile for everytime make protoc_all is run
open question:
- Do we want to further remove omitempty from other places
  - https://github.com/tendermint/tendermint/blob/master/rpc/lib/types/types.go#L151
  - and other places

Signed-off-by: Marko Baricevic marbar3778@yahoo.com

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

- remove omitempty from *pb.go files - added command to makefile for everytime `make protoc_all` is run - open question: - Do we want to further remove omitempty from other places - https://github.com/tendermint/tendermint/blob/master/rpc/lib/types/types.go#L151 - and other places ref #3882 Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

codecov-io · 2019-08-08T10:18:31Z

Codecov Report

Merging #3889 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3889      +/-   ##
=========================================
- Coverage   66.87%   66.8%   -0.08%     
=========================================
  Files         219     221       +2     
  Lines       18486   18510      +24     
=========================================
+ Hits        12363   12365       +2     
- Misses       5197    5222      +25     
+ Partials      926     923       -3

Impacted Files	Coverage Δ
libs/common/result.go	`0% <0%> (ø)`
crypto/merkle/result.go	`0% <0%> (ø)`
consensus/state.go	`78.96% <0%> (-0.36%)`	⬇️
consensus/reactor.go	`77.96% <0%> (-0.12%)`	⬇️
blockchain/v0/reactor.go	`79.71% <0%> (+0.94%)`	⬆️
privval/signer_endpoint.go	`84% <0%> (+2.66%)`	⬆️
privval/signer_server.go	`100% <0%> (+4.34%)`	⬆️

abci/types/result.go

Makefile

safareli · 2019-08-08T12:31:38Z

Do we want to further remove omitempty from other places https://github.com/tendermint/tendermint/blob/master/rpc/lib/types/types.go#L151

omitempty shouldn't be removed from RPCResponse as it would be violation of (JSON-RPC v2 spec)[https://www.jsonrpc.org/specification#response_object], this parts in particular:

result
This member is REQUIRED on success.
This member MUST NOT exist if there was an error invoking the method.
The value of this member is determined by the method invoked on the Server.

error
This member is REQUIRED on error.
This member MUST NOT exist if there was no error triggered during invocation.
The value for this member MUST be an Object as defined in section 5.1.

(I had issue with use of omitempty for all fields in record.)

melekes

#3889 (comment)
#3889 (comment)

martyall · 2019-08-22T17:46:51Z

I'm looking at your most recent commit and I'm not if we're on the same page, so I just want to lay out how we understand the problem.

If you have omitempty in any field for JSON codecs, it causes the key to be omitted during serialization if the field is uninitialized. This makes it difficult for strongly typed languages (in our case Haskell) to parse the data idiomatically. For example this addition you've made b50aa9a#diff-e41225101b540b7e08bbbcddb8f8ffdcR499 defines

type RequestEcho struct {
	Message              string   `protobuf:"bytes,1,opt,name=message,proto3" json:"message,omitempty"`
	XXX_NoUnkeyedLiteral struct{} `json:"-"`
	XXX_unrecognized     []byte   `json:"-"`
	XXX_sizecache        int32    `json:"-"`

Doesn't this mean that in the event that the Message field wasn't initialized, then the JSON object will not include a message field?

Sorry if I'm wrong, but again just wanting to make sure we're on the same page. Long story short, if keys sometimes appear, sometimes don't it makes it really hard for us. If they don't have a value, you are still free to use JSON's null value, but it has to be there. Thanks!

tac0turtle · 2019-08-22T17:57:31Z

Yes, I believe we are on the same page. @melekes requested to change to use jsonpb and not remove omitempty from the *pb.go files. So it would be handled by the marshler instead of the script to remove from the pb.go files. This is how I understood #3889 (comment)

martyall · 2019-08-22T17:59:00Z

🙏

ebuchman

Looks like latest commit can replace #3912

types/tx.go

abci/types/types.pb.go

benchmarks/proto/test.pb.go

martyall · 2019-08-28T19:09:07Z

Just checking in, if I were to build a docker image on this branch would it do what we need?

abci/types/messages_test.go

crypto/merkle/result.go

libs/common/result.go

safareli · 2019-09-07T19:07:38Z

crypto/merkle/result.go

+//---------------------------------------------------------------------------
+// override JSON marshalling so we emit defaults (ie. disable omitempty)
+// note we need Unmarshal functions too because protobuf had the bright idea
+// to marshal int64->string. cool. cool, cool, cool: https://developers.google.com/protocol-buffers/docs/proto3#json


I might be misunderstanding something but AFAIK int64, fixed64 and uint64 should be encoded as strings?
#3898 (comment)

Yep, we should probably remove the "note we need ..."

melekes

utACK

abci/types/result.go

melekes · 2019-09-13T14:45:13Z

=== RUN   TestTransportMultiplexValidateNodeInfo
panic: send on closed channel

goroutine 3027 [running]:
github.com/tendermint/tendermint/p2p.TestTransportMultiplexAcceptNonBlocking.func1(0xc000d1c100, 0xc000cca240, 0xc000cca300, 0xc000cca2a0)
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:255 +0x55c
created by github.com/tendermint/tendermint/p2p.TestTransportMultiplexAcceptNonBlocking
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:224 +0x271

not sure if it's related. probably not

tac0turtle · 2019-09-15T18:58:26Z

@blinky3713 you can give the branch a try now, sorry for the delay

melekes · 2019-09-16T10:19:49Z

abci/types/result.go

-// override JSON marshalling so we emit defaults (ie. disable omitempty)
-// note we need Unmarshal functions too because protobuf had the bright idea
-// to marshal int64->string. cool. cool, cool, cool: https://developers.google.com/protocol-buffers/docs/proto3#json
+// override JSON marshalling so we emit defaults (ie. enable omitempty)


"disable omitempty" was correct. sorry for confusion

melekes · 2019-09-16T16:01:49Z

@marbar3778 can you also update the changelog?

tac0turtle added C:abci Component: Application Blockchain Interface T:ux Type: Issue or Pull Request related to developer experience labels Aug 8, 2019

tac0turtle requested a review from melekes August 8, 2019 10:10

tac0turtle requested review from ebuchman and xla as code owners August 8, 2019 10:10

melekes reviewed Aug 8, 2019

View reviewed changes

abci/types/result.go Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

melekes suggested changes Aug 11, 2019

View reviewed changes

This was referenced Aug 19, 2019

Track omitempty issue f-o-a-m/kepler#46

Closed

Generate full tendermint client using swagger.json f-o-a-m/kepler#47

Closed

This was referenced Aug 21, 2019

Don't use omitempty #3882

Closed

address PR comments and concerns #3912

Closed

bring back omitempty to *pb.go

b50aa9a

ebuchman reviewed Aug 23, 2019

View reviewed changes

types/tx.go Outdated Show resolved Hide resolved

abci/types/types.pb.go Show resolved Hide resolved

benchmarks/proto/test.pb.go Outdated Show resolved Hide resolved

Update types/tx.go

cc628c3

melekes reviewed Sep 3, 2019

View reviewed changes

abci/types/messages_test.go Show resolved Hide resolved

melekes added the WIP label Sep 3, 2019

melekes assigned tac0turtle Sep 3, 2019

golangcibot reviewed Sep 6, 2019

View reviewed changes

crypto/merkle/result.go Show resolved Hide resolved

libs/common/result.go Show resolved Hide resolved

custom marshlers

61fed2a

tac0turtle force-pushed the marko/remove_omitEmpty branch from af16790 to 61fed2a Compare September 6, 2019 13:25

tac0turtle added 3 commits September 6, 2019 15:31

undo benchmark omitepmty

4dbd913

Merge branch 'master' into marko/remove_omitEmpty

5684ea5

golangci lint fix

8c14be6

safareli reviewed Sep 7, 2019

View reviewed changes

Merge branch 'master' into marko/remove_omitEmpty

2983e96

melekes approved these changes Sep 13, 2019

View reviewed changes

abci/types/result.go Show resolved Hide resolved

Merge branch 'master' into marko/remove_omitEmpty

96f9618

tac0turtle removed the WIP label Sep 15, 2019

melekes reviewed Sep 16, 2019

View reviewed changes

cleanup comments

32b3eba

tac0turtle force-pushed the marko/remove_omitEmpty branch from 537971a to 32b3eba Compare September 16, 2019 10:27

Merge branch 'master' into marko/remove_omitEmpty

ac25b71

tac0turtle and others added 2 commits September 16, 2019 18:25

changelog_pending entry

813c01a

Merge branch 'master' into marko/remove_omitEmpty

f8701a5

melekes merged commit 1b54369 into master Sep 18, 2019

melekes deleted the marko/remove_omitEmpty branch September 18, 2019 09:32

melekes mentioned this pull request Sep 18, 2019

Tendermint not returning error code on the client side #3676

Closed

Conversation

tac0turtle commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

safareli commented Aug 8, 2019

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

martyall commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tac0turtle commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martyall commented Aug 22, 2019

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martyall commented Aug 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

safareli Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

melekes Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melekes commented Sep 13, 2019

Uh oh!

tac0turtle commented Sep 15, 2019

Uh oh!

melekes Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

melekes commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tac0turtle commented Aug 8, 2019 •

edited

Loading

codecov-io commented Aug 8, 2019 •

edited

Loading

martyall commented Aug 22, 2019 •

edited

Loading

tac0turtle commented Aug 22, 2019 •

edited

Loading