Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206

jaekwon · 2018-07-19T01:08:13Z

Currently we don't write empty structs even if it's a non-nil pointer to an empty struct.
This loses information about nil vs empty structs, when say the field value is a non-nil pointer to an empty struct.
This causes all kinds of issues with Tendermint, and probably isn't the behavior that we want, and it isn't the same as goproto3 behavior either.

This PR tries to fix this.
Still to do:

write tests
test against tendermint

liamsi · 2018-07-19T10:58:01Z

This causes all kinds of issues with Tendermint, and probably isn't the behavior that we want, and it isn't the same as goproto3 behavior either.

Are the issues in tendermint noticeable in failing tests? Just asking because I'm using the latest pre-release of amino in this tendermint PR:
tendermint/tendermint#1742
All tendermint tests pass there (and the pre-release does not incl. the changes made here but the struct skipping code AFAIK).

- compare to proto3 behaviour - test for pointers to empty struct and nil pointer Signed-off-by: Liamsi <[email protected]>

liamsi · 2018-07-19T15:49:56Z

Thanks @jaekwon! I've added a very simple test-case in #209
The behaviour you wanted to change passes the test. There is a minor difference on what we end if when we encode "nothing" (see #209 for details and a change that fixes the failing test).

- use []byte{} not []byte(nil) Signed-off-by: Liamsi <[email protected]>

jaekwon · 2018-07-20T04:50:26Z

@liamsi Modified the fuzz tests to also include empty structs. There's one test case that still fails... we should also run the other fuzz tests to test decoding.

This reverts commit e90abf1

…writeemptyptr_tests

Tests for #206

liamsi

Looks good to me. We could improve one error message in being more precise and we could make the "nil struct pointers not supporeted"-error a const. error. such that the caller can handle the case if desired. We can think of this in follow up PRs.

We still need to test it against tendermint, though.

liamsi · 2018-08-05T16:38:36Z

binary-encode.go

+					// Proto3's Golang client does.
+					// This also makes it easier to upgrade to Amino2
+					// which would enable the encoding of nil structs.
+					return errors.New("Amino1 doesn't support nil struct pointers")


I propose this to be a sentinel error / constant. This would give the caller some control over the behaviour, too.
Also, I think the error message should be "Amino1 doesn't support nil struct pointers in lists"

Ok, this changed in later commit. Maybe, "nil struct pointers in lists are not supported when empty_elements field tag is set". The sentinel error might still be useful, though. We can merge this as it is and think about other cases where this makes sense too (see #191)

liamsi · 2018-08-05T16:55:51Z

binary-decode.go

+			//  * next ByteLength bytes are 0x00, and
+			//  * - erv is not a struct pointer, or
+			//    - field option doesn't have EmptyElements set
+			// (the condition below uses demorgan's law)


nit: Demorgan's

liamsi · 2018-08-05T17:26:54Z

tested against tendermint (develop). There will be a failing regression test. One has to apply the following changes for the test to pass:

Index: types/proto3_test.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- types/proto3_test.go	(revision 6e3c5e803322d7ae08dbbe6f885899c6204beed5)
+++ types/proto3_test.go	(date 1533489963000)
@@ -67,11 +67,6 @@
 		Height:  150,
 		Time:    &proto3.Timestamp{Seconds: seconds, Nanos: nanos},
 		NumTxs:  7,
-		// This is not fully skipped in amino (yet) although it is empty:
-		LastBlockID: &proto3.BlockID{
-			PartsHeader: &proto3.PartSetHeader{
-			},
-		},
 		TotalTxs:       100,
 		LastCommitHash: []byte("commit hash"),
 		DataHash:       []byte("data hash"),

liamsi · 2018-08-06T11:11:48Z

We should document that we differ from protobuf here but that we can enforce protobuf compatible behaviour by adding the empty_elements tag to structs (referencing #203).

Also, I'm wondering if this shouldn't be the default behaviour and the tag would explicitly signal that we deviate from proto3 instead (and add the tag into tendermint types to where necessary). But I do not think it is reasonable to block this PR on this decision / discussion.

Thanks @jaekwon. Merging now. And will prep a release afterwards.

* DecodeInt{8, 16} negative limit checks + tests (#125) Updates #120 Also adds a test for Byteslice encoding and decoding roundtripping and part slicing then rejoining. * fix circleci2.0 * some metalinter issues * skip over default values when encoding time (#178) * do not encode empty structs, unless `amino:"write_empty"` is set (#179) * skip over empty structs by default * slightly more info on panics * Fix zero time decoding (#190) - fix decoding of skipped fields in time, or completely skipped time - necessary because sec=0 and ns=0 do not result in time.Time{} and vice versa * Prepare release 0.11.0 (#193) * Fix time decoding & encoding of arrays and structs - top-level entry functions called with BinFieldNum:1 to properly encode e.g. arrays of structs (see non-time related test) - add defaultValue method, different from #196 it deals with multiply nested pointers * Removed dependency on tmlibs/common * Always write empty if struct field is pointer * add tests for #206 - test for pointers to empty struct and nil pointer * fix proto3 compatibility for empty structs * Add EmptyStruct to fuzz tests * Revert "fix proto3 compatibility for empty structs" * Do not allow encoding of nil struct pointers in a slice/array * By default, 0-length list elements are decoded as nil

Always write empty if struct field is pointer

fa6b28c

jaekwon requested a review from liamsi July 19, 2018 01:08

add tests for #206

9a02f42

- compare to proto3 behaviour - test for pointers to empty struct and nil pointer Signed-off-by: Liamsi <[email protected]>

liamsi and others added 2 commits July 19, 2018 18:38

fix proto3 compatibility for empty structs

e90abf1

- use []byte{} not []byte(nil) Signed-off-by: Liamsi <[email protected]>

Add EmptyStruct to fuzz tests

8202139

jaekwon mentioned this pull request Jul 20, 2018

Tests for #206 #209

Merged

liamsi and others added 5 commits August 1, 2018 18:41

cleanup and split up tests & roundtrip

b9cc22d

Revert "fix proto3 compatibility for empty structs"

97e46a8

This reverts commit e90abf1

Merge remote-tracking branch 'remotes/origin/jae/writeemptyptr' into …

2af9ca1

…writeemptyptr_tests

merge jae's branch and improve comment

0472424

Merge pull request #209 from Liamsi/writeemptyptr_tests

6db2352

Tests for #206

jaekwon force-pushed the jae/writeemptyptr branch from f8b2561 to 6db2352 Compare August 5, 2018 03:39

jaekwon added 2 commits August 4, 2018 20:43

Do not allow encoding of nil struct pointers in a slice/array

a137b4e

By default, 0-length list elements are decoded as nil

4908822

jaekwon changed the title ~~WIP Always write empty if struct field is pointer~~ Empty structs: write_empty and empty_elements Aug 5, 2018

jaekwon changed the title ~~Empty structs: write_empty and empty_elements~~ Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) Aug 5, 2018

jaekwon changed the title ~~Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set)~~ Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) or (write_empty is set) Aug 5, 2018

jaekwon changed the title ~~Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) or (write_empty is set)~~ Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) Aug 5, 2018

