Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LuciferYang wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/iceberg/expression/json_serde.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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]] {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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]);
}
Expand Down Expand Up @@ -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: {}",
Expand Down
27 changes: 27 additions & 0 deletions src/iceberg/test/expression_json_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading