diff --git a/build.gradle b/build.gradle index 321e1819ed..7b106eff89 100644 --- a/build.gradle +++ b/build.gradle @@ -523,6 +523,7 @@ task(generateGolangCodecsWithXSD, type: JavaExec) { args = ['sbe-tool/src/test/resources/group-with-data-schema.xml', 'sbe-tool/src/test/resources/FixBinary.xml', 'sbe-tool/src/test/resources/issue435.xml', + 'sbe-tool/src/test/resources/issue472.xml', 'sbe-tool/src/test/resources/since-deprecated-test-schema.xml', 'sbe-tool/src/test/resources/example-bigendian-test-schema.xml', 'gocode/resources/example-composite.xml', diff --git a/gocode/Makefile b/gocode/Makefile index 807f6b6c11..f06989db26 100644 --- a/gocode/Makefile +++ b/gocode/Makefile @@ -68,7 +68,7 @@ test: $(DEP) go install \ ;done)) (export GOPATH=$(GOPATH) && \ - (for t in baseline-bignendian mktdata group_with_data group_with_data_extension composite_elements composite since-deprecated simple issue435; do \ + (for t in baseline-bigendian mktdata group_with_data group_with_data_extension composite_elements composite since-deprecated simple issue435 issue472; do \ cd $(GOPATH)/src/$$t && \ go build && \ go fmt && \ diff --git a/gocode/src/issue472/Issue472_test.go b/gocode/src/issue472/Issue472_test.go new file mode 100644 index 0000000000..5a928881eb --- /dev/null +++ b/gocode/src/issue472/Issue472_test.go @@ -0,0 +1,59 @@ +package issue472 + +import ( + "bytes" + "testing" +) + +// Optional values should allow NullValue in RangeCheck +func TestOptionalRangeCheck(t *testing.T) { + var in Issue472 + Issue472Init(&in) + if err := in.RangeCheck(in.SbeSchemaVersion(), in.SbeSchemaVersion()); err != nil { + t.Log("Issue472 RangeCheck() did not accept a NullValue for an optional field", err) + t.Fail() + } +} + +// Optional values should be set to Null Value by the Init() method +func TestOptionalInit(t *testing.T) { + var in Issue472 + Issue472Init(&in) + if in.Optional != in.OptionalNullValue() { + t.Log("Issue472Init() failed to initialize optional field to Null Value") + t.Fail() + } +} + +func TestEncodeDecode(t *testing.T) { + + m := NewSbeGoMarshaller() + var in Issue472 + Issue472Init(&in) + var buf = new(bytes.Buffer) + + // in contains a single optional field which is not initialized + if err := in.Encode(m, buf, true); err != nil { + t.Log("Encoding Error", err) + t.Fail() + } + + // Create a new Issue472 and decode into it + out := *new(Issue472) + + if err := out.Decode(m, buf, in.SbeSchemaVersion(), in.SbeBlockLength(), true); err != nil { + t.Log("Decoding Error", err) + t.Fail() + } + t.Log("Issue472 decodes as: ", out) + + // Sanity checks + if buf.Len() != 0 { + t.Logf("buffer not drained") + t.Fail() + } + if in != out { + t.Logf("in != out\n%s\n%s", in, out) + t.Fail() + } +} diff --git a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/golang/GolangGenerator.java b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/golang/GolangGenerator.java index a9ed08cd74..267e008526 100644 --- a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/golang/GolangGenerator.java +++ b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/golang/GolangGenerator.java @@ -422,8 +422,15 @@ private void generateDecodePrimitive( private void generateRangeCheckPrimitive( final StringBuilder sb, final String varName, - final Token token) + final Token token, + final Boolean isOptional) { + // Note that constant encoding is a property of the type + // and so works on signal/encoding tokens but optionality is + // a property of the field and so is not set on the encoding token. + // For this reason we can use the token to check for constancy + // but pass in optionality as a parameter + // Constant values don't need checking if (token.isConstantEncoding()) { @@ -431,7 +438,7 @@ private void generateRangeCheckPrimitive( } // If this field is unknown then we have nothing to check - // Otherwise do a The Min,MaxValue checks (possibly for arrays) + // Otherwise do the Min,MaxValue checks (possibly for arrays) this.imports.add("fmt"); if (token.arrayLength() > 1) { @@ -449,9 +456,22 @@ private void generateRangeCheckPrimitive( } else { + // Optional fields can be NullValue which may be outside the + // range of Min->Max so we need a special case. + // Structured this way for go fmt sanity on both cases. + final String check; + if (isOptional) + { + check = "\t\tif %1$s != %1$sNullValue() && (%1$s < %1$sMinValue() || %1$s > %1$sMaxValue()) {\n"; + } + else + { + check = "\t\tif %1$s < %1$sMinValue() || %1$s > %1$sMaxValue() {\n"; + } + sb.append(String.format( "\tif %1$sInActingVersion(actingVersion) {\n" + - "\t\tif %1$s < %1$sMinValue() || %1$s > %1$sMaxValue() {\n" + + check + "\t\t\treturn fmt.Errorf(\"Range check failed on %1$s " + "(%%d < %%d > %%d)\", %1$sMinValue(), %1$s, %1$sMaxValue())\n" + "\t\t}\n" + @@ -468,24 +488,40 @@ private void generateRangeCheckPrimitive( } } - private void generateInitPrimitive( + private void generateOptionalInitPrimitive( final StringBuilder sb, final String varName, final Token token) { + final Encoding encoding = token.encoding(); + + // Optional items get initialized to their NullValue + sb.append(String.format( + "\t%1$s = %2$s\n", + varName, + generateNullValueLiteral(encoding.primitiveType(), encoding))); + } + + private void generateConstantInitPrimitive( + final StringBuilder sb, + final String varName, + final Token token) + { + final Encoding encoding = token.encoding(); + // Decode of a constant is simply assignment if (token.isConstantEncoding()) { // if primitiveType="char" this is a character array - if (token.encoding().primitiveType() == CHAR) + if (encoding.primitiveType() == CHAR) { - if (token.encoding().constValue().size() > 1) + if (encoding.constValue().size() > 1) { // constValue is a string sb.append(String.format( "\tcopy(%1$s[:], \"%2$s\")\n", varName, - token.encoding().constValue())); + encoding.constValue())); } else { @@ -493,7 +529,7 @@ private void generateInitPrimitive( sb.append(String.format( "\t%1$s[0] = %2$s\n", varName, - token.encoding().constValue())); + encoding.constValue())); } } else @@ -501,7 +537,7 @@ private void generateInitPrimitive( sb.append(String.format( "\t%1$s = %2$s\n", varName, - generateLiteral(token.encoding().primitiveType(), token.encoding().constValue().toString()))); + generateLiteral(encoding.primitiveType(), encoding.constValue().toString()))); } } } @@ -652,17 +688,28 @@ private int generateEncodeDecode( encode.append(generateEncodeOffset(gap, "")); decode.append(generateDecodeOffset(gap, "")); currentOffset += signalToken.encodedLength() + gap; + final String primitive = Character.toString(varName) + "." + propertyName; - // Encode of a constant is a nullop - if (!signalToken.isConstantEncoding()) + // Encode of a constant is a nullop and we want to + // initialize constant values. + if (signalToken.isConstantEncoding()) + { + generateConstantInitPrimitive(init, primitive, signalToken); + } + else { generateEncodePrimitive(encode, varName, formatPropertyName(signalToken.name()), signalToken); + } + // Optional tokens also get initialized + if (signalToken.isOptionalEncoding()) + { + generateOptionalInitPrimitive(init, primitive, signalToken); } - final String primitive = Character.toString(varName) + "." + propertyName; + generateDecodePrimitive(decode, primitive, signalToken); - generateRangeCheckPrimitive(rangeCheck, primitive, signalToken); - generateInitPrimitive(init, primitive, signalToken); + generateRangeCheckPrimitive(rangeCheck, primitive, signalToken, signalToken.isOptionalEncoding()); + break; case BEGIN_GROUP: @@ -1004,16 +1051,29 @@ private int generateFieldEncodeDecode( gap = encodingToken.offset() - currentOffset; encode.append(generateEncodeOffset(gap, "")); decode.append(generateDecodeOffset(gap, "")); + final String primitive = Character.toString(varName) + "." + propertyName; - // Encode of a constant is a nullop - if (!encodingToken.isConstantEncoding()) + // Encode of a constant is a nullop and we want to + // initialize constant values. + // (note: constancy is determined by the type's token) + if (encodingToken.isConstantEncoding()) + { + generateConstantInitPrimitive(init, primitive, encodingToken); + } + else { generateEncodePrimitive(encode, varName, formatPropertyName(signalToken.name()), encodingToken); } - final String primitive = Character.toString(varName) + "." + propertyName; + + // Optional tokens get initialized to NullValue + // (note: optionality is determined by the field's token) + if (signalToken.isOptionalEncoding()) + { + generateOptionalInitPrimitive(init, primitive, encodingToken); + } + generateDecodePrimitive(decode, primitive, encodingToken); - generateRangeCheckPrimitive(rc, primitive, encodingToken); - generateInitPrimitive(init, primitive, encodingToken); + generateRangeCheckPrimitive(rc, primitive, encodingToken, signalToken.isOptionalEncoding()); break; } diff --git a/sbe-tool/src/main/resources/golang/templates/SbeMarshallingBigEndian.go b/sbe-tool/src/main/resources/golang/templates/SbeMarshallingBigEndian.go index 2263a6f00d..1de8cb50a3 100755 --- a/sbe-tool/src/main/resources/golang/templates/SbeMarshallingBigEndian.go +++ b/sbe-tool/src/main/resources/golang/templates/SbeMarshallingBigEndian.go @@ -84,7 +84,6 @@ func (m *SbeGoMessageHeader) Decode(_m *SbeGoMarshaller, _r io.Reader) error { return nil } - func (m *SbeGoMarshaller) WriteUint8(w io.Writer, v uint8) error { m.b1[0] = byte(v) _, err := w.Write(m.b1) diff --git a/sbe-tool/src/test/resources/issue472.xml b/sbe-tool/src/test/resources/issue472.xml new file mode 100644 index 0000000000..b2eab51353 --- /dev/null +++ b/sbe-tool/src/test/resources/issue472.xml @@ -0,0 +1,20 @@ + + + + + + + + + + + + + +