Fix header field serialization

- Implement correct header field serialization using varint
- Add and update tests

src/protobuf/qprotobufobject_p.cpp

@@ -88,7 +88,7 @@ QByteArray ProtobufObjectPrivate::serializeValue(const QVariant &propertyValue,
 
     if (fieldIndex != NotUsedFieldIndex
             && type != UnknownWireType) {
-        result.prepend(encodeHeaderByte(fieldIndex, type));
+        result.prepend(encodeHeader(fieldIndex, type));
     }
     return result;
 }

src/protobuf/qprotobufobject_p.h

@@ -40,7 +40,7 @@
 #include "qtprotobuflogging.h"
 #include "selfcheckiterator.h"
 
-#define ASSERT_FIELD_NUMBER(X) Q_ASSERT_X(X < 128 && X > 0, T::staticMetaObject.className(), "fieldIndex is out of range")
+#define ASSERT_FIELD_NUMBER(X) Q_ASSERT_X(X < 536870912 && X > 0, T::staticMetaObject.className(), "fieldIndex is out of range")
 
 namespace qtprotobuf {
 
@@ -127,8 +127,8 @@ public:
         };
     }
 
-    static char encodeHeaderByte(int fieldIndex, WireTypes wireType);
-    static bool decodeHeaderByte(char typeByte, int &fieldIndex, WireTypes &wireType);
+    static QByteArray encodeHeader(int fieldIndex, WireTypes wireType);
+    static bool decodeHeader(SelfcheckIterator &it, int &fieldIndex, WireTypes &wireType);
 
     static QByteArray serializeValue(const QVariant &propertyValue, int fieldIndex, const QMetaProperty &metaProperty);
     static QByteArray serializeUserType(const QVariant &propertyValue, int &fieldIndex, WireTypes &type);
@@ -304,7 +304,7 @@ public:
 
         QByteArray serializedList;
         for (auto &value : listValue) {
-            serializedList.append(encodeHeaderByte(outFieldIndex, LengthDelimited));
+            serializedList.append(encodeHeader(outFieldIndex, LengthDelimited));
             serializedList.append(serializeLengthDelimited(value));
         }
 
@@ -329,7 +329,7 @@ public:
                 continue;
             }
             QByteArray serializedValue = serializeLengthDelimited(value->serialize());
-            serializedValue.prepend(encodeHeaderByte(outFieldIndex, LengthDelimited));
+            serializedValue.prepend(encodeHeader(outFieldIndex, LengthDelimited));
             serializedList.append(serializedValue);
         }
 
@@ -351,7 +351,7 @@ public:
             QByteArray result;
             result = mapSerializeHelper<K, 1>(it.key(), kSerializer) + mapSerializeHelper<V, 2>(it.value(), vSerializer);
             prependLengthDelimitedSize(result);
-            result.prepend(encodeHeaderByte(outFieldIndex, LengthDelimited));
+            result.prepend(encodeHeader(outFieldIndex, LengthDelimited));
             mapResult.append(result);
         }
         outFieldIndex = NotUsedFieldIndex;
@@ -374,7 +374,7 @@ public:
             }
             result = mapSerializeHelper<K, 1>(it.key(), kSerializer) + mapSerializeHelper<V, 2>(it.value().data(), vSerializer);
             prependLengthDelimitedSize(result);
-            result.prepend(encodeHeaderByte(outFieldIndex, LengthDelimited));
+            result.prepend(encodeHeader(outFieldIndex, LengthDelimited));
             mapResult.append(result);
         }
         outFieldIndex = NotUsedFieldIndex;
@@ -388,7 +388,7 @@ public:
         QByteArray result = handlers.serializer(QVariant::fromValue<V>(value), mapIndex);
         if (mapIndex != NotUsedFieldIndex
                 && handlers.type != UnknownWireType) {
-            result.prepend(encodeHeaderByte(mapIndex, handlers.type));
+            result.prepend(encodeHeader(mapIndex, handlers.type));
         }
         return result;
     }
@@ -400,7 +400,7 @@ public:
         QByteArray result = handlers.serializer(QVariant::fromValue<V*>(value), mapIndex);
         if (mapIndex != NotUsedFieldIndex
                 && handlers.type != UnknownWireType) {
-            result.prepend(encodeHeaderByte(mapIndex, handlers.type));
+            result.prepend(encodeHeader(mapIndex, handlers.type));
         }
         return result;
     }