Bump version

a832898

jaekwon mentioned this pull request Aug 5, 2018

Disallow nil in lists (repeated fields) #212

Closed

liamsi approved these changes Aug 5, 2018

View reviewed changes

liamsi merged commit b501f6c into develop Aug 6, 2018

liamsi deleted the jae/writeemptyptr branch October 30, 2018 18:11

liamsi mentioned this pull request Apr 24, 2019

Protobuf3 compatibility and repeated nulls #260

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206

Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206

jaekwon commented Jul 19, 2018 •

edited by liamsi

Loading

liamsi commented Jul 19, 2018 •

edited

Loading

liamsi commented Jul 19, 2018 •

edited

Loading

jaekwon commented Jul 20, 2018

liamsi left a comment •

edited

Loading

liamsi Aug 5, 2018

liamsi Aug 5, 2018

liamsi commented Aug 5, 2018

liamsi commented Aug 6, 2018 •

edited

Loading

Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206

Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206

Conversation

jaekwon commented Jul 19, 2018 • edited by liamsi Loading

liamsi commented Jul 19, 2018 • edited Loading

liamsi commented Jul 19, 2018 • edited Loading

jaekwon commented Jul 20, 2018

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

liamsi Aug 5, 2018

Choose a reason for hiding this comment

liamsi Aug 5, 2018

Choose a reason for hiding this comment

liamsi commented Aug 5, 2018

liamsi commented Aug 6, 2018 • edited Loading

jaekwon commented Jul 19, 2018 •

edited by liamsi

Loading

liamsi commented Jul 19, 2018 •

edited

Loading

liamsi commented Jul 19, 2018 •

edited

Loading

liamsi left a comment •

edited

Loading

liamsi commented Aug 6, 2018 •

edited

Loading