(iceberg-cpp) branch main updated: fix(expression): return a parse error for non-string type nodes in expression JSON (#982)

[gangwu]

This is an automated email from the ASF dual-hosted git repository.

wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git


The following commit(s) were added to refs/heads/main by this push:
     new ecf36247 fix(expression): return a parse error for non-string type 
nodes in expression JSON (#982)
ecf36247 is described below

commit ecf36247c6abbfaee591a0c89f8c5055344e3a1b
Author: YangJie <yangji...@baidu.com>
AuthorDate: Sun Oct 11 09:16:30 2026 -0400

    fix(expression): return a parse error for non-string type nodes in 
expression JSON (#982)
    
    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.
---
 src/iceberg/expression/json_serde.cc     | 13 +++++++------
 src/iceberg/test/expression_json_test.cc | 27 +++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/src/iceberg/expression/json_serde.cc 
b/src/iceberg/expression/json_serde.cc
index 1afed021..59f3fdcc 100644
--- a/src/iceberg/expression/json_serde.cc
+++ b/src/iceberg/expression/json_serde.cc
@@ -86,7 +86,7 @@ nlohmann::json MakeTransformJson(std::string_view 
transform_str,
 
 /// Helper to check if a JSON term represents a transform
 bool IsTransformTerm(const nlohmann::json& json) {
-  return json.is_object() && json.contains(kType) &&
+  return json.is_object() && json.contains(kType) && json[kType].is_string() &&
          json[kType].get<std::string>() == kTransform && json.contains(kTerm);
 }
 
@@ -228,8 +228,9 @@ nlohmann::json ToJson(const BoundTransform& transform) {
 
 Result<std::unique_ptr<NamedReference>> NamedReferenceFromJson(
     const nlohmann::json& json) {
-  if (json.is_object() && json.contains(kType) &&
-      json[kType].get<std::string>() == kReference && json.contains(kTerm)) {
+  if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
+      json[kType].get<std::string>() == kReference && json.contains(kTerm) &&
+      json[kTerm].is_string()) {
     return NamedReference::Make(json[kTerm].get<std::string>());
   }
   if (!json.is_string()) [[unlikely]] {
@@ -331,7 +332,7 @@ Result<int64_t> GetInt64Checked(const nlohmann::json& json) 
{
 
 Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* type) {
   // If {"type": "literal", "value": <actual>} wrapper is present, unwrap it 
first.
-  if (json.is_object() && json.contains(kType) &&
+  if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
       json[kType].get<std::string>() == kLiteral && json.contains(kValue)) {
     return LiteralFromJson(json[kValue], type);
   }
@@ -499,7 +500,7 @@ Result<Literal> LiteralFromJson(const nlohmann::json& json, 
const Type* type) {
 
 Result<Literal> LiteralFromJson(const nlohmann::json& json) {
   // Unwrap {"type": "literal", "value": <actual>} wrapper
-  if (json.is_object() && json.contains(kType) &&
+  if (json.is_object() && json.contains(kType) && json[kType].is_string() &&
       json[kType].get<std::string>() == kLiteral && json.contains(kValue)) {
     return LiteralFromJson(json[kValue]);
   }
@@ -619,7 +620,7 @@ Result<std::shared_ptr<Expression>> 
ExpressionFromJson(const nlohmann::json& jso
                           SafeDumpJson(json));
   }
 
-  if (json[kType].get<std::string>() == kLiteral) {
+  if (json[kType].is_string() && json[kType].get<std::string>() == kLiteral) {
     if (!json.contains(kValue) || !json[kValue].is_boolean()) [[unlikely]] {
       return JsonParseError(
           "Expression of type 'literal' must have a boolean 'value' field: {}",
diff --git a/src/iceberg/test/expression_json_test.cc 
b/src/iceberg/test/expression_json_test.cc
index 1c993cc1..1afecc71 100644
--- a/src/iceberg/test/expression_json_test.cc
+++ b/src/iceberg/test/expression_json_test.cc
@@ -577,4 +577,31 @@ INSTANTIATE_TEST_SUITE_P(
       return info.param.name;
     });
 
+// A non-string "type" node must produce a parse error, not an uncaught
+// nlohmann type_error escaping the Result contract.
+TEST(ExpressionJsonTest, NonStringTypeIsParseError) {
+  EXPECT_THAT(ExpressionFromJson(R"({"type": 42, "term": "a"})"_json),
+              IsError(ErrorKind::kJsonParseError));
+  EXPECT_THAT(ExpressionFromJson(R"({"type": 42, "left": true, "right": 
true})"_json),
+              IsError(ErrorKind::kJsonParseError));
+  EXPECT_THAT(LiteralFromJson(R"({"type": 42, "value": 1})"_json),
+              IsError(ErrorKind::kJsonParseError));
+
+  // A reference wrapper with a non-string term is likewise a parse error.
+  EXPECT_THAT(
+      ExpressionFromJson(
+          
R"({"type":"eq","term":{"type":"reference","term":42},"value":1})"_json),
+      IsError(ErrorKind::kJsonParseError));
+
+  // A non-string "type" on a predicate's term node, routed through the
+  // transform-term check and then the named-reference parser.
+  EXPECT_THAT(
+      
ExpressionFromJson(R"({"type":"eq","term":{"type":42,"term":"x"},"value":1})"_json),
+      IsError(ErrorKind::kJsonParseError));
+
+  // A non-string "type" on the type-aware LiteralFromJson overload.
+  EXPECT_THAT(LiteralFromJson(R"({"type":42,"value":1})"_json, int32().get()),
+              IsError(ErrorKind::kJsonParseError));
+}
+
 }  // namespace iceberg