@@ -556,8 +556,7 @@ public:
         qProtoDebug() << __func__ << "count:" << count;
         SelfcheckIterator last = it + count;
         while (it != last) {
-            decodeHeaderByte(*it, mapIndex, type);
-            ++it;
+            decodeHeader(it, mapIndex, type);
             if(mapIndex == 1) {
                 key = deserializeMapHelper<K>(it);
             } else {
@@ -585,8 +584,7 @@ public:
         qProtoDebug() << __func__ << "count:" << count;
         SelfcheckIterator last = it + count;
         while (it != last) {
-            decodeHeaderByte(*it, mapIndex, type);
-            ++it;
+            decodeHeader(it, mapIndex, type);
             if(mapIndex == 1) {
                 key = deserializeMapHelper<K>(it);
             } else {
@@ -680,13 +678,16 @@ public:
             //Each iteration we expect iterator is setup to beginning of next chunk
             int fieldNumber = NotUsedFieldIndex;
             WireTypes wireType = UnknownWireType;
-            if (!ProtobufObjectPrivate::decodeHeaderByte(*it, fieldNumber, wireType)) {
+            if (!ProtobufObjectPrivate::decodeHeader(it, fieldNumber, wireType)) {
                 qProtoCritical() << "Message received doesn't contains valid header byte. "
                                     "Trying next, but seems stream is broken" << QString::number((*it), 16);
                 throw std::invalid_argument("Message received doesn't contains valid header byte. "
                                       "Seems stream is broken");
             }
 
+            //FIXME: This check is incorrect in protobuf syntax 3 perspective.
+            //It should be possible to process stream even if it contains field
+            //is not a part of the structure.
             auto propertyNumberIt = T::propertyOrdering.find(fieldNumber);
             if (propertyNumberIt == std::end(T::propertyOrdering)) {
                 qProtoCritical() << "Message received contains invalid field number. "
@@ -699,7 +700,6 @@ public:
             int propertyIndex = propertyNumberIt->second;
             QMetaProperty metaProperty = T::staticMetaObject.property(propertyIndex);
 
-            ++it;
             ProtobufObjectPrivate::deserializeProperty(object, wireType, metaProperty, it);
         }
     }
@@ -750,32 +750,31 @@ public:
  *  bit number | 7  6  5  4  3 | 2  1  0
  * @param fieldIndex The index of a property in parent object
  * @param wireType Serialization type used for the property with index @p fieldIndex
- * @return Byte with encoded fieldIndex and wireType
  *
- * @todo change types of @p fieldIndex and @p wireType to "char"
+ * @return Varint encoded fieldIndex and wireType
  */
-inline char ProtobufObjectPrivate::encodeHeaderByte(int fieldIndex, WireTypes wireType)
+inline QByteArray ProtobufObjectPrivate::encodeHeader(int fieldIndex, WireTypes wireType)
 {
-    char header = (fieldIndex << 3) | wireType;
-    return header;
+    uint32_t header = (fieldIndex << 3) | wireType;
+    return serializeBasic(header, fieldIndex);
 }
 
 /*! @brief Decode a property field index and its serialization type from the header byte
  *
- * @param[in] typeByte Header byte with encoded field index and serialization type
+ * @param[in] Iterator that points to header with encoded field index and serialization type
  * @param[out] fieldIndex Decoded index of a property in parent object
  * @param[out] wireType Decoded serialization type used for the property with index @p fieldIndex
  *
- * @todo true if both decoded wireType and fieldIndex have "allowed" values and false, otherwise
+ * @return true if both decoded wireType and fieldIndex have "allowed" values and false, otherwise
  */
-inline bool ProtobufObjectPrivate::decodeHeaderByte(char typeByte, int &fieldIndex, WireTypes &wireType)
+inline bool ProtobufObjectPrivate::decodeHeader(SelfcheckIterator &it, int &fieldIndex, WireTypes &wireType)
 {
     // bin(0x07) == 0000 0111
-    wireType = static_cast<WireTypes>(typeByte & 0x07);
-    fieldIndex = typeByte >> 3;
+    uint32_t header = deserializeVarintCommon<uint32_t>(it);
+    wireType = static_cast<WireTypes>(header & 0x07);
+    fieldIndex = header >> 3;
 
-    // FIXME: field index max value is 2**5 - 1 (== 32)
-    return fieldIndex < 128 && fieldIndex > 0 && (wireType == Varint
+    return fieldIndex < 536870912 && fieldIndex > 0 && (wireType == Varint
                                                   || wireType == Fixed64
                                                   || wireType == Fixed32
                                                   || wireType == LengthDelimited);

tests/test_protobuf/CMakeLists.txt

@@ -132,6 +132,10 @@ set(GENERATED_HEADERS
     simpleuintmessage.h
     stepchildenummessage.h
     emptymessage.h
+    fieldindextest1message.h
+    fieldindextest2message.h
+    fieldindextest3message.h
+    fieldindextest4message.h
 )
 
 file(GLOB SOURCES

tests/test_protobuf/deserializationtest.cpp

@@ -81,6 +81,12 @@
 #include "simpleuint64complexmessagemapmessage.h"
 
 #include "simplestringcomplexmessagemapmessage.h"
+
+#include "fieldindextest1message.h"
+#include "fieldindextest2message.h"
+#include "fieldindextest3message.h"
+#include "fieldindextest4message.h"
+
 using namespace qtprotobufnamespace::tests;
 using namespace qtprotobuf::tests;
 using namespace qtprotobuf;
@@ -867,3 +873,22 @@ TEST_F(DeserializationTest, SimpleStringComplexMapInvalidFieldDeserializeTest)
                  std::invalid_argument);
     ASSERT_TRUE(test.mapField().isEmpty());
 }
+
+TEST_F(DeserializationTest, FieldIndexRangeTest)
+{
+    FieldIndexTest1Message msg1({0});
+    msg1.deserialize(QByteArray::fromHex("f80102"));
+    ASSERT_EQ(msg1.testField(), 1);
+
+    FieldIndexTest2Message msg2({0});
+    msg2.deserialize(QByteArray::fromHex("f8ff0302"));
+    ASSERT_EQ(msg2.testField(), 1);
+
+    FieldIndexTest3Message msg3({0});
+    msg3.deserialize(QByteArray::fromHex("f8ffff0702"));
+    ASSERT_EQ(msg3.testField(), 1);
+
+    FieldIndexTest4Message msg4({0});
+    msg4.deserialize(QByteArray::fromHex("f8ffffff0f02"));
+    ASSERT_EQ(msg4.testField(), 1);
+}

tests/test_protobuf/proto/simpletest.proto

@@ -540,3 +540,19 @@ enum TestEnum {
     TEST_ENUM_VALUE3 = 4;
     TEST_ENUM_VALUE4 = 3;
 }
+
+message FieldIndexTest1Message {
+    sint32 testField = 31;
+}
+
+message FieldIndexTest2Message {
+    sint32 testField = 8191;
+}
+
+message FieldIndexTest3Message {
+    sint32 testField = 2097151;
+}
+
+message FieldIndexTest4Message {
+    sint32 testField = 536870911;
+}

tests/test_protobuf/serializationtest.cpp

@@ -71,6 +71,11 @@
 
 #include "simplestringstringmapmessage.h"
 
+#include "fieldindextest1message.h"
+#include "fieldindextest2message.h"
+#include "fieldindextest3message.h"
+#include "fieldindextest4message.h"
+
 #include "qtprotobuf.h"
 
 using namespace qtprotobufnamespace::tests;
@@ -1323,7 +1328,7 @@ TEST_F(SerializationTest, ComplexTypeSerializeTest)
     test.setTestComplexField(stringMsg);
 
     result = test.serialize();
-//    qDebug() << "result" << result.toHex();
+    //    qDebug() << "result" << result.toHex();
 
     ASSERT_TRUE(result == QByteArray::fromHex("1208320671776572747908d3ffffff0f")
                 || result == QByteArray::fromHex("08d3ffffff0f12083206717765727479"));
@@ -1569,7 +1574,7 @@ TEST_F(SerializationTest, SimpleFixed32StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "3a0a0d0a000000120374656e3a0e0d0f00000012076669667465656e3a110d2a000000120a666f757274792074776f");
+                 "3a0a0d0a000000120374656e3a0e0d0f00000012076669667465656e3a110d2a000000120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleSFixed32StringMapSerializeTest)
@@ -1579,7 +1584,7 @@ TEST_F(SerializationTest, SimpleSFixed32StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "4a170dd6ffffff12106d696e757320666f757274792074776f4a0a0d0a000000120374656e4a0e0d0f00000012076669667465656e");
+                 "4a170dd6ffffff12106d696e757320666f757274792074776f4a0a0d0a000000120374656e4a0e0d0f00000012076669667465656e");
 }
 
 TEST_F(SerializationTest, SimpleInt32StringMapSerializeTest)
@@ -1589,7 +1594,7 @@ TEST_F(SerializationTest, SimpleInt32StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "1a1108f6ffffff0f12096d696e75732074656e1a0b080f12076669667465656e1a0e082a120a666f757274792074776f");
+                 "1a1108f6ffffff0f12096d696e75732074656e1a0b080f12076669667465656e1a0e082a120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleSInt32StringMapSerializeTest)
@@ -1599,7 +1604,7 @@ TEST_F(SerializationTest, SimpleSInt32StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "0a14085312106d696e757320666f757274792074776f0a070814120374656e0a0b081e12076669667465656e");
+                 "0a14085312106d696e757320666f757274792074776f0a070814120374656e0a0b081e12076669667465656e");
 }
 
 TEST_F(SerializationTest, SimpleUInt32StringMapSerializeTest)
@@ -1609,7 +1614,7 @@ TEST_F(SerializationTest, SimpleUInt32StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "2a07080a120374656e2a0b080f12076669667465656e2a0e082a120a666f757274792074776f");
+                 "2a07080a120374656e2a0b080f12076669667465656e2a0e082a120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleFixed64StringMapSerializeTest)
@@ -1619,7 +1624,7 @@ TEST_F(SerializationTest, SimpleFixed64StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "420e090a00000000000000120374656e4212090f0000000000000012076669667465656e4215092a00000000000000120a666f757274792074776f");
+                 "420e090a00000000000000120374656e4212090f0000000000000012076669667465656e4215092a00000000000000120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleSFixed64StringMapSerializeTest)
@@ -1629,7 +1634,7 @@ TEST_F(SerializationTest, SimpleSFixed64StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "521b09d6ffffffffffffff12106d696e757320666f757274792074776f520e090a00000000000000120374656e5212090f0000000000000012076669667465656e");
+                 "521b09d6ffffffffffffff12106d696e757320666f757274792074776f520e090a00000000000000120374656e5212090f0000000000000012076669667465656e");
 }
 
 TEST_F(SerializationTest, SimpleInt64StringMapSerializeTest)
