Skip to content

Commit

Permalink
fix(bigquery): Fixes parsing of null type values returned from BQ ser…
Browse files Browse the repository at this point in the history 
…ver (#14384)
  • Loading branch information
@jsrinnn
jsrinnn authored Jun 27, 2024
1 parent 964ef1f commit 113730d
Show file tree
Hide file tree
Showing 6 changed files with 165 additions and 33 deletions.
5 changes: 3 additions & 2 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,11 @@ void to_json(nlohmann::json& j, ColumnData const& c) {
j = nlohmann::json{{"v", c.value}};
}
void from_json(nlohmann::json const& j, ColumnData& c) {
SafeGetTo(c.value, j, "v");
SafeGetToWithNullable(c.value, c.is_null, j, "v");
}

bool operator==(ColumnData const& lhs, ColumnData const& rhs) {
return lhs.value == rhs.value;
return (lhs.value == rhs.value && lhs.is_null == rhs.is_null);
}

bool operator==(RowData const& lhs, RowData const& rhs) {
Expand All @@ -651,6 +651,7 @@ std::string ColumnData::DebugString(absl::string_view name,
int indent) const {
return internal::DebugFormatter(name, options, indent)
.StringField("value", value)
.Field("is_null", is_null)
.Build();
}

Expand Down
2 changes: 2 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,8 @@ bool operator==(Struct const& lhs, Struct const& rhs);

struct ColumnData {
std::string value;
// TODO(#14387): Use absl::optional instead.
bool is_null{false};
std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
int indent = 0) const;
Expand Down
86 changes: 71 additions & 15 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,25 @@ TEST(CommonV2ResourcesTest, ColumnDataFromJson) {

ColumnData expected;
expected.value = "12345";
expected.is_null = false;

EXPECT_EQ(expected.value, actual.value);
}

TEST(CommonV2ResourcesTest, ColumnDataFromJsonNull) {
std::string text =
R"({
"v":null
})";
auto json = nlohmann::json::parse(text, nullptr, false);
EXPECT_TRUE(json.is_object());

ColumnData actual;
from_json(json, actual);

ColumnData expected;
expected.value = "";
expected.is_null = true;

EXPECT_EQ(expected.value, actual.value);
}
Expand Down Expand Up @@ -510,69 +529,106 @@ TEST(CommonV2ResourcesTest, RowDataFromJson) {
actual.columns.begin()));
}

TEST(CommonV2ResourcesTest, RowDataFromJsonNullValues) {
std::string text =
R"({
"f":[
{
"v":null
},
{
"v":null
}
]
})";
auto json = nlohmann::json::parse(text, nullptr, false);
EXPECT_TRUE(json.is_object());

RowData actual;
from_json(json, actual);

RowData expected;
expected.columns.push_back(ColumnData{"", true});
expected.columns.push_back(ColumnData{"", true});

EXPECT_TRUE(std::equal(expected.columns.begin(), expected.columns.end(),
actual.columns.begin()));
}

TEST(CommonV2ResourcesTest, ColumnsDebugString) {
ColumnData column_data;
column_data.value = "12345";

EXPECT_EQ(column_data.DebugString("ColumnData", TracingOptions{}),
R"(ColumnData {)"
R"( value: "12345")"
R"( is_null: false)"
R"( })");

EXPECT_EQ(column_data.DebugString("ColumnData",
TracingOptions{}.SetOptions(
"truncate_string_field_longer_than=2")),
R"(ColumnData {)"
R"( value: "12...<truncated>...")"
R"( is_null: false)"
R"( })");

EXPECT_EQ(column_data.DebugString("ColumnData", TracingOptions{}.SetOptions(
"single_line_mode=F")),
R"(ColumnData {
value: "12345"
is_null: false
})");
}

TEST(CommonV2ResourcesTest, RowDataDebugString) {
RowData row_data = MakeRowData();

EXPECT_EQ(row_data.DebugString("RowData", TracingOptions{}),
R"(RowData { columns { value: "col1" })"
R"( columns { value: "col2" } columns {)"
R"( value: "col3" } columns {)"
R"( value: "col4" } columns {)"
R"( value: "col5" } columns { value: "col6" } })");

EXPECT_EQ(row_data.DebugString("RowData",
TracingOptions{}.SetOptions(
"truncate_string_field_longer_than=2")),
R"(RowData { columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." } })");
R"(RowData { columns { value: "col1" is_null: false })"
R"( columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false })"
R"( columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false })"
R"( columns { value: "col6" is_null: false } })");

EXPECT_EQ(
row_data.DebugString(
"RowData",
TracingOptions{}.SetOptions("truncate_string_field_longer_than=2")),
R"(RowData { columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false } })");

EXPECT_EQ(row_data.DebugString(
"RowData", TracingOptions{}.SetOptions("single_line_mode=F")),
R"(RowData {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
})");
}
Expand Down
36 changes: 24 additions & 12 deletions google/cloud/bigquery/v2/minimal/internal/job_response_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1996,9 +1996,9 @@ TEST(QueryResponseTest, DebugString) {
R"( kind: "query-kind" page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000 num_dml_affected_rows: 5)"
R"( job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2016,9 +2016,9 @@ TEST(QueryResponseTest, DebugString) {
R"( query_results { kind: "query-k...<truncated>...")"
R"( page_token: "np123" total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2045,21 +2045,27 @@ TEST(QueryResponseTest, DebugString) {
rows {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
}
schema {
Expand Down Expand Up @@ -2149,9 +2155,9 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( kind: "query-kind" etag: "query-etag" page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2168,9 +2174,9 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( etag: "query-e...<truncated>..." page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2197,21 +2203,27 @@ TEST(GetQueryResultsResponseTest, DebugString) {
rows {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
}
schema {
Expand Down
32 changes: 28 additions & 4 deletions google/cloud/bigquery/v2/minimal/internal/json_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ bool SafeGetTo(ResponseType& value, nlohmann::json const& j,
std::string const& key) {
auto i = j.find(key);
if (i != j.end()) {
i->get_to(value);
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
i->get_to(value);
}
return true;
}
return false;
Expand All @@ -70,10 +73,13 @@ bool SafeGetTo(std::shared_ptr<T>& value, nlohmann::json const& j,
std::string const& key) {
auto i = j.find(key);
if (i == j.end()) return false;
if (value == nullptr) {
value = std::make_shared<T>();
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
if (value == nullptr) {
value = std::make_shared<T>();
}
i->get_to(*value);
}
i->get_to(*value);
return true;
}

Expand All @@ -85,6 +91,24 @@ void SafeGetTo(nlohmann::json const& j, std::string const& key, R& (C::*f)(T) &,
(obj.*f)(i->get<T>());
}
}

// Same as SafeGetTo but also returns if value was null.
template <typename ResponseType>
bool SafeGetToWithNullable(ResponseType& value, bool& is_null,
nlohmann::json const& j, std::string const& key) {
auto i = j.find(key);
is_null = false;
if (i != j.end()) {
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
i->get_to(value);
} else {
is_null = true;
}
return true;
}
return false;
}
// NOLINTEND(misc-no-recursion)

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down
37 changes: 37 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/json_utils_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,43 @@ TEST(JsonUtilsTest, RemoveEmptyObjects) {
EXPECT_EQ(expected, json.dump());
}

TEST(JsonUtilsTest, SafeGetToNullValue) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":null})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
EXPECT_TRUE(SafeGetTo(val, json, key));
EXPECT_EQ(val, "");
}

TEST(JsonUtilsTest, SafeGetToWithNullableNullValue) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":null})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
bool is_null;
EXPECT_TRUE(SafeGetToWithNullable(val, is_null, json, key));
EXPECT_EQ(val, "");
EXPECT_TRUE(is_null);
}

TEST(JsonUtilsTest, SafeGetToWithNullableNonNull) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":"123"})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
bool is_null;
EXPECT_TRUE(SafeGetToWithNullable(val, is_null, json, key));
EXPECT_EQ(val, "123");
EXPECT_FALSE(is_null);
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace bigquery_v2_minimal_internal
} // namespace cloud
Expand Down

0 comments on commit 113730d

Please sign in to comment.