Repository: envoyproxy/envoy
File: source/common/protobuf/visitor.cc (C++)

Code before:
namespace {

absl::Status traverseMessageWorker(ConstProtoVisitor& visitor, const Protobuf::Message& message,
                                   std::vector<const Protobuf::Message*>& parents,
                                   bool was_any_or_top_level, bool recurse_into_any) {
  RETURN_IF_NOT_OK(visitor.onMessage(message, parents, was_any_or_top_level));

  // If told to recurse into Any messages, do that here and skip the rest of the function.
  if (recurse_into_any) {
    std::unique_ptr<Protobuf::Message> inner_message;
    absl::string_view target_type_url;

    if (message.GetTypeName() == "google.protobuf.Any") {
      auto* any_message = Protobuf::DynamicCastMessage<Protobuf::Any>(&message);
      inner_message = Helper::typeUrlToMessage(any_message->type_url());
      target_type_url = any_message->type_url();
      if (inner_message != nullptr) {
        RETURN_IF_NOT_OK(MessageUtil::unpackTo(*any_message, *inner_message));
      } else {
        ENVOY_LOG_MISC(debug,
                       "Unknown type URL '{}' during message traversal, type may be missing "
                       "from the descriptor pool",
                       any_message->type_url());
        return absl::InvalidArgumentError(
            fmt::format("Invalid type_url '{}' during traversal", target_type_url));
      }
    } else if (message.GetTypeName() == "xds.type.v3.TypedStruct") {
      auto output_or_error = Helper::convertTypedStruct<xds::type::v3::TypedStruct>(message);
      RETURN_IF_NOT_OK_REF(output_or_error.status());
      std::tie(inner_message, target_type_url) = std::move(output_or_error.value());
    } else if (message.GetTypeName() == "udpa.type.v1.TypedStruct") {
      auto output_or_error = Helper::convertTypedStruct<udpa::type::v1::TypedStruct>(message);
      RETURN_IF_NOT_OK_REF(output_or_error.status());
      std::tie(inner_message, target_type_url) = std::move(output_or_error.value());
    }

    if (inner_message != nullptr) {
      // Push the Any message as a wrapper.
      Helper::ScopedMessageParents scoped_parents(parents, message);
      return traverseMessageWorker(visitor, *inner_message, parents, true, recurse_into_any);
    } else if (!target_type_url.empty()) {
      return absl::InvalidArgumentError(
          fmt::format("Invalid type_url '{}' during traversal", target_type_url));
    }
  }
  Protobuf::ReflectableMessage reflectable_message = createReflectableMessage(message);
  const Protobuf::Descriptor* descriptor = reflectable_message->GetDescriptor();
  const Protobuf::Reflection* reflection = reflectable_message->GetReflection();
  for (int i = 0; i < descriptor->field_count(); ++i) {
    const Protobuf::FieldDescriptor* field = descriptor->field(i);
    visitor.onField(message, *field);

Review comment (bug):
```suggestion
      if (inner_message == nullptr) {
        ENVOY_LOG_MISC(error,
                       "Unknown type URL '{}' during message traversal, type may be missing "
                       "from the descriptor pool",
                       any_message->type_url());
        return absl::InvalidArgumentError(
            fmt::format("Invalid type_url '{}' during traversal", target_type_url));
      }
      RETURN_IF_NOT_OK(MessageUtil::unpackTo(*any_message, *inner_message));
```
More readable to have the condition be an error, and the normal behavior be not indented. I also upgraded the log message to an error, though it might be better to just drop the log message since it's going to be returning an error anyway.

Code after:

absl::Status traverseMessageWorker(ConstProtoVisitor& visitor, const Protobuf::Message& message,
                                   std::vector<const Protobuf::Message*>& parents,
                                   bool was_any_or_top_level, bool recurse_into_any) {
  RETURN_IF_NOT_OK(visitor.onMessage(message, parents, was_any_or_top_level));

  // If told to recurse into Any messages, do that here and skip the rest of the function.
  if (recurse_into_any) {
    std::unique_ptr<Protobuf::Message> inner_message;
    absl::string_view target_type_url;

    if (message.GetTypeName() == "google.protobuf.Any") {
      auto* any_message = Protobuf::DynamicCastMessage<Protobuf::Any>(&message);
      inner_message = Helper::typeUrlToMessage(any_message->type_url());
      target_type_url = any_message->type_url();
      if (inner_message == nullptr) {
        return absl::InvalidArgumentError(
            fmt::format("Invalid type_url '{}' during traversal", target_type_url));
      }
      RETURN_IF_NOT_OK(MessageUtil::unpackTo(*any_message, *inner_message));
    } else if (message.GetTypeName() == "xds.type.v3.TypedStruct") {
      auto output_or_error = Helper::convertTypedStruct<xds::type::v3::TypedStruct>(message);
      RETURN_IF_NOT_OK_REF(output_or_error.status());
      std::tie(inner_message, target_type_url) = std::move(output_or_error.value());
    } else if (message.GetTypeName() == "udpa.type.v1.TypedStruct") {
      auto output_or_error = Helper::convertTypedStruct<udpa::type::v1::TypedStruct>(message);
      RETURN_IF_NOT_OK_REF(output_or_error.status());
      std::tie(inner_message, target_type_url) = std::move(output_or_error.value());
    }

    if (inner_message != nullptr) {
      // Push the Any message as a wrapper.
      Helper::ScopedMessageParents scoped_parents(parents, message);
      return traverseMessageWorker(visitor, *inner_message, parents, true, recurse_into_any);
    } else if (!target_type_url.empty()) {
      return absl::InvalidArgumentError(
          fmt::format("Invalid type_url '{}' during traversal", target_type_url));
    }
  }
  Protobuf::ReflectableMessage reflectable_message = createReflectableMessage(message);
  const Protobuf::Descriptor* descriptor = reflectable_message->GetDescriptor();
  const Protobuf::Reflection* reflection = reflectable_message->GetReflection();
  for (int i = 0; i < descriptor->field_count(); ++i) {
    const Protobuf::FieldDescriptor* field = descriptor->field(i);
    visitor.onField(message, *field);

    // If this is a message, recurse in to the sub-message.
    if (field->cpp_type() == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE) {
      Helper::ScopedMessageParents scoped_parents(parents, message);

      if (field->is_repeated()) {