From 6f940d4307f45a107ca796fbca0970cc014b01a6 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Mon, 13 Jan 2025 12:38:08 -0800 Subject: [PATCH 1/3] add test cases demonstrating codec behavioud --- network/codec/cbor/cbor_behaviour_test.go | 66 +++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 network/codec/cbor/cbor_behaviour_test.go diff --git a/network/codec/cbor/cbor_behaviour_test.go b/network/codec/cbor/cbor_behaviour_test.go new file mode 100644 index 00000000000..dcb85b34272 --- /dev/null +++ b/network/codec/cbor/cbor_behaviour_test.go @@ -0,0 +1,66 @@ +package cbor + +import ( + "testing" + + "github.com/fxamacker/cbor/v2" + "github.com/stretchr/testify/assert" + + cborcodec "github.com/onflow/flow-go/model/encoding/cbor" +) + +// The CBOR network codec uses the cbor.ExtraDecErrorUnknownField option, which +// causes decoding to return an error when decoding a message which contains an +// extra field, not present in the target (struct into which we are decoding). +// +// This test validates this behaviour. +func TestBehaviour_DecodeExtraField(t *testing.T) { + type model1 struct { + A int + } + type model2 struct { + A int + B int + } + + m2 := model2{ + A: 100, + B: 200, + } + bz, err := cborcodec.EncMode.Marshal(m2) + assert.NoError(t, err) + + var m1 model1 + err = defaultDecMode.Unmarshal(bz, &m1) + assert.Error(t, err) + target := &cbor.UnknownFieldError{} + assert.ErrorAs(t, err, &target) +} + +// The CBOR network codec uses the cbor.ExtraDecErrorUnknownField option, which +// causes decoding to return an error when decoding a message which contains an +// extra field, not present in the target (struct into which we are decoding). +// +// This test validates that, when decoding a message which omits a field present +// in the target, no error is returned. +func TestBehaviour_DecodeOmittedField(t *testing.T) { + type model1 struct { + A int + } + type model2 struct { + A int + B int + } + + m1 := model1{ + A: 100, + } + bz, err := cborcodec.EncMode.Marshal(m1) + assert.NoError(t, err) + + var m2 model2 + err = defaultDecMode.Unmarshal(bz, &m2) + assert.NoError(t, err) + assert.Equal(t, m2.A, m1.A) + assert.Equal(t, m2.B, int(0)) +} From 46a252f2d3e5e97395519d7238a1e3694daedf78 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Thu, 16 Jan 2025 14:07:54 -0800 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Alexander Hentschel --- network/codec/cbor/cbor_behaviour_test.go | 117 ++++++++++++++++++---- 1 file changed, 95 insertions(+), 22 deletions(-) diff --git a/network/codec/cbor/cbor_behaviour_test.go b/network/codec/cbor/cbor_behaviour_test.go index dcb85b34272..48dd8bc6dd5 100644 --- a/network/codec/cbor/cbor_behaviour_test.go +++ b/network/codec/cbor/cbor_behaviour_test.go @@ -9,40 +9,77 @@ import ( cborcodec "github.com/onflow/flow-go/model/encoding/cbor" ) -// The CBOR network codec uses the cbor.ExtraDecErrorUnknownField option, which +// The CBOR network codec uses the [cbor.ExtraDecErrorUnknownField] option, which // causes decoding to return an error when decoding a message which contains an // extra field, not present in the target (struct into which we are decoding). // // This test validates this behaviour. func TestBehaviour_DecodeExtraField(t *testing.T) { - type model1 struct { - A int - } - type model2 struct { - A int - B int - } + t.Run("decoding NON-ZERO VALUE of extra field not present in the target struct is forbidden", func(t *testing.T) { + type model1 struct { + A int + } + type model2 struct { + A int + B int + } - m2 := model2{ - A: 100, - B: 200, - } - bz, err := cborcodec.EncMode.Marshal(m2) - assert.NoError(t, err) + m2 := model2{ + A: 100, + B: 200, + } + bz, err := cborcodec.EncMode.Marshal(m2) + assert.NoError(t, err) - var m1 model1 - err = defaultDecMode.Unmarshal(bz, &m1) - assert.Error(t, err) - target := &cbor.UnknownFieldError{} - assert.ErrorAs(t, err, &target) + var m1 model1 + err = defaultDecMode.Unmarshal(bz, &m1) + assert.Error(t, err) + target := &cbor.UnknownFieldError{} + assert.ErrorAs(t, err, &target) + }) + + t.Run("decoding ZERO VALUE of extra field not present in the target struct is forbidden", func(t *testing.T) { + type model1 struct { + A *int + } + type model2 struct { + A *int + B *int + } + + a := 100 + m2 := model2{ + A: &a, + // B has zero-value + } + bz, err := cborcodec.EncMode.Marshal(m2) + assert.NoError(t, err) + + var m1 model1 + err = defaultDecMode.Unmarshal(bz, &m1) + assert.Error(t, err) + target := &cbor.UnknownFieldError{} + assert.ErrorAs(t, err, &target) + }) } -// The CBOR network codec uses the cbor.ExtraDecErrorUnknownField option, which +// The CBOR network codec uses the [cbor.ExtraDecErrorUnknownField] option, which // causes decoding to return an error when decoding a message which contains an // extra field, not present in the target (struct into which we are decoding). // -// This test validates that, when decoding a message which omits a field present -// in the target, no error is returned. +// This test validates that, when decoding a message which OMITS a field present +// in the target, no error is returned. +// +// This behaviour is very useful for downwards compatibility: for example if we add +// a new field B to a struct, nodes running the updated software can still decode +// messages emitted by the old software - with the convention that in the decoded +// message, field B has the zero-value. +// However, note that the reverse (i.e. downwards compatibility) is not true *by default* +// Specifically the old software cannot decode the new struct, even if field B has the +// zero value, as demonstrated by the test [TestBehaviour_DecodeExtraField] above. +// +// Nevertheless, downwards compatibility can be improved with suitable conventions +// as demonstrated in the test [TestBehaviour_OmittingNewFieldForDownwardsCompatibility] below func TestBehaviour_DecodeOmittedField(t *testing.T) { type model1 struct { A int @@ -64,3 +101,39 @@ func TestBehaviour_DecodeOmittedField(t *testing.T) { assert.Equal(t, m2.A, m1.A) assert.Equal(t, m2.B, int(0)) } + +// This test demonstrates a possible convention for improving downwards compatibility, +// when we want to add a new field to an existing struct. Let's say that the struct +// `model1` describes the old data structure, to which we want to add a new integer +// field `B`. +// Note that the following pattern only works out of the box, if field `B` is required +// according to the new protocol convention. In other words, the new software can +// differentiate between the old and the new data model based on whether field `B` +// is present. +// The important aspects are +// 1. define field `B` as a pointer variable. Thereby, the new software can represent +// an old data model with `B` being nil, while the new data model always has `B` ≠ nil. +// 2. In the new software, provide the cbor directive `cbor:",omitempty"`, which instructs +// cbor to omit the field entirely during the encoding step. Thereby the new software +// reproduces the encoding of the old software when dealing with the old data model. +func TestBehaviour_OmittingNewFieldForDownwardsCompatibility(t *testing.T) { + type model1 struct { + A int + } + type model2 struct { + A int + B *int `cbor:",omitempty"` + } + + a := 100 + m2 := model2{ + A: a, + } + // m2.B is `nil`, which cbor will omit in the encoding step according to our directive "omitempty" + bz, err := cborcodec.EncMode.Marshal(m2) + assert.NoError(t, err) + + var m1 model1 + err = defaultDecMode.Unmarshal(bz, &m1) + assert.NoError(t, err) +} From f03b83e393c3df600107915150789bf5ec8cf229 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Thu, 16 Jan 2025 15:08:15 -0800 Subject: [PATCH 3/3] lint --- network/codec/cbor/cbor_behaviour_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/network/codec/cbor/cbor_behaviour_test.go b/network/codec/cbor/cbor_behaviour_test.go index 48dd8bc6dd5..1487bee18e4 100644 --- a/network/codec/cbor/cbor_behaviour_test.go +++ b/network/codec/cbor/cbor_behaviour_test.go @@ -68,12 +68,12 @@ func TestBehaviour_DecodeExtraField(t *testing.T) { // extra field, not present in the target (struct into which we are decoding). // // This test validates that, when decoding a message which OMITS a field present -// in the target, no error is returned. +// in the target, no error is returned. // // This behaviour is very useful for downwards compatibility: for example if we add // a new field B to a struct, nodes running the updated software can still decode -// messages emitted by the old software - with the convention that in the decoded -// message, field B has the zero-value. +// messages emitted by the old software - with the convention that in the decoded +// message, field B has the zero-value. // However, note that the reverse (i.e. downwards compatibility) is not true *by default* // Specifically the old software cannot decode the new struct, even if field B has the // zero value, as demonstrated by the test [TestBehaviour_DecodeExtraField] above.