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 2 commits
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
20 changes: 13 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,19 @@ 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();
}
std::string StrEscape(const std::string & s) {
std::string tmp;
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.

tmp.reserve(s.size());

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.

feel free to take over the PR

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 mean, it's your PR.

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.

my use case isn't going to notice the 2 ns (maybe?) speed improvement, if you need it feel free to add it though.

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.

It's a review comment, it is good to address them, so that there is forward progress on proposed changes.
Not sure how these github suggestions work:

Suggested change
std::string tmp;
std::string tmp;
tmp.reserve(s.size());

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 do realize that std::string is already designed for this kind of concatenation? this is trading 0-1-2 reallocs (depending on the length of the key) with a guaranteed two calls (strlen,realloc), not sure if there's a real win here, unless it's an issue of coding style?

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.

We know the escaped string will be at least not shorter than the input string.
clang/gcc will not magically insert that pre-allocation.
Therefore, if we don't do it explicitly, we are relying on the sane behavior
of the std::string implementation in the C++ std library that is used.

  • We will likely always have some allocs if the string after escaping does not fit within small-size-optimization.
  • Without pre-allocation, if the source string does not fit within small-size-optimization,
    and there won't be extra escape symbols, we will have allocations.
  • WITH pre-allocation, if the source string does not fit within small-size-optimization,
    and there won't be extra escape symbols, we will NOT have allocations. WIN.

It is well-known that reserve() is good, and it is quite obvious it won't hurt here.
I do not understand your point.

std::for_each(s.begin(), s.end(), [&tmp](char c) {
switch (c) {
case '\b': tmp += "\\b"; break;
case '\f': tmp += "\\f"; break;
case '\n': tmp += "\\n"; break;
case '\r': tmp += "\\r"; break;
case '\t': tmp += "\\t"; break;
case '\\': tmp += "\\\\"; break;
case '"' : tmp += "\\\""; break;
default : tmp += c; break; } } );
return tmp;
}

#ifdef BENCHMARK_STL_ANDROID_GNUSTL
Expand Down
3 changes: 1 addition & 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,7 @@ inline std::string StrCat(Args&&... args) {
return ss.str();
}

void ReplaceAll(std::string* str, const std::string& from,
const std::string& to);
std::string 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