Skip to content

Commit b4eecba

Browse files
committed
fix(expression): return parse error for non-string type nodes in JSON serde
IsTransformTerm, NamedReferenceFromJson, both LiteralFromJson overloads, and ExpressionFromJson read the 'type' discriminator with json[kType].get<std::string>() guarded only by contains(), so a present-but-non-string 'type' (e.g. a number) threw an uncaught nlohmann type_error out of the Result-returning parse API. Check is_string() first, matching the sibling OperationTypeFromJson guard, so malformed expression JSON yields JsonParseError instead of an escaped exception.
1 parent 3c5715c commit b4eecba

2 files changed

Lines changed: 34 additions & 6 deletions

File tree

‎src/iceberg/expression/json_serde.cc‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ nlohmann::json MakeTransformJson(std::string_view transform_str,
8686

8787
/// Helper to check if a JSON term represents a transform
8888
bool IsTransformTerm(const nlohmann::json& json) {
89-
return json.is_object() && json.contains(kType) &&
89+
return json.is_object() && json.contains(kType) && json[kType].is_string() &&
9090
json[kType].get<std::string>() == kTransform && json.contains(kTerm);
9191
}
9292

@@ -228,8 +228,9 @@ nlohmann::json ToJson(const BoundTransform& transform) {
228228

229229
Result<std::unique_ptr<NamedReference>> NamedReferenceFromJson(
230230
const nlohmann::json& json) {
231-
if (json.is_object() && json.contains(kType) &&
232-
json[kType].get<std::string>() == kReference && json.contains(kTerm)) {
231+
if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
232+
json[kType].get<std::string>() == kReference && json.contains(kTerm) &&
233+
json[kTerm].is_string()) {
233234
return NamedReference::Make(json[kTerm].get<std::string>());
234235
}
235236
if (!json.is_string()) [[unlikely]] {
@@ -331,7 +332,7 @@ Result<int64_t> GetInt64Checked(const nlohmann::json& json) {
331332

332333
Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* type) {
333334
// If {"type": "literal", "value": <actual>} wrapper is present, unwrap it first.
334-
if (json.is_object() && json.contains(kType) &&
335+
if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
335336
json[kType].get<std::string>() == kLiteral && json.contains(kValue)) {
336337
return LiteralFromJson(json[kValue], type);
337338
}
@@ -499,7 +500,7 @@ Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* type) {
499500

500501
Result<Literal> LiteralFromJson(const nlohmann::json& json) {
501502
// Unwrap {"type": "literal", "value": <actual>} wrapper
502-
if (json.is_object() && json.contains(kType) &&
503+
if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
503504
json[kType].get<std::string>() == kLiteral && json.contains(kValue)) {
504505
return LiteralFromJson(json[kValue]);
505506
}
@@ -619,7 +620,7 @@ Result<std::shared_ptr<Expression>> ExpressionFromJson(const nlohmann::json& jso
619620
SafeDumpJson(json));
620621
}
621622

622-
if (json[kType].get<std::string>() == kLiteral) {
623+
if (json[kType].is_string() && json[kType].get<std::string>() == kLiteral) {
623624
if (!json.contains(kValue) || !json[kValue].is_boolean()) [[unlikely]] {
624625
return JsonParseError(
625626
"Expression of type 'literal' must have a boolean 'value' field: {}",

‎src/iceberg/test/expression_json_test.cc‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,4 +577,31 @@ INSTANTIATE_TEST_SUITE_P(
577577
return info.param.name;
578578
});
579579

580+
// A non-string "type" node must produce a parse error, not an uncaught
581+
// nlohmann type_error escaping the Result contract.
582+
TEST(ExpressionJsonTest, NonStringTypeIsParseError) {
583+
EXPECT_THAT(ExpressionFromJson(R"({"type": 42, "term": "a"})"_json),
584+
IsError(ErrorKind::kJsonParseError));
585+
EXPECT_THAT(ExpressionFromJson(R"({"type": 42, "left": true, "right": true})"_json),
586+
IsError(ErrorKind::kJsonParseError));
587+
EXPECT_THAT(LiteralFromJson(R"({"type": 42, "value": 1})"_json),
588+
IsError(ErrorKind::kJsonParseError));
589+
590+
// A reference wrapper with a non-string term is likewise a parse error.
591+
EXPECT_THAT(
592+
ExpressionFromJson(
593+
R"({"type":"eq","term":{"type":"reference","term":42},"value":1})"_json),
594+
IsError(ErrorKind::kJsonParseError));
595+
596+
// A non-string "type" on a predicate's term node, routed through the
597+
// transform-term check and then the named-reference parser.
598+
EXPECT_THAT(
599+
ExpressionFromJson(R"({"type":"eq","term":{"type":42,"term":"x"},"value":1})"_json),
600+
IsError(ErrorKind::kJsonParseError));
601+
602+
// A non-string "type" on the type-aware LiteralFromJson overload.
603+
EXPECT_THAT(LiteralFromJson(R"({"type":42,"value":1})"_json, int32().get()),
604+
IsError(ErrorKind::kJsonParseError));
605+
}
606+
580607
} // namespace iceberg

0 commit comments

Comments
 (0)