Merge pull request #475 from MarketFactory/issue472

mjpt777 · web-flow · commit 7201946bc659 · 2017-06-27T09:02:55.000+01:00
Issue472 Handle presence="optional"
diff --git 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
@@ -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
@@ -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
@@ -422,16 +422,23 @@ 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())
         {
             return;
         }
 
         // 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,40 +488,56 @@ 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
                 {
                     // constValue is a char
                     sb.append(String.format(
                         "\t%1$s[0] = %2$s\n",
                         varName,
-                        token.encoding().constValue()));
+                        encoding.constValue()));
                 }
             }
             else
             {
                 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
@@ -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
@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<sbe:messageSchema xmlns:sbe="https://fixprotocol.io/2016/sbe"
+                   package="issue472"
+                   id="472"
+                   version="0"
+                   semanticVersion="1.0"
+                   description="issue 472 test case"
+                   byteOrder="bigEndian">
+    <types>
+        <composite name="messageHeader" description="Message identifiers and length of message root">
+            <type name="blockLength" primitiveType="uint16"/>
+            <type name="templateId" primitiveType="uint16"/>
+            <type name="schemaId" primitiveType="uint16"/>
+            <type name="version" primitiveType="uint16"/>
+        </composite>
+    </types>
+    <sbe:message name="issue472" id="1" description="issue 472 test">
+        <field name="optional" type="uint64" id="2" presence="optional"/>
+    </sbe:message>
+</sbe:messageSchema>

Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,6 @@ func (m SbeGoMessageHeader) Decode(_m SbeGoMarshaller, _r io.Reader) error {`
`84`	`84`	`return nil`
`85`	`85`	`}`
`86`	`86`
`87`		`-`
`88`	`87`	`func (m *SbeGoMarshaller) WriteUint8(w io.Writer, v uint8) error {`
`89`	`88`	`m.b1[0] = byte(v)`
`90`	`89`	`_, err := w.Write(m.b1)`