@@ -1639,7 +1644,7 @@ TEST_F(SerializationTest, SimpleInt64StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "221608f6ffffffffffffffff0112096d696e75732074656e220b080f12076669667465656e220e082a120a666f757274792074776f");
+                 "221608f6ffffffffffffffff0112096d696e75732074656e220b080f12076669667465656e220e082a120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleSInt64StringMapSerializeTest)
@@ -1648,7 +1653,7 @@ TEST_F(SerializationTest, SimpleSInt64StringMapSerializeTest)
     test.setMapField({{10, {"ten"}}, {-42, {"minus fourty two"}}, {15, {"fifteen"}}});
     QByteArray result = test.serialize();
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "1214085312106d696e757320666f757274792074776f12070814120374656e120b081e12076669667465656e");
+                 "1214085312106d696e757320666f757274792074776f12070814120374656e120b081e12076669667465656e");
 }
 
 TEST_F(SerializationTest, SimpleUInt64StringMapSerializeTest)
@@ -1658,7 +1663,7 @@ TEST_F(SerializationTest, SimpleUInt64StringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "3207080a120374656e320b080f12076669667465656e320e082a120a666f757274792074776f");
+                 "3207080a120374656e320b080f12076669667465656e320e082a120a666f757274792074776f");
 }
 
 TEST_F(SerializationTest, SimpleStringStringMapSerializeTest)
