Skip to content

Commit 0de68c1

Browse files
16bit-ykikoclaude
andcommitted
fix(serde): reject structs with missing required fields during deserialization
Previously, if a non-optional struct field was absent from the input, it was silently default-constructed. This broke variant backtracking: when deserializing variant<A, B>, if A's required fields were missing, A would still "succeed" and B would never be tried. Semantics aligned with Rust's serde: - Default strict: missing non-optional field → error - std::optional<T> fields are automatically allowed to be absent (like Rust's Option<T>) - schema::default_value attribute (like Rust's #[serde(default)]) allows a field to be absent, keeping its default-constructed value - defaulted<T> convenience alias for annotation<T, schema::default_value> Implementation: - Add required_field_mask<T>() — compile-time bitmask of required field indices (non-excluded, non-optional, no default_value attr) - Track seen_fields in deserialize_reflectable and reject if any required field is missing - Strip cv from refl::field_type before specialization checks - Add forwarding operator= to inherit_use_type annotation for move-only types (e.g. unique_ptr) Also: - Fix json_rpc_incoming::result to use defaulted<RawValue> - Update test structs to use defaulted<T> on nullable fields - Add 8 required field check tests (strict, optional, defaulted) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 259f13d commit 0de68c1

File tree

8 files changed

+157
-39
lines changed

8 files changed

+157
-39
lines changed

include/eventide/serde/serde/annotation.h

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <utility>
77

88
#include "eventide/serde/serde/attrs/behavior.h"
9+
#include "eventide/serde/serde/attrs/schema.h"
910

1011
namespace eventide::serde {
1112

@@ -69,6 +70,14 @@ struct annotation<T, Attrs...> : T {
6970
static_assert(detail::validate_attrs<std::tuple<Attrs...>>(), "Invalid attribute combination");
7071

7172
using T::T;
73+
74+
template <typename U>
75+
requires (!std::same_as<std::remove_cvref_t<U>, annotation> && std::assignable_from<T&, U>)
76+
constexpr annotation& operator=(U&& raw) {
77+
T::operator=(std::forward<U>(raw));
78+
return *this;
79+
}
80+
7281
using annotated_type = T;
7382
using attrs = std::tuple<Attrs...>;
7483
};
@@ -119,4 +128,10 @@ constexpr auto operator==(const L& lhs, const R& rhs) -> bool {
119128
return lhs == annotated_value(rhs);
120129
}
121130

131+
/// Convenience alias: annotation<T, schema::default_value>.
132+
/// Marks a field as allowed to be absent during deserialization,
133+
/// keeping its default-constructed value. Like Rust's #[serde(default)].
134+
template <typename T>
135+
using defaulted = annotation<T, schema::default_value>;
136+
122137
} // namespace eventide::serde

include/eventide/serde/serde/attrs/schema.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,11 @@ struct skip {};
113113

114114
struct flatten {};
115115

116+
/// Allow a field to be absent during deserialization.
117+
/// When missing, the field keeps its default-constructed value.
118+
/// Equivalent to Rust's #[serde(default)].
119+
struct default_value {};
120+
116121
template <fixed_string Name>
117122
struct rename {
118123
constexpr inline static std::string_view name = Name;

include/eventide/serde/serde/utils/reflectable.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -295,9 +295,11 @@ constexpr auto serialize_reflectable(S& s, const V& v) -> std::expected<typename
295295
return s_struct.end();
296296
}
297297

298-
/// True if field I of struct T may be absent from JSON without error.
299-
/// A field is optional if it is excluded (skip/flatten) or its underlying
300-
/// value type is std::optional<T>.
298+
/// True if field I of struct T may be absent during deserialization.
299+
/// A field is optional if:
300+
/// - excluded (skip/flatten)
301+
/// - its underlying type is std::optional<T> (like Rust's Option<T>)
302+
/// - annotated with schema::default_value (like Rust's #[serde(default)])
301303
template <typename T, std::size_t I>
302304
consteval bool is_field_optional() {
303305
if constexpr(schema::is_field_excluded<T, I>()) {
@@ -307,6 +309,10 @@ consteval bool is_field_optional() {
307309
// so strip cv before checking specialization.
308310
using field_t = std::remove_cv_t<refl::field_type<T, I>>;
309311
if constexpr(serde::annotated_type<field_t>) {
312+
using attrs_t = typename field_t::attrs;
313+
if constexpr(tuple_has_v<attrs_t, schema::default_value>) {
314+
return true;
315+
}
310316
return is_specialization_of<std::optional, typename field_t::annotated_type>;
311317
} else {
312318
return is_specialization_of<std::optional, field_t>;
@@ -379,6 +385,7 @@ constexpr auto deserialize_reflectable(D& d, V& v) -> std::expected<void, E> {
379385
}
380386

381387
// Verify all required (non-optional, non-excluded) fields were present.
388+
// This enables correct variant backtracking and catches malformed input.
382389
constexpr std::uint64_t required = required_field_mask<value_t>();
383390
if((seen_fields & required) != required) {
384391
return std::unexpected(E::type_mismatch);

pixi.toml

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,14 +98,14 @@ fd -H -e yaml -e yml -E pnpm-lock.yaml -x prettier --write && \
9898
fd -H "^\\.clang-(format|tidy)$" -x prettier --write --parser yaml
9999
"""
100100
format = { depends-on = [
101-
"format-cpp",
102-
"format-python",
103-
"format-lua",
104-
"format-web",
105-
"format-markdown",
106-
"format-json",
107-
"format-toml",
108-
"format-yaml"
101+
"format-cpp",
102+
"format-python",
103+
"format-lua",
104+
"format-web",
105+
"format-markdown",
106+
"format-json",
107+
"format-toml",
108+
"format-yaml"
109109
] }
110110

111111
[feature.format.target.win-64.tasks]
@@ -129,7 +129,7 @@ cmd = "xmake config --clean --yes --mode={{ build_type }}{% if pixi.is_osx %} --
129129
[tasks.ci-xmake-build]
130130
args = ["build_type"]
131131
depends-on = [
132-
{ task = "ci-xmake-configure", args = ["{{ build_type }}"] },
133-
{ task = "xmake-build" },
134-
{ task = "xmake-test" },
132+
{ task = "ci-xmake-configure", args = ["{{ build_type }}"] },
133+
{ task = "xmake-build" },
134+
{ task = "xmake-test" },
135135
]

src/ipc/json_codec.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ struct json_rpc_incoming {
5454
std::optional<protocol::RequestID> id;
5555
std::optional<std::string> method;
5656
std::optional<serde::RawValue> params;
57-
std::optional<serde::RawValue> result;
57+
// Not optional<RawValue> because "result": null is a valid success
58+
// response — optional would lose it as nullopt. defaulted<RawValue>
59+
// keeps absent → empty(), null → "null" text.
60+
serde::defaulted<serde::RawValue> result;
5861
std::optional<Error> error;
5962
};
6063

@@ -82,14 +85,14 @@ IncomingMessage JsonCodec::parse_message(std::string_view payload) {
8285

8386
// No method + has id → response
8487
if(envelope->id.has_value()) {
85-
auto has_result = envelope->result.has_value() && !envelope->result->empty();
88+
auto has_result = !envelope->result.empty();
8689
auto has_error = envelope->error.has_value();
8790

8891
if(has_error && !has_result) {
8992
return IncomingErrorResponse{*envelope->id, std::move(*envelope->error)};
9093
}
9194
if(has_result && !has_error) {
92-
return IncomingResponse{*envelope->id, std::move(envelope->result->data)};
95+
return IncomingResponse{*envelope->id, std::move(envelope->result.data)};
9396
}
9497
return IncomingErrorResponse{*envelope->id,
9598
Error(protocol::ErrorCode::InvalidRequest,

tests/unit/serde/json/simdjson_tests.cpp

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "eventide/zest/zest.h"
1414
#include "eventide/serde/json/deserializer.h"
1515
#include "eventide/serde/json/serializer.h"
16+
#include "eventide/serde/serde/annotation.h"
1617
#include "eventide/serde/serde/serde.h"
1718

1819
namespace eventide::serde {
@@ -379,6 +380,81 @@ TEST_CASE(misc_behavior) {
379380

380381
}; // TEST_SUITE(serde_simdjson)
381382

383+
// ═══════════════════════════════════════════════════════════════════════
384+
// Required field checks
385+
// ═══════════════════════════════════════════════════════════════════════
386+
387+
struct StrictStruct {
388+
int x{};
389+
std::string name;
390+
};
391+
392+
struct MixedStruct {
393+
int required_field{};
394+
std::optional<int> optional_field;
395+
defaulted<std::string> defaulted_field;
396+
};
397+
398+
struct AllOptional {
399+
std::optional<int> a;
400+
std::optional<std::string> b;
401+
};
402+
403+
TEST_SUITE(serde_required_fields) {
404+
405+
TEST_CASE(missing_required_field_fails) {
406+
// "name" is required (non-optional), missing → error
407+
auto result = from_json<StrictStruct>(R"({"x": 42})");
408+
EXPECT_FALSE(result.has_value());
409+
}
410+
411+
TEST_CASE(all_required_fields_present_succeeds) {
412+
auto result = from_json<StrictStruct>(R"({"x": 42, "name": "hello"})");
413+
ASSERT_TRUE(result.has_value());
414+
EXPECT_EQ(result->x, 42);
415+
EXPECT_EQ(result->name, "hello");
416+
}
417+
418+
TEST_CASE(empty_json_object_fails_if_required_fields) {
419+
auto result = from_json<StrictStruct>(R"({})");
420+
EXPECT_FALSE(result.has_value());
421+
}
422+
423+
TEST_CASE(optional_field_can_be_absent) {
424+
auto result = from_json<MixedStruct>(R"({"required_field": 7})");
425+
ASSERT_TRUE(result.has_value());
426+
EXPECT_EQ(result->required_field, 7);
427+
EXPECT_FALSE(result->optional_field.has_value());
428+
EXPECT_EQ(result->defaulted_field, std::string{});
429+
}
430+
431+
TEST_CASE(defaulted_field_can_be_absent) {
432+
auto result = from_json<MixedStruct>(R"({"required_field": 1})");
433+
ASSERT_TRUE(result.has_value());
434+
EXPECT_EQ(result->defaulted_field, std::string{});
435+
}
436+
437+
TEST_CASE(defaulted_field_present_is_used) {
438+
auto result = from_json<MixedStruct>(R"({"required_field": 1, "defaulted_field": "hi"})");
439+
ASSERT_TRUE(result.has_value());
440+
EXPECT_EQ(result->defaulted_field, std::string{"hi"});
441+
}
442+
443+
TEST_CASE(all_optional_struct_empty_object_succeeds) {
444+
auto result = from_json<AllOptional>(R"({})");
445+
ASSERT_TRUE(result.has_value());
446+
EXPECT_FALSE(result->a.has_value());
447+
EXPECT_FALSE(result->b.has_value());
448+
}
449+
450+
TEST_CASE(unknown_fields_ignored_by_default) {
451+
auto result = from_json<StrictStruct>(R"({"x": 1, "name": "ok", "extra": true})");
452+
ASSERT_TRUE(result.has_value());
453+
EXPECT_EQ(result->x, 1);
454+
}
455+
456+
}; // TEST_SUITE(serde_required_fields)
457+
382458
} // namespace
383459

384460
} // namespace eventide::serde

tests/unit/serde/json/simdjson_variant_detail_tests.cpp

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -718,13 +718,24 @@ TEST_CASE(in_optional) {
718718
EXPECT_FALSE(out.has_value());
719719
}
720720

721-
TEST_CASE(default_field_values_preserved) {
722-
// When a struct field is missing in the JSON, it should keep its default
721+
TEST_CASE(missing_required_field_rejects_tagged_candidate) {
722+
// Missing non-optional field in a tagged variant should fail deserialization
723723
IntTagShape out{};
724-
ASSERT_TRUE(from_json(R"({"type":"rect","width":5.0})", out).has_value());
725-
auto& rect = std::get<Rect>(out);
726-
EXPECT_EQ(rect.width, 5.0);
727-
EXPECT_EQ(rect.height, 0.0); // default
724+
auto result = from_json(R"({"type":"rect","width":5.0})", out);
725+
EXPECT_FALSE(result.has_value());
726+
}
727+
728+
TEST_CASE(missing_required_field_rejects_untagged_variant_candidate) {
729+
// In untagged variant probing, a missing required field should reject
730+
// the candidate and try the next alternative.
731+
// Circle has field "radius", Rect has "width" + "height".
732+
// {"width": 5.0} is missing "height" so Rect should be rejected.
733+
using UntaggedShape = std::variant<Rect, Circle>;
734+
UntaggedShape out{};
735+
// This should fail because Rect needs both width+height, and Circle
736+
// doesn't match either (no "radius" field).
737+
auto result = from_json(R"({"width":5.0})", out);
738+
EXPECT_FALSE(result.has_value());
728739
}
729740

730741
}; // TEST_SUITE(serde_variant_int_tag)

tests/unit/serde/standard_case_suite.h

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,13 @@
3939
#include <variant>
4040
#include <vector>
4141

42+
#include "eventide/serde/serde/annotation.h"
4243
#include "eventide/serde/serde/attrs.h"
4344

4445
namespace eventide::serde::standard_case {
4546

47+
using eventide::serde::defaulted;
48+
4649
struct Basic {
4750
bool is_valid{};
4851
std::int32_t i32{};
@@ -63,7 +66,7 @@ struct Compound {
6366
struct Nullables {
6467
std::optional<int> opt_value;
6568
std::optional<std::string> opt_empty;
66-
std::unique_ptr<Basic> heap_allocated;
69+
defaulted<std::unique_ptr<Basic>> heap_allocated;
6770

6871
auto operator==(const Nullables& other) const -> bool {
6972
if(opt_value != other.opt_value || opt_empty != other.opt_empty) {
@@ -87,7 +90,7 @@ enum class Role : std::uint8_t {
8790

8891
struct ADTs {
8992
Role role{};
90-
std::variant<std::monostate, int, std::string, Basic> multi_variant;
93+
defaulted<std::variant<std::monostate, int, std::string, Basic>> multi_variant;
9194

9295
auto operator==(const ADTs&) const -> bool = default;
9396
};
@@ -178,10 +181,10 @@ inline auto pointer_value_equal(const Ptr& lhs, const Ptr& rhs) -> bool {
178181
}
179182

180183
struct SmartPointers {
181-
std::unique_ptr<Basic> unique_basic;
182-
std::shared_ptr<Basic> shared_basic;
183-
std::shared_ptr<Basic> shared_empty;
184-
std::vector<std::shared_ptr<Basic>> shared_list;
184+
defaulted<std::unique_ptr<Basic>> unique_basic;
185+
defaulted<std::shared_ptr<Basic>> shared_basic;
186+
defaulted<std::shared_ptr<Basic>> shared_empty;
187+
defaulted<std::vector<std::shared_ptr<Basic>>> shared_list;
185188
std::optional<std::shared_ptr<Basic>> opt_shared;
186189

187190
auto operator==(const SmartPointers& other) const -> bool {
@@ -368,16 +371,14 @@ inline auto make_ultimate() -> Ultimate {
368371
.heterogeneous_tuple = std::tuple<int, bool, std::string>{7, true, "tuple"},
369372
};
370373

371-
out.nullables = Nullables{
372-
.opt_value = 17,
373-
.opt_empty = std::nullopt,
374-
.heap_allocated = std::make_unique<Basic>(Basic{
375-
.is_valid = false,
376-
.i32 = 128,
377-
.f64 = -9.75,
378-
.text = "heap",
379-
}),
380-
};
374+
out.nullables.opt_value = 17;
375+
out.nullables.opt_empty = std::nullopt;
376+
out.nullables.heap_allocated = std::make_unique<Basic>(Basic{
377+
.is_valid = false,
378+
.i32 = 128,
379+
.f64 = -9.75,
380+
.text = "heap",
381+
});
381382

382383
out.adts = ADTs{
383384
.role = Role::user,

0 commit comments

Comments
 (0)