Skip to content

escape special chars in csv and json output. #802

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

Merged
dmah42 merged 5 commits into from
Apr 19, 2019
Merged

escape special chars in csv and json output. #802

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f56de0
escape special chars in csv and json output.
tesch1 Apr 16, 2019
2cb120a
functionalize.
tesch1 Apr 16, 2019
5db5aab
split string escape functions between csv and json
tesch1 Apr 18, 2019
807edf4
Update src/csv_reporter.cc
LebedevRI Apr 18, 2019
f3ceed0
Update src/json_reporter.cc
LebedevRI Apr 18, 2019
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
17 changes: 3 additions & 14 deletions src/csv_reporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,11 @@ void CSVReporter::ReportRuns(const std::vector<Run>& reports) {

void CSVReporter::PrintRunData(const Run& run) {
std::ostream& Out = GetOutputStream();

// Field with embedded double-quote characters must be doubled and the field
// delimited with double-quotes.
std::string name = run.benchmark_name();
ReplaceAll(&name, "\"", "\"\"");
Out << '"' << name << "\",";
Out << '"' << StrEscape(run.benchmark_name()) << "\",";
Copy link
Collaborator

@LebedevRI LebedevRI Apr 16, 2019

Choose a reason for hiding this comment

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

Can you please link where it's documented what should be escaped in CSV?
I'm probably looking in wrong places, https://tools.ietf.org/html/rfc4180 only says to replace " with ""
?

Copy link
Contributor Author

@tesch1 tesch1 Apr 16, 2019

Choose a reason for hiding this comment

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

is there even a standard for csv? at least \r and \n need to be escaped, \b and \f probably should be, \t is a matter of taste- the tests dont check for it in any case. and anyway, isn't csv support being deprecated #500 ?

Copy link
Collaborator

@LebedevRI LebedevRI Apr 16, 2019

Choose a reason for hiding this comment

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

at least \r and \n need to be escaped

I'm not seeing that in a quick test with loading

"test","my
test"

into libreoffice:
image

And replacing \n with "\n":

"test","my\ntest"

breaks it:
image
Thus yes, i'm curious as to motivation/documentation.

and anyway, isn't csv support being deprecated #500 ?

Yep

Copy link
Contributor Author

@tesch1 tesch1 Apr 16, 2019

Choose a reason for hiding this comment

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

you're right, \t and \ should proabaly not be escaped for csv. the other ones I think the "right thing" is to escape them. \b because if you're lookign at csv, you typically want to see it on a terminal, and any \b would end up hidden, and they're more often than not in there by mistake.

Copy link
Collaborator

@LebedevRI LebedevRI Apr 16, 2019

Choose a reason for hiding this comment

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

I agree that JSON changes are good.
The issue does not mention anything about CSV; are you affected by CSV side of things?

How about a middle ground solution then?

  1. Click apply on that reserve() suggestion.
  2. Drop CSV changes, thus avoiding all the questions here as to what should and should not be escaped.

if (run.error_occurred) {
Out << std::string(elements.size() - 3, ',');
Out << "true,";
std::string msg = run.error_message;
ReplaceAll(&msg, "\"", "\"\"");
Out << '"' << msg << "\"\n";
Out << '"' << StrEscape(run.error_message) << "\"\n";
return;
}

Expand Down Expand Up @@ -130,11 +123,7 @@ void CSVReporter::PrintRunData(const Run& run) {
}
Out << ",";
if (!run.report_label.empty()) {
// Field with embedded double-quote characters must be doubled and the field
// delimited with double-quotes.
std::string label = run.report_label;
ReplaceAll(&label, "\"", "\"\"");
Out << "\"" << label << "\"";
Out << "\"" << StrEscape(run.report_label) << "\"";
}
Out << ",,"; // for error_occurred and error_message

Expand Down
19 changes: 7 additions & 12 deletions src/json_reporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,29 +33,29 @@ namespace benchmark {
namespace {

std::string FormatKV(std::string const& key, std::string const& value) {
return StrFormat("\"%s\": \"%s\"", key.c_str(), value.c_str());
return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, const char* value) {
return StrFormat("\"%s\": \"%s\"", key.c_str(), value);
return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, bool value) {
return StrFormat("\"%s\": %s", key.c_str(), value ? "true" : "false");
return StrFormat("\"%s\": %s", StrEscape(key).c_str(), value ? "true" : "false");
}

std::string FormatKV(std::string const& key, int64_t value) {
std::stringstream ss;
ss << '"' << key << "\": " << value;
ss << '"' << StrEscape(key) << "\": " << value;
return ss.str();
}

std::string FormatKV(std::string const& key, double value) {
std::stringstream ss;
ss << '"' << key << "\": ";
ss << '"' << StrEscape(key) << "\": ";

if (std::isnan(value))
ss << "NaN";
ss << (value < 0 ? "-" : "") << "NaN";
else if (std::isinf(value))
ss << (value < 0 ? "-" : "") << "Infinity";
else {
Expand Down Expand Up @@ -88,12 +88,7 @@ bool JSONReporter::ReportContext(const Context& context) {
out << indent << FormatKV("host_name", context.sys_info.name) << ",\n";

if (Context::executable_name) {
// windows uses backslash for its path separator,
// which must be escaped in JSON otherwise it blows up conforming JSON
// decoders
std::string executable_name = Context::executable_name;
ReplaceAll(&executable_name, "\\", "\\\\");
out << indent << FormatKV("executable", executable_name) << ",\n";
out << indent << FormatKV("executable", Context::executable_name) << ",\n";
}

CPUInfo const& info = context.cpu_info;
Expand Down
18 changes: 11 additions & 7 deletions src/string_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,17 @@ std::string StrFormat(const char* format, ...) {
return tmp;
}

void ReplaceAll(std::string* str, const std::string& from,
const std::string& to) {
std::size_t start = 0;
while ((start = str->find(from, start)) != std::string::npos) {
str->replace(start, from.length(), to);
start += to.length();
}
StrEscape::StrEscape(const std::string & s) {
std::for_each(s.begin(), s.end(), [this](char c) {
switch (c) {
case '\b': (*this) += "\\b"; break;
case '\f': (*this) += "\\f"; break;
case '\n': (*this) += "\\n"; break;
case '\r': (*this) += "\\r"; break;
case '\t': (*this) += "\\t"; break;
case '\\': (*this) += "\\\\"; break;
case '"' : (*this) += "\\\""; break;
default : (*this) += c; break; } } );
}

#ifdef BENCHMARK_STL_ANDROID_GNUSTL
Expand Down
5 changes: 3 additions & 2 deletions src/string_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ inline std::string StrCat(Args&&... args) {
return ss.str();
}

void ReplaceAll(std::string* str, const std::string& from,
const std::string& to);
struct StrEscape : std::string {
Copy link
Collaborator

@LebedevRI LebedevRI Apr 16, 2019

Choose a reason for hiding this comment

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

Uhm.
/me doesn't like

Copy link
Member

@dmah42 dmah42 Apr 16, 2019

Choose a reason for hiding this comment

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

ooh i didn't spot that. yeah, that should likely just be a method that takes a string and returns a string.

Copy link
Contributor Author

@tesch1 tesch1 Apr 16, 2019

Choose a reason for hiding this comment

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

/what /does /you /prefer /?

Copy link
Contributor Author

@tesch1 tesch1 Apr 16, 2019

Choose a reason for hiding this comment

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

how about this...

StrEscape(const std::string & s);
};

#ifdef BENCHMARK_STL_ANDROID_GNUSTL
/*
Expand Down
20 changes: 20 additions & 0 deletions test/reporter_output_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,26 @@ ADD_CASES(
"manual_time_stddev\",%csv_report$"},
{"^\"BM_UserStats/iterations:5/repeats:3/manual_time_\",%csv_report$"}});

// ========================================================================= //
// ------------------------- Testing StrEscape ----------------------------- //
// ========================================================================= //

void BM_JSON_Format(benchmark::State& state) {
state.SkipWithError("val\b\f\n\r\t\\\"with\"escapes");
for (auto _ : state) {
}
}
BENCHMARK(BM_JSON_Format);
ADD_CASES(TC_JSONOut, {{"\"name\": \"BM_JSON_Format\",$"},
{"\"run_name\": \"BM_JSON_Format\",$", MR_Next},
{"\"run_type\": \"iteration\",$", MR_Next},
{"\"repetitions\": 0,$", MR_Next},
{"\"repetition_index\": 0,$", MR_Next},
{"\"threads\": 1,$", MR_Next},
{"\"error_occurred\": true,$", MR_Next},
{R"("error_message": "val\\b\\f\\n\\r\\t\\\\\\"with\\"escapes",$)", MR_Next}});
ADD_CASES(TC_CSVOut, {{R"(^"BM_JSON_Format",,,,,,,,true,"val\\b\\f\\n\\r\\t\\\\\\"with\\"escapes"$)"}});

// ========================================================================= //
// --------------------------- TEST CASES END ------------------------------ //
// ========================================================================= //
Expand Down