Repository: envoyproxy/envoy
File: source/extensions/filters/http/proto_api_scrubber/scrubbing_util_lib/field_checker.cc (C++)

Code before:
#include "source/extensions/filters/http/proto_api_scrubber/scrubbing_util_lib/field_checker.h"

#include "source/common/grpc/common.h"
#include "source/common/http/matching/data_impl.h"
#include "source/common/protobuf/protobuf.h"
#include "source/common/protobuf/utility.h"
#include "source/extensions/filters/http/proto_api_scrubber/filter_config.h"

#include "absl/strings/numbers.h"
#include "absl/strings/str_join.h"
#include "proto_processing_lib/proto_scrubber/field_checker_interface.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace ProtoApiScrubber {

std::string FieldChecker::resolveEnumName(const std::string& value_str,
                                          const Protobuf::Field* field) const {
  int enum_number;
  if (!absl::SimpleAtoi(value_str, &enum_number)) {
    return "";
  }

  // Extract Type Name from URL "type.googleapis.com/package.Name"
  absl::string_view type_name = Envoy::TypeUtil::typeUrlToDescriptorFullName(field->type_url());

  // Return the corresponding enum name.
  return filter_config_ptr_->getEnumName(std::string(type_name), enum_number);
}

std::string FieldChecker::constructFieldMask(const std::vector<std::string>& path,
                                             const Protobuf::Field* field) const {
  if (path.empty()) {
    return "";
  }

  // Translate the last segment of the `path` wherever required.
  std::string last_segment = path.back();
  switch (field->kind()) {
  case Protobuf::Field::TYPE_ENUM:
    // For enums, last segment of the path contains integer value of the enum.
    // It needs to be translated to the corresponding string value of the enum.
    if (std::string name = resolveEnumName(last_segment, field); !name.empty()) {
      last_segment = name;
    }
    break;

Review comment (bug):
High-level comment:
The return of a special value (empty string) when there's something wrong, here and in other places, is looking like a code-smell, as it requires all the layers above understand that an empty string could signify different error conditions.
I'll let you decide if that's ok or not as code-owner of this extension, and because this seems "private" to the internal utility library (AFAICT).

Code after:
#include "source/extensions/filters/http/proto_api_scrubber/scrubbing_util_lib/field_checker.h"

#include "source/common/grpc/common.h"
#include "source/common/http/matching/data_impl.h"
#include "source/common/protobuf/protobuf.h"
#include "source/common/protobuf/utility.h"
#include "source/extensions/filters/http/proto_api_scrubber/filter_config.h"

#include "absl/strings/numbers.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/str_join.h"
#include "proto_processing_lib/proto_scrubber/field_checker_interface.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace ProtoApiScrubber {

absl::StatusOr<absl::string_view>
FieldChecker::resolveEnumName(absl::string_view value_str, const Protobuf::Field* field) const {
  int enum_number;
  if (!absl::SimpleAtoi(value_str, &enum_number)) {
    return absl::InvalidArgumentError(
        absl::StrCat("Enum value '", value_str, "' is not a valid integer."));
  }

  // Extract Type Name from URL "type.googleapis.com/package.Name"
  absl::string_view type_name = Envoy::TypeUtil::typeUrlToDescriptorFullName(field->type_url());

  // Return the corresponding enum name.
  return filter_config_ptr_->getEnumName(type_name, enum_number);
}

std::string FieldChecker::constructFieldMask(const std::vector<std::string>& path,
                                             const Protobuf::Field* field) const {
  if (path.empty()) {
    return "";
  }

  // Translate the last segment of the `path` wherever required.
  absl::string_view last_segment = path.back();
  switch (field->kind()) {
  case Protobuf::Field::TYPE_ENUM: {
    if (auto name_or_status = resolveEnumName(last_segment, field);
        name_or_status.ok() && !name_or_status.value().empty()) {
      last_segment = name_or_status.value();
    } else {