@@ -1668,7 +1673,34 @@ TEST_F(SerializationTest, SimpleStringStringMapSerializeTest)
     QByteArray result = test.serialize();
 
     ASSERT_STREQ(result.toHex().toStdString().c_str(),
-                "6a0a0a0362656e120374656e6a100a05737765657412076669667465656e6a210a13776861742069732074686520616e737765723f120a666f757274792074776f");
+                 "6a0a0a0362656e120374656e6a100a05737765657412076669667465656e6a210a13776861742069732074686520616e737765723f120a666f757274792074776f");
+}
+
+TEST_F(SerializationTest, FieldIndexRangeTest)
+{
+    FieldIndexTest1Message msg1;
+    msg1.setTestField(1);
+    QByteArray result = msg1.serialize();
+    ASSERT_STREQ(result.toHex().toStdString().c_str(),
+                 "f80102");
+
+    FieldIndexTest2Message msg2;
+    msg2.setTestField(1);
+    result = msg2.serialize();
+    ASSERT_STREQ(result.toHex().toStdString().c_str(),
+                 "f8ff0302");
+
+    FieldIndexTest3Message msg3;
+    msg3.setTestField(1);
+    result = msg3.serialize();
+    ASSERT_STREQ(result.toHex().toStdString().c_str(),
+                 "f8ffff0702");
+
+    FieldIndexTest4Message msg4;
+    msg4.setTestField(1);
+    result = msg4.serialize();
+    ASSERT_STREQ(result.toHex().toStdString().c_str(),
+                 "f8ffffff0f02");
 }
 
 //TEST_F(SerializationTest, DISABLE_BenchmarkTest)