Skip to content

Commit

Permalink
Deprecating field_ref::is_set
Browse files Browse the repository at this point in the history 
Reviewed By: iahs

Differential Revision: D62673423

fbshipit-source-id: b1228e9ac55502ca19f305fddbd44e1bd7487f2a
  • Loading branch information
@facebook-github-bot
TJ Yin authored and facebook-github-bot committed Sep 18, 2024
1 parent 706fa75 commit b56c904
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 22 deletions.
59 changes: 55 additions & 4 deletions thrift/lib/cpp2/FieldRef.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,15 +318,24 @@ class field_ref {
bitref_ = other.is_set();
}

[[deprecated("Use is_set() method instead")]] FOLLY_ERASE bool has_value()
const noexcept {
[[deprecated(
"Avoid using has_value() API for non-optional field since it's often used incorrectly. "
"If this is a legit use-case, please migrate to "
"apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()).")]]
FOLLY_ERASE bool has_value() const noexcept {
return bool(bitref_);
}

/// Returns true iff the field is set. field_ref doesn't provide conversion to
/// bool to avoid confusion between checking if the field is set and getting
/// the field's value, particularly for bool fields.
FOLLY_ERASE bool is_set() const noexcept { return bool(bitref_); }
[[deprecated(
"Avoid using is_set() API for non-optional field since it's often used incorrectly. "
"If this is a legit use-case, please migrate to "
"apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()).")]]
FOLLY_ERASE bool is_set() const noexcept {
return bool(bitref_);
}

/// Returns a reference to the value.
FOLLY_ERASE reference_type value() const noexcept {
Expand Down Expand Up @@ -1224,7 +1233,13 @@ class intern_boxed_field_ref {
// Returns true iff the field is set. 'intern_boxed_field_ref' doesn't provide
// conversion to bool to avoid confusion between checking if the field is set
// and getting the field's value, particularly for bool fields.
FOLLY_ERASE bool is_set() const noexcept { return bool(bitref_); }
[[deprecated(
"Avoid using is_set() API for non-optional field since it's often used incorrectly. "
"If this is a legit use-case, please migrate to "
"apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()).")]]
FOLLY_ERASE bool is_set() const noexcept {
return bool(bitref_);
}

FOLLY_ERASE void reset() noexcept {
// reset to the intern default.
Expand Down Expand Up @@ -1657,6 +1672,18 @@ struct alias_isset_fn {
}
};

struct is_non_optional_field_set_manually_or_by_serializer_fn {
template <typename T>
bool operator()(field_ref<T> ref) const noexcept {
return ref.is_set();
}

template <typename T>
bool operator()(intern_boxed_field_ref<T> ref) const noexcept {
return ref.is_set();
}
};

template <typename T>
FOLLY_ERASE apache::thrift::optional_field_ref<T&&> make_optional_field_ref(
T&& ref,
Expand Down Expand Up @@ -1730,6 +1757,30 @@ constexpr apache::thrift::detail::move_to_unique_ptr_fn move_to_unique_ptr;
constexpr apache::thrift::detail::assign_from_unique_ptr_fn
assign_from_unique_ptr;

// is_non_optional_field_set_manually_or_by_serializer
//
// Whether the non-optional field is set manually or by the serializer.
//
// When it is set manually, it's true when users use `obj.field_ref() = value`.
// If users use `*obj.field_ref() = value` it will still be false.
//
// When it is set by the serializer, it can return false if
// 1. Thrift struct is not generated by deserializer. (e.g., Users created it
// manually).
// 2. It is actually an optional field in the serializer due to schema
// evolution, and the serializer did not set the field.
// 3. It doesn't exist in the serializer due to schema evolution.
//
// Note that we will always try to serialize this field since it's not an
// optional field.
//
// Example:
//
// apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref());
constexpr apache::thrift::detail::
is_non_optional_field_set_manually_or_by_serializer_fn
is_non_optional_field_set_manually_or_by_serializer;

[[deprecated("Use `emplace` or `operator=` to set Thrift fields.")]] //
constexpr apache::thrift::detail::ensure_isset_unsafe_fn ensure_isset_unsafe;

Expand Down
44 changes: 26 additions & 18 deletions thrift/test/FieldRefTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,14 @@ class TestStructInternBoxedValue {
apache::thrift::detail::isset_bitset<2> __isset{};
};

template <class FieldRef>
bool is_set(FieldRef s) {
EXPECT_EQ(
s.is_set(),
apache::thrift::is_non_optional_field_set_manually_or_by_serializer(s));
return s.is_set();
}

// TODO(dokwon): Clean up FieldRefTest using TYPED_TEST.
TEST(field_ref_test, access_default_value) {
auto s = TestStruct();
Expand All @@ -378,17 +386,17 @@ TEST(field_ref_test, access_default_value) {

TEST(field_ref_test, has_value) {
auto s = TestStruct();
EXPECT_FALSE(s.name().is_set());
EXPECT_FALSE(is_set(s.name()));
s.name() = "foo";
EXPECT_TRUE(s.name().is_set());
EXPECT_TRUE(is_set(s.name()));
}

TEST(field_ref_test, assign) {
auto s = TestStruct();
EXPECT_FALSE(s.name().is_set());
EXPECT_FALSE(is_set(s.name()));
EXPECT_EQ(*s.name(), "default");
s.name() = "foo";
EXPECT_TRUE(s.name().is_set());
EXPECT_TRUE(is_set(s.name()));
EXPECT_EQ(*s.name(), "foo");
}

Expand All @@ -397,10 +405,10 @@ TEST(field_ref_test, copy_from) {
auto s2 = TestStruct();
s.name() = "foo";
s.name().copy_from(s2.name());
EXPECT_FALSE(s.name().is_set());
EXPECT_FALSE(is_set(s.name()));
s2.name() = "foo";
s.name().copy_from(s2.name());
EXPECT_TRUE(s.name().is_set());
EXPECT_TRUE(is_set(s.name()));
EXPECT_EQ(*s.name(), "foo");
}

Expand All @@ -410,7 +418,7 @@ TEST(field_ref_test, copy_from_const) {
const auto& s_const = s2;
s2.name() = "foo";
s.name().copy_from(s_const.name());
EXPECT_TRUE(s.name().is_set());
EXPECT_TRUE(is_set(s.name()));
EXPECT_EQ(*s.name(), "foo");
}

Expand All @@ -419,7 +427,7 @@ TEST(field_ref_test, copy_from_other_type) {
auto s2 = TestStruct();
s2.int_val() = 42;
s.int_assign().copy_from(s2.int_val());
EXPECT_TRUE(s.int_assign().is_set());
EXPECT_TRUE(is_set(s.int_assign()));
EXPECT_EQ(s.int_assign()->value, 42);
}

Expand All @@ -442,7 +450,7 @@ TEST(field_ref_test, is_assignable) {
TEST(field_ref_test, assign_forwards) {
auto s = TestStruct();
s.int_assign() = 42;
EXPECT_TRUE(s.int_assign().is_set());
EXPECT_TRUE(is_set(s.int_assign()));
EXPECT_EQ(s.int_assign()->value, 42);
}

Expand All @@ -451,7 +459,7 @@ TEST(field_ref_test, construct_const_from_mutable) {
s.name() = "foo";
field_ref<std::string&> name = s.name();
field_ref<const std::string&> const_name = name;
EXPECT_TRUE(const_name.is_set());
EXPECT_TRUE(is_set(const_name));
EXPECT_EQ(*const_name, "foo");
}

Expand Down Expand Up @@ -484,15 +492,15 @@ TEST(field_ref_test, mutable_accessors) {
EXPECT_EQ(*name, "baz");

// Field is not marked as set but that's OK for unqualified field.
EXPECT_FALSE(name.is_set());
EXPECT_FALSE(is_set(name));
}

TEST(field_ref_test, ensure) {
TestStruct s;
s.name().value() = "foo";
EXPECT_FALSE(s.name().is_set());
EXPECT_FALSE(is_set(s.name()));
s.name().ensure();
EXPECT_TRUE(s.name().is_set());
EXPECT_TRUE(is_set(s.name()));
EXPECT_EQ(s.name(), "foo");
}

Expand Down Expand Up @@ -1083,7 +1091,7 @@ TYPED_TEST(intern_boxed_field_ref_typed_test, emplace) {
typename TestFixture::Struct s;
s.struct_field1().emplace();
if constexpr (TestFixture::Struct::qual == FieldQualifier::Fill) {
EXPECT_TRUE(s.struct_field1().is_set());
EXPECT_TRUE(is_set(s.struct_field1()));
}
EXPECT_NE(
&*std::as_const(s).struct_field1(), &*std::as_const(s).struct_field2());
Expand All @@ -1093,21 +1101,21 @@ TYPED_TEST(intern_boxed_field_ref_typed_test, emplace) {
TYPED_TEST(intern_boxed_field_ref_typed_test, has_value) {
typename TestFixture::Struct s;
if constexpr (TestFixture::Struct::qual == FieldQualifier::Fill) {
EXPECT_FALSE(s.struct_field1().is_set());
EXPECT_FALSE(is_set(s.struct_field1()));
s.struct_field1() = ThriftStruct();
EXPECT_TRUE(s.struct_field1().is_set());
EXPECT_TRUE(is_set(s.struct_field1()));
}
}

TYPED_TEST(intern_boxed_field_ref_typed_test, ensure) {
typename TestFixture::Struct s;
s.struct_field1().value().name() = "foo";
if constexpr (TestFixture::Struct::qual == FieldQualifier::Fill) {
EXPECT_FALSE(s.struct_field1().is_set());
EXPECT_FALSE(is_set(s.struct_field1()));
}
s.struct_field1().ensure();
if constexpr (TestFixture::Struct::qual == FieldQualifier::Fill) {
EXPECT_TRUE(s.struct_field1().is_set());
EXPECT_TRUE(is_set(s.struct_field1()));
}
EXPECT_EQ(s.struct_field1()->name(), "foo");
}
Expand Down

0 comments on commit b56c904

Please sign in to comment.