Sandbox::https://github.com/apache/iceberg-cpp/pull/982

LuciferYang · 2026-10-01T11:30:56Z

What

Expression JSON deserialization in src/iceberg/expression/json_serde.cc called json[kType].get<std::string>() (and one json[kTerm].get<std::string>()) guarded only by is_object() and contains(kType). A non-string "type"/"term" made nlohmann throw json::type_error.302, which escaped these Result-returning functions (the expression parse chain has no try/catch) and terminated the caller instead of returning an error. This is the same failure mode as the merged #857.

It is reachable, not test-only. Table-metadata parsing hits the type-aware LiteralFromJson through FieldFromJson's initial-default/write-default handling, and REST responses hit the expression parsers through the scan-metrics report filter and the residual/partition/plan filters, all via ICEBERG_ASSIGN_OR_RAISE, which forwards a Result error but not a thrown exception.

Closes #979.

How

Each of the six get<std::string>() sites now checks is_string() first and returns JsonParseError on a non-string node, mirroring the sibling OperationTypeFromJson. Valid string input is unaffected: the added is_string() sits in a short-circuit &&, so a well-formed node evaluates exactly as before, and the guard only rejects input that previously threw.

Testing

NonStringTypeIsParseError in expression_json_test.cc covers all six guards: a non-string "type" at the top level (ExpressionFromJson), on an and/or node, on a predicate's term node (routed through the transform-term check and then the named-reference parser), a non-string reference "term", and a non-string "type" on both the untyped and the type-aware LiteralFromJson overloads. Each case returns kJsonParseError; without the fix the corresponding input throws json::type_error.302 out of the Result-returning function. Removing any single guard makes one of these cases throw, so no guard is left unpinned.

… 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.

Copilot

Copilot review overview

🟢 Approval recommended

The guards comprehensively address the exception paths with focused regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Prevents malformed expression JSON from throwing exceptions outside the Result contract.

Changes:

Validates "type" and "term" nodes before string conversion.
Adds regression coverage for all six affected parsing paths.

File	Description
`src/iceberg/expression/json_serde.cc`	Guards string conversions and returns parse errors.
`src/iceberg/test/expression_json_test.cc`	Tests non-string discriminator handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

advancedxy

LGTM.

Agent alerted me there's one other similar failure issue during the code review:

iceberg-cpp/src/iceberg/json_serde.cc

Lines 496 to 505 in b4eecba

    
           Result<std::unique_ptr<Type>> ListTypeFromJson(const nlohmann::json& json) { 
        
             ICEBERG_ASSIGN_OR_RAISE(auto element_type, TypeFromJson(json[kElement])); 
        
             ICEBERG_ASSIGN_OR_RAISE(auto element_id, GetJsonValue<int32_t>(json, kElementId)); 
        
             ICEBERG_ASSIGN_OR_RAISE(auto element_required, 
        
                                     GetJsonValue<bool>(json, kElementRequired)); 
        
             ICEBERG_ASSIGN_OR_RAISE(auto type, ListType::Make(SchemaField( 
        
                                                    element_id, std::string(ListType::kElementName), 
        
                                                    std::move(element_type), !element_required))); 
        
             return std::unique_ptr<Type>(std::move(type));

's TypeFromJson accesses json[kElement] directly without check its existence, which also triggers SIGABRT. It should follow the MapTypeFromJson's practice. would you mind to submit a follow-up pr as well?

LuciferYang · 2026-10-10T13:09:54Z

LGTM.

Agent alerted me there's one other similar failure issue during the code review:

iceberg-cpp/src/iceberg/json_serde.cc

Lines 496 to 505 in b4eecba

Result<std::unique_ptr<Type>> ListTypeFromJson(const nlohmann::json& json) {

ICEBERG_ASSIGN_OR_RAISE(auto element_type, TypeFromJson(json[kElement]));

ICEBERG_ASSIGN_OR_RAISE(auto element_id, GetJsonValue<int32_t>(json, kElementId));

ICEBERG_ASSIGN_OR_RAISE(auto element_required,

GetJsonValue<bool>(json, kElementRequired));

ICEBERG_ASSIGN_OR_RAISE(auto type, ListType::Make(SchemaField(

element_id, std::string(ListType::kElementName),

std::move(element_type), !element_required)));

return std::unique_ptr<Type>(std::move(type));

's TypeFromJson accesses json[kElement] directly without check its existence, which also triggers SIGABRT. It should follow the MapTypeFromJson's practice. would you mind to submit a follow-up pr as well?

Yes, will fix in a separate PR.

LuciferYang · 2026-10-10T14:00:53Z

@advancedxy Opened #997 for ListTypeFromJson. I went through the other const json[key] reads under src/iceberg and that was the only one that didn't check the key exists first.

Copilot AI balanced review requested due to automatic review settings October 1, 2026 11:30

Copilot started reviewing on behalf of LuciferYang October 1, 2026 11:31 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

kamcheungting-db approved these changes Oct 4, 2026

View reviewed changes

advancedxy reviewed Oct 10, 2026

View reviewed changes

This was referenced Oct 10, 2026

bug: ListTypeFromJson hits undefined behavior when a list type has no "element" #996

Open

fix(json): return a parse error for a list type missing 'element' #997

Open

Navigation Menu

Repository navigation

fix(expression): return a parse error for non-string type nodes in expression JSON - #982

fix(expression): return a parse error for non-string type nodes in expression JSON#982
LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:fix/sweep-8-expr-json-uncaught-exceptions

LuciferYang commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

advancedxy left a comment

Uh oh!

LuciferYang commented Oct 10, 2026

Uh oh!

LuciferYang commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Result<std::unique_ptr<Type>> ListTypeFromJson(const nlohmann::json& json) {
	ICEBERG_ASSIGN_OR_RAISE(auto element_type, TypeFromJson(json[kElement]));
	ICEBERG_ASSIGN_OR_RAISE(auto element_id, GetJsonValue<int32_t>(json, kElementId));
	ICEBERG_ASSIGN_OR_RAISE(auto element_required,
	GetJsonValue<bool>(json, kElementRequired));

	ICEBERG_ASSIGN_OR_RAISE(auto type, ListType::Make(SchemaField(
	element_id, std::string(ListType::kElementName),
	std::move(element_type), !element_required)));
	return std::unique_ptr<Type>(std::move(type));

Conversation

LuciferYang commented Oct 1, 2026

What

How

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

advancedxy left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Oct 10, 2026

Uh oh!

LuciferYang commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants