From 68a2efb987a2321ccc2b689a8140db2c8df6c7ed Mon Sep 17 00:00:00 2001 From: Hong Shin Date: Thu, 18 Sep 2025 14:15:17 -0700 Subject: [PATCH] hpb: introduce hpb::StatusOr parse that takes in hpb::ParseOptions This CL deprecates the hpb::Parse overload that returns absl::StatusOr. Instead, prefer hpb::Parse -> hpb::StatusOr. String aliasing via upb is allow plumbed through with this CL. PiperOrigin-RevId: 808732870 --- hpb/BUILD | 11 ++++++ hpb/backend/upb/BUILD | 2 ++ hpb/backend/upb/upb.h | 20 +++++++++++ hpb/hpb.h | 11 ++++++ hpb/options.h | 50 +++++++++++++++++++++++++++ hpb_generator/tests/BUILD | 4 +++ hpb_generator/tests/extension_test.cc | 26 ++++++++++++++ hpb_generator/tests/test_generated.cc | 26 ++++++++++++++ 8 files changed, 150 insertions(+) create mode 100644 hpb/options.h diff --git a/hpb/BUILD b/hpb/BUILD index e30486f18ca9c..2bfeff038d31f 100644 --- a/hpb/BUILD +++ b/hpb/BUILD @@ -49,6 +49,7 @@ cc_library( ":arena", ":extension", ":multibackend", + ":options", ":ptr", ":status", "//hpb/internal", @@ -194,3 +195,13 @@ cc_library( "@abseil-cpp//absl/base:core_headers", ], ) + +cc_library( + name = "options", + hdrs = ["options.h"], + visibility = ["//visibility:public"], + deps = [ + "//hpb:extension", + "//hpb:multibackend", + ], +) diff --git a/hpb/backend/upb/BUILD b/hpb/backend/upb/BUILD index 97d5bd545d829..2f979fa6fc877 100644 --- a/hpb/backend/upb/BUILD +++ b/hpb/backend/upb/BUILD @@ -18,8 +18,10 @@ cc_library( ":interop", "//hpb:arena", "//hpb:extension", + "//hpb:options", "//hpb:ptr", "//hpb:status", + "//hpb/backend:types", "//hpb/internal", "//hpb/internal:message_lock", "//hpb/internal:template_help", diff --git a/hpb/backend/upb/upb.h b/hpb/backend/upb/upb.h index a5cc679f5bb77..32bb82efb6914 100644 --- a/hpb/backend/upb/upb.h +++ b/hpb/backend/upb/upb.h @@ -13,11 +13,13 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "hpb/arena.h" +#include "hpb/backend/types.h" #include "hpb/backend/upb/interop.h" #include "hpb/extension.h" #include "hpb/internal/internal.h" #include "hpb/internal/message_lock.h" #include "hpb/internal/template_help.h" +#include "hpb/options.h" #include "hpb/ptr.h" #include "hpb/status.h" #include "upb/mem/arena.h" @@ -106,6 +108,24 @@ absl::StatusOr Parse(absl::string_view bytes, return MessageDecodeError(status); } +template +hpb::StatusOr Parse(absl::string_view bytes, ParseOptions options) { + int flags = 0; + if (options.alias_string) { + flags |= kUpb_DecodeOption_AliasString; + } + T message; + auto* arena = interop::upb::GetArena(&message); + upb_DecodeStatus status = upb_Decode( + bytes.data(), bytes.size(), interop::upb::GetMessage(&message), + interop::upb::GetMiniTable(&message), + internal::GetUpbExtensions(options.extension_registry), flags, arena); + if (status == kUpb_DecodeStatus_Ok) { + return hpb::StatusOr(std::move(message)); + } + return hpb::StatusOr(internal::backend::Error(status)); +} + } // namespace hpb::internal::backend::upb #include "upb/port/undef.inc" diff --git a/hpb/hpb.h b/hpb/hpb.h index 6bcb89f11e597..5171810e89410 100644 --- a/hpb/hpb.h +++ b/hpb/hpb.h @@ -17,7 +17,9 @@ #include "hpb/extension.h" #include "hpb/internal/template_help.h" #include "hpb/multibackend.h" +#include "hpb/options.h" #include "hpb/ptr.h" +#include "hpb/status.h" #if HPB_INTERNAL_BACKEND == HPB_INTERNAL_BACKEND_UPB #include "hpb/backend/upb/upb.h" @@ -70,7 +72,16 @@ ABSL_MUST_USE_RESULT bool Parse(internal::PtrOrRaw message, return backend::Parse(message, bytes, extension_registry); } +// Note that the default extension registry is the the generated registry. template +hpb::StatusOr Parse(absl::string_view bytes, ParseOptions options) { + return backend::Parse(bytes, options); +} + +// Deprecated. Use the overload that returns hpb::StatusOr instead. +// Note that the default extension registry is the empty registry. +template +ABSL_DEPRECATED("Prefer the overload that returns hpb::StatusOr") absl::StatusOr Parse(absl::string_view bytes, const ExtensionRegistry& extension_registry = ExtensionRegistry::empty_registry()) { diff --git a/hpb/options.h b/hpb/options.h new file mode 100644 index 0000000000000..bfb6dde6f11cd --- /dev/null +++ b/hpb/options.h @@ -0,0 +1,50 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2025 Google LLC. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +#ifndef GOOGLE_PROTOBUF_HPB_OPTIONS_H__ +#define GOOGLE_PROTOBUF_HPB_OPTIONS_H__ + +#include "hpb/extension.h" +#include "hpb/multibackend.h" + +namespace hpb { + +struct ParseOptions { + // If true, the parsed proto may alias the input string instead of copying. + // Aliased data could include string fields, unknown fields, and possibly + // other data. + // + // REQUIRES: the input string outlives the resulting proto. + bool alias_string = false; + +#if HPB_INTERNAL_BACKEND == HPB_INTERNAL_BACKEND_UPB + // For the upb backend, the user can determine which extension registry + // they wish to use. Unless there are compelling reasons to do otherwise, + // we recommend using the generated registry, which uses linker arrays + // and intelligently performs tree shaking when possible. + const ExtensionRegistry& extension_registry = + ExtensionRegistry::generated_registry(); +#endif +}; + +inline ParseOptions ParseOptionsDefault() { return ParseOptions(); } + +#if HPB_INTERNAL_BACKEND == HPB_INTERNAL_BACKEND_UPB +// The empty registry is provided here as a convenience for extant users. +// Prefer the generated registry. +inline ParseOptions ParseOptionsWithEmptyRegistry() { + ParseOptions options{ + .alias_string = false, + .extension_registry = ExtensionRegistry::empty_registry(), + }; + return options; +} +#endif + +} // namespace hpb + +#endif // GOOGLE_PROTOBUF_HPB_OPTIONS_H__ diff --git a/hpb_generator/tests/BUILD b/hpb_generator/tests/BUILD index 02502f9b9e778..afa58d16669ce 100644 --- a/hpb_generator/tests/BUILD +++ b/hpb_generator/tests/BUILD @@ -163,9 +163,11 @@ cc_test( "//hpb/backend/upb:interop", "//hpb:arena", "//hpb:extension", + "//hpb:options", "//hpb:requires", "//hpb:ptr", "//hpb:repeated_field", + "//hpb:status", "//upb/mem", "//upb/mini_table", ], @@ -180,7 +182,9 @@ cc_test( "//hpb", "//hpb:arena", "//hpb:extension", + "//hpb:options", "//hpb:requires", + "//hpb:status", "//hpb/backend/upb:interop", "//upb/mem", "@abseil-cpp//absl/status:status_matchers", diff --git a/hpb_generator/tests/extension_test.cc b/hpb_generator/tests/extension_test.cc index 82cb0c8b284a6..c1d5164e72ec2 100644 --- a/hpb_generator/tests/extension_test.cc +++ b/hpb_generator/tests/extension_test.cc @@ -20,7 +20,9 @@ #include "hpb/arena.h" #include "hpb/backend/upb/interop.h" #include "hpb/hpb.h" +#include "hpb/options.h" #include "hpb/requires.h" +#include "hpb/status.h" #include "upb/mem/arena.h" namespace { @@ -488,6 +490,30 @@ TEST(CppGeneratedCode, ParseWithExtensionRegistry) { ->ext_name()); } +TEST(CppGeneratedCode, HpbStatusGeneratedRegistry) { + TestModel model; + ThemeExtension extension1; + extension1.set_ext_name("Hello World"); + EXPECT_EQ(true, ::hpb::SetExtension(&model, ThemeExtension::theme_extension, + extension1) + .ok()); + hpb::Arena arena; + auto bytes = ::hpb::Serialize(&model, arena); + EXPECT_EQ(true, bytes.ok()); + + // By default, hpb::ParseOptionsDefault uses the generated registry. + hpb::StatusOr parsed_model = + ::hpb::Parse(bytes.value(), hpb::ParseOptionsDefault()); + EXPECT_EQ(true, parsed_model.ok()); + EXPECT_EQ(true, hpb::GetExtension(&parsed_model.value(), + ThemeExtension::theme_extension) + .ok()); + EXPECT_EQ("Hello World", hpb::GetExtension(&parsed_model.value(), + ThemeExtension::theme_extension) + .value() + ->ext_name()); +} + TEST(CppGeneratedCode, ClearSubMessage) { // Fill model. TestModel model; diff --git a/hpb_generator/tests/test_generated.cc b/hpb_generator/tests/test_generated.cc index c1986525671a2..a7e95c7b42e83 100644 --- a/hpb_generator/tests/test_generated.cc +++ b/hpb_generator/tests/test_generated.cc @@ -21,8 +21,10 @@ #include "hpb/arena.h" #include "hpb/backend/upb/interop.h" #include "hpb/hpb.h" +#include "hpb/options.h" #include "hpb/ptr.h" #include "hpb/requires.h" +#include "hpb/status.h" namespace { @@ -405,6 +407,30 @@ TEST(CppGeneratedCode, MessageMapStringKeyAndInt32Value) { EXPECT_EQ(false, result_after_delete.ok()); } +TEST(CppGeneratedCode, HpbStatus) { + TestModel model; + model.set_str1("lightweight status"); + hpb::Arena arena; + absl::StatusOr bytes = ::hpb::Serialize(&model, arena); + EXPECT_EQ(true, bytes.ok()); + + hpb::StatusOr parsed_model = ::hpb::Parse( + bytes.value(), hpb::ParseOptionsWithEmptyRegistry()); + EXPECT_EQ(true, parsed_model.ok()); + EXPECT_EQ("lightweight status", parsed_model.value().str1()); +} + +TEST(CppGeneratedCode, HpbStatusFail) { + hpb::StatusOr status = ::hpb::Parse( + "definitely not a proto", hpb::ParseOptionsWithEmptyRegistry()); + EXPECT_EQ(false, status.ok()); + EXPECT_EQ(status.error(), "Wire format was corrupt"); + + absl::StatusOr to_absl_status = status.ToAbslStatusOr(); + EXPECT_EQ(false, to_absl_status.ok()); + EXPECT_EQ(to_absl_status.status().message(), "Wire format was corrupt"); +} + TEST(CppGeneratedCode, SerializeUsingArena) { TestModel model; model.set_str1("Hello World");