pw_protobuf: Force use of callbacks for oneof

pw_protobuf's message structures previously generated each of the
members of a oneof group as a separate struct member, allowing multiple
of them to be set to serialize semantically-invalid wire messages.

This replaces oneof struct member generation with a single callback for
the entire oneof group, allowing the user to encode/decode the desired
oneof field using the wire stream encoder and decoder.

Change-Id: I2e4f79c5f7d2ec99eb036e0f93a77675a29a625c
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/236592
Reviewed-by: Armando Montanez <[email protected]>
Lint: Lint 🤖 <[email protected]>

pw_protobuf/codegen_message_test.cc

@@ -1301,7 +1301,7 @@ TEST(CodegenMessage, DISABLED_ReadDoesNotOverrun) {
        false,
        false,
        false,
-       false,
+       internal::CallbackType::kNone,
        0,
        sizeof(KeyValuePair::Message) * 2,
        {}},
@@ -1903,7 +1903,7 @@ TEST(CodegenMessage, DISABLED_WriteDoesNotOverrun) {
        false,
        false,
        false,
-       false,
+       internal::CallbackType::kNone,
        0,
        sizeof(KeyValuePair::Message) * 2,
        {}},
@@ -2079,5 +2079,148 @@ TEST(CodegenMessage, MaxSize) {
   EXPECT_EQ(count_message.enums.max_size(), RepeatedTest::kEnumsMaxSize);
 }
 
+TEST(CodegenMessage, OneOf_Encode) {
+  OneOfTest::Message message;
+
+  int invocations = 0;
+  message.type.SetEncoder([&invocations](OneOfTest::StreamEncoder& encoder) {
+    invocations++;
+    return encoder.WriteAnInt(32);
+  });
+
+  // clang-format off
+  constexpr uint8_t expected_proto[] = {
+    // type.an_int
+    0x08, 0x20,
+  };
+  // clang-format on
+
+  std::array<std::byte, 8> buffer;
+  OneOfTest::MemoryEncoder oneof_test(buffer);
+
+  EXPECT_EQ(oneof_test.Write(message), OkStatus());
+  EXPECT_EQ(invocations, 1);
+
+  EXPECT_EQ(oneof_test.size(), sizeof(expected_proto));
+  EXPECT_EQ(
+      std::memcmp(oneof_test.data(), expected_proto, sizeof(expected_proto)),
+      0);
+}
+
+TEST(CodegenMessage, OneOf_Encode_MultipleTimes) {
+  OneOfTest::Message message;
+
+  int invocations = 0;
+  message.type.SetEncoder([&invocations](OneOfTest::StreamEncoder& encoder) {
+    invocations++;
+    return encoder.WriteAString("oneof");
+  });
+
+  // clang-format off
+  constexpr uint8_t expected_proto[] = {
+    // type.a_string
+    0x12, 0x05, 'o', 'n', 'e', 'o', 'f'
+  };
+  // clang-format on
+
+  // Write the same message struct to two different buffers. Even though its
+  // internal state is modified during the write, it should be logically const
+  // with both writes successfully producing the same output.
+
+  std::array<std::byte, 8> buffer_1;
+  std::array<std::byte, 8> buffer_2;
+  OneOfTest::MemoryEncoder oneof_test_1(buffer_1);
+  OneOfTest::MemoryEncoder oneof_test_2(buffer_2);
+
+  EXPECT_EQ(oneof_test_1.Write(message), OkStatus());
+  EXPECT_EQ(invocations, 1);
+  EXPECT_EQ(oneof_test_1.size(), sizeof(expected_proto));
+  EXPECT_EQ(
+      std::memcmp(oneof_test_1.data(), expected_proto, sizeof(expected_proto)),
+      0);
+
+  EXPECT_EQ(oneof_test_2.Write(message), OkStatus());
+  EXPECT_EQ(invocations, 2);
+  EXPECT_EQ(oneof_test_2.size(), sizeof(expected_proto));
+  EXPECT_EQ(
+      std::memcmp(oneof_test_2.data(), expected_proto, sizeof(expected_proto)),
+      0);
+}
+
+TEST(CodegenMessage, OneOf_Encode_UnsetEncoderFails) {
+  OneOfTest::Message message;
+  std::array<std::byte, 8> buffer;
+  OneOfTest::MemoryEncoder oneof_test(buffer);
+  EXPECT_EQ(oneof_test.Write(message), Status::DataLoss());
+}
+
+TEST(CodegenMessage, OneOf_Decode) {
+  // clang-format off
+  constexpr uint8_t proto_data[] = {
+    // type.a_message
+    0x1a, 0x02, 0x08, 0x01,
+  };
+  // clang-format on
+
+  stream::MemoryReader reader(as_bytes(span(proto_data)));
+  OneOfTest::StreamDecoder stream_decoder(reader);
+
+  struct {
+    OneOfTest::Fields field;
+    OneOfTest::AMessage::Message submessage;
+    int invocations = 0;
+  } result;
+
+  OneOfTest::Message message;
+  message.type.SetDecoder(
+      [&result](OneOfTest::Fields field, OneOfTest::StreamDecoder& decoder) {
+        result.field = field;
+        result.invocations++;
+        if (field == OneOfTest::Fields::kAMessage) {
+          return decoder.GetAMessageDecoder().Read(result.submessage);
+        }
+        return Status::InvalidArgument();
+      });
+
+  EXPECT_EQ(stream_decoder.Read(message), OkStatus());
+  EXPECT_EQ(result.field, OneOfTest::Fields::kAMessage);
+  EXPECT_EQ(result.invocations, 1);
+  EXPECT_EQ(result.submessage.a_bool, true);
+}
+
+TEST(CodegenMessage, OneOf_Decode_MultipleOneOfFieldsFails) {
+  // clang-format off
+  constexpr uint8_t proto_data[] = {
+    // type.an_int
+    0x08, 0x20,
+    // type.a_message
+    0x1a, 0x02, 0x08, 0x01,
+  };
+  // clang-format on
+
+  stream::MemoryReader reader(as_bytes(span(proto_data)));
+  OneOfTest::StreamDecoder stream_decoder(reader);
+
+  OneOfTest::Message message;
+  message.type.SetDecoder(
+      [](OneOfTest::Fields, OneOfTest::StreamDecoder&) { return OkStatus(); });
+
+  EXPECT_EQ(stream_decoder.Read(message), Status::DataLoss());
+}
+
+TEST(CodegenMessage, OneOf_Decode_UnsetDecoderFails) {
+  // clang-format off
+  constexpr uint8_t proto_data[] = {
+    // type.an_int
+    0x08, 0x20,
+  };
+  // clang-format on
+
+  stream::MemoryReader reader(as_bytes(span(proto_data)));
+  OneOfTest::StreamDecoder stream_decoder(reader);
+  OneOfTest::Message message;
+  EXPECT_EQ(stream_decoder.Read(message), Status::DataLoss());
+}
+
 }  // namespace
 }  // namespace pw::protobuf

pw_protobuf/docs.rst

@@ -67,6 +67,13 @@ Message Structures
 The highest level API is based around message structures created through C++
 code generation, integrated with Pigweed's build system.
 
+.. warning::
+
+   Message structures only support a subset of protobuf field types. Before
+   continuing, refer to :ref:`pw_protobuf-message-limitations` to understand
+   what types of protobuf messages can and cannot be represented, and whether
+   or not message structures are a suitable choice.
+
 This results in the following generated structure:
 
 .. code-block:: c++
@@ -260,6 +267,31 @@ from one message type to another:
   encoder's ``status()`` call. Always check the status of calls or the encoder,
   as in the case of error, the encoded data will be invalid.
 
+.. _pw_protobuf-message-limitations:
+
+Limitations
+-----------
+``pw_protobuf``'s message structure API is incomplete. Generally speaking, it is
+reasonable to use for basic messages containing simple inline fields (scalar
+types, bytes, and strings) and nested messages of similar form. Beyond this,
+certain other types of protobuf specifiers can be used, but may be limited in
+how and when they are supported. These cases are described below.
+
+If an object representation of protobuf messages is desired, the Pigweed team
+recommends using `Nanopb`_, which is fully supported within Pigweed's build and
+RPC systems.
+
+Message structures are eventually intended to be replaced with an alternative
+object model. See `SEED-0103 <http://pwrev.dev/133971>`_ for additional
+information about how message structures came to be and our future plans.
+
+``oneof`` fields
+^^^^^^^^^^^^^^^^
+``oneof`` protobuf fields cannot be inlined within a message structure: they
+must be encoded and decoded using callbacks.
+
+.. _pw_protobuf-per-field-apis:
+
 Per-Field Writers and Readers
 =============================
 The middle level API is based around typed methods to write and read each
@@ -1039,7 +1071,7 @@ that can hold the set of values encoded by it, following these rules.
      message Store {
        Store nearest_store = 1;
        repeated int32 employee_numbers = 2;
-       string driections = 3;
+       string directions = 3;
        repeated string address = 4;
        repeated Employee employees = 5;
      }
@@ -1061,6 +1093,75 @@ that can hold the set of values encoded by it, following these rules.
   A Callback object can be converted to a ``bool`` indicating whether a callback
   is set.
 
+* Fields defined within a ``oneof`` group are represented by a ``OneOf``
+  callback.
+
+  .. code-block:: protobuf
+
+     message OnlineOrder {
+       Product product = 1;
+       Customer customer = 2;
+
+       oneof delivery {
+         Address shipping_address = 3;
+         Date pickup_date = 4;
+       }
+     }
+
+  .. code-block::
+
+     // No options set.
+
+  .. code-block:: c++
+
+     struct OnlineOrder::Message {
+       Product::Message product;
+       Customer::Message customer;
+       pw::protobuf::OneOf<OnlineOrder::StreamEncoder,
+                           OnlineOrder::StreamDecoder,
+                           OnlineOrder::Fields>
+         delivery;
+     };
+
+  Encoding a ``oneof`` field is identical to using a regular field callback.
+  The encode callback will be invoked once when the message is written. Users
+  must ensure that only a single field is written to the encoder within the
+  callback.
+
+  .. code-block:: c++
+
+     OnlineOrder::Message message;
+     message.delivery.SetEncoder(
+         [&pickup_date](OnlineOrder::StreamEncoder& encoder) {
+           return encoder.GetPickupDateEncoder().Write(pickup_date);
+         });
+
+  The ``OneOf`` decoder callback is invoked when reading a message structure
+  when a field within the ``oneof`` group is encountered. The found field is
+  passed to the callback.
+
+  If multiple fields from the ``oneof`` group are encountered within a ``Read``,
+  it will fail with a ``DATA_LOSS`` status.
+
+  .. code-block:: c++
+
+     OnlineOrder::Message message;
+     message.delivery.SetDecoder(
+         [this](OnlineOrder::Fields field, OnlineOrder::StreamDecoder& decoder) {
+           switch (field) {
+             case OnlineOrder::Fields::kShippingAddress:
+               PW_TRY(decoder.GetShippingAddressDecoder().Read(&this->shipping_address));
+               break;
+             case OnlineOrder::Fields::kPickupDate:
+               PW_TRY(decoder.GetPickupDateDecoder().Read(&this->pickup_date));
+               break;
+             default:
+               return pw::Status::DataLoss();
+           }
+
+           return pw::OkStatus();
+         });
+
 Message structures can be copied, but doing so will clear any assigned
 callbacks. To preserve functions applied to callbacks, ensure that the message
 structure is moved.
@@ -2313,10 +2414,9 @@ allocation, making it unsuitable for many embedded systems.
 
 nanopb
 ======
-`nanopb <https://github.com/nanopb/nanopb>`_ is a commonly used embedded
-protobuf library with very small code size and full code generation. It provides
-both encoding/decoding functionality and in-memory C structs representing
-protobuf messages.
+`Nanopb`_ is a commonly used embedded protobuf library with very small code size
+and full code generation. It provides both encoding/decoding functionality and
+in-memory C structs representing protobuf messages.
 
 nanopb works well for many embedded products; however, using its generated code
 can run into RAM usage issues when processing nontrivial protobuf messages due
@@ -2334,3 +2434,5 @@ intuitive user interface.
 
 Depending on the requirements of a project, either of these libraries could be
 suitable.
+
+.. _Nanopb: https://jpa.kapsi.fi/nanopb/

pw_protobuf/encoder.cc

@@ -252,19 +252,25 @@ Status StreamEncoder::Write(span<const std::byte> message,
   for (const auto& field : table) {
     // Calculate the span of bytes corresponding to the structure field to
     // read from.
-    const auto values =
+    ConstByteSpan values =
         message.subspan(field.field_offset(), field.field_size());
     PW_CHECK(values.begin() >= message.begin() &&
              values.end() <= message.end());
 
     // If the field is using callbacks, interpret the input field accordingly
     // and allow the caller to provide custom handling.
-    if (field.use_callback()) {
+    if (field.callback_type() == internal::CallbackType::kSingleField) {
       const Callback<StreamEncoder, StreamDecoder>* callback =
           reinterpret_cast<const Callback<StreamEncoder, StreamDecoder>*>(
               values.data());
       PW_TRY(callback->Encode(*this));
       continue;
+    } else if (field.callback_type() == internal::CallbackType::kOneOfGroup) {
+      const OneOf<StreamEncoder, StreamDecoder>* callback =
+          reinterpret_cast<const OneOf<StreamEncoder, StreamDecoder>*>(
+              values.data());
+      PW_TRY(callback->Encode(*this));
+      continue;
     }
 
     switch (field.wire_type()) {
@@ -560,7 +566,28 @@ Status StreamEncoder::Write(span<const std::byte> message,
     }
   }
 
+  ResetOneOfCallbacks(message, table);
+
   return status_;
 }
 
+void StreamEncoder::ResetOneOfCallbacks(
+    ConstByteSpan message, span<const internal::MessageField> table) {
+  for (const auto& field : table) {
+    // Calculate the span of bytes corresponding to the structure field to
+    // read from.
+    ConstByteSpan values =
+        message.subspan(field.field_offset(), field.field_size());
+    PW_CHECK(values.begin() >= message.begin() &&
+             values.end() <= message.end());
+
+    if (field.callback_type() == internal::CallbackType::kOneOfGroup) {
+      const OneOf<StreamEncoder, StreamDecoder>* callback =
+          reinterpret_cast<const OneOf<StreamEncoder, StreamDecoder>*>(
+              values.data());
+      callback->invoked_ = false;
+    }
+  }
+}
+
 }  // namespace pw::protobuf

pw_protobuf/public/pw_protobuf/encoder.h

@@ -789,6 +789,12 @@ class StreamEncoder {
                               WireType type,
                               size_t data_size);
 
+  // Callbacks for oneof fields set a flag to ensure they are only invoked once.
+  // To maintain logical constness of message structs passed to write, this
+  // resets each callback's invoked flag following a write operation.
+  void ResetOneOfCallbacks(ConstByteSpan message,
+                           span<const internal::MessageField> table);
+
   // The current encoder status. This status is only updated to reflect the
   // first error encountered. Any further write operations are blocked when the
   // encoder enters an error state.