Skip to content

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

Open
LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:fix/sweep-8-expr-json-uncaught-exceptions
Open

LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:fix/sweep-8-expr-json-uncaught-exceptions

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

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 AI balanced review requested due to automatic review settings October 1, 2026 11:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: expression JSON deserialization throws an uncaught exception on a non-string "type"/"term"

3 participants