Skip to content

fix: edge cases parsing of DD_TAGS #233

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
dmehala merged 1 commit into from
Sep 3, 2025
Merged

fix: edge cases parsing of DD_TAGS #233

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
8 changes: 3 additions & 5 deletions src/datadog/parse_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,10 @@ Expected<std::unordered_map<std::string, std::string>> parse_tags(

if (separator == std::string::npos) {
key = std::string{trim(token)};
if (key.empty()) continue;
} else {
key = std::string{trim(token.substr(0, separator))};
if (key.empty()) {
continue;
}
if (key.empty()) continue;
value = std::string{trim(token.substr(separator + 1))};
Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

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

Could it be interesting to ignore the key with no value ?

Copy link
Copy Markdown
Collaborator Author

@dmehala dmehala Sep 3, 2025

Choose a reason for hiding this comment

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

that's done in parse_tags l.210

Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

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

Sorry my comment was obviously wrong, to ignore the value with empty value*

}

Expand Down Expand Up @@ -183,15 +182,14 @@ Expected<std::unordered_map<std::string, std::string>> parse_tags(
break;
} else if (input[i] == ' ') {
separator = ' ';
break;
}
}

if (separator == 0) {
goto capture_all;
}

for (; i < end; ++i) {
for (i = beg; i < end; ++i) {
Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

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

Can we put i = 0 instead ? I'm almost sure that beg is 0 at this time and it makes the code a slightly more readable

Copy link
Copy Markdown
Collaborator Author

@dmehala dmehala Sep 3, 2025

Choose a reason for hiding this comment

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

Make sens. Unfortunately, this PR has been opened for too long, and revisiting it for a nit is not a priority. However, please don’t hesitate to open a new PR if you'd like to revisit or continue the work

if (input[i] == separator) {
auto tag = input.substr(beg, i - beg);
if (tag.size() > 0) {
Expand Down
140 changes: 80 additions & 60 deletions test/test_parse_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,56 +145,72 @@ PARSE_UTIL_TEST("parse_tags") {
};

auto test_case = GENERATE(values<TestCase>({
{
__LINE__,
"space separated tags",
"env:test aKey:aVal bKey:bVal cKey:",
{
{"env", "test"},
{"aKey", "aVal"},
{"bKey", "bVal"},
{"cKey", ""},
},
},
{
__LINE__,
"comma separated tags",
"env:test,aKey:aVal,bKey:bVal,cKey:",
{
{"env", "test"},
{"aKey", "aVal"},
{"bKey", "bVal"},
{"cKey", ""},
},
},
{
__LINE__,
"mixed separator 1/3",
"env:test,aKey:aVal bKey:bVal cKey:",
{
{"env", "test"},
{"aKey", "aVal bKey:bVal cKey:"},
},
},
{
__LINE__,
"mixed separator 2/3",
"env:test bKey :bVal, dKey: dVal cKey:",
{
{"env", "test bKey :bVal"},
{"dKey", "dVal cKey:"},
},
},
{
__LINE__,
"mixed separator 3/3",
"env :test, aKey : aVal bKey:bVal cKey:",
{
{"env", "test"},
{"aKey", "aVal bKey:bVal cKey:"},
},
},
{
__LINE__,
"multiple semi-colons",
"env:keyWithA:Semicolon bKey:bVal cKey",
{
{"env", "keyWithA:Semicolon"},
{"bKey", "bVal"},
{"cKey", ""},
},
},
{__LINE__,
"space separated tags",
"env:test aKey:aVal bKey:bVal cKey:",
"mixed separator edge case",
"env:keyWith: , , Lots:Of:Semicolons ",
{
{"env", "test"},
{"aKey", "aVal"},
{"bKey", "bVal"},
{"cKey", ""},
{"env", "keyWith:"},
{"Lots", "Of:Semicolons"},
}},
{__LINE__,
"comma separated tags",
"env:test aKey:aVal bKey:bVal cKey:",
{
{"env", "test"},
{"aKey", "aVal"},
{"bKey", "bVal"},
{"cKey", ""},
}},
{__LINE__,
"mixed separator but comma first",
"env:test,aKey:aVal bKey:bVal cKey:",
{
{"env", "test"},
{"aKey", "aVal bKey:bVal cKey:"},
}},
{__LINE__,
"mixed separator but space first",
"env:test bKey :bVal dKey: dVal cKey:",
{
{"env", "test"},
{"bKey", ""},
{"dKey", ""},
{"dVal", ""},
{"cKey", ""},
}},
{__LINE__,
"mixed separator but space first",
"env:keyWithA:Semicolon bKey:bVal cKey",
{
{"env", "keyWithA:Semicolon"},
{"bKey", "bVal"},
{"cKey", ""},
}},
// {__LINE__,
// "mixed separator edge case",
// "env:keyWith: , , Lots:Of:Semicolons ",
// {
// {"env", "keyWith:"},
// {"Lots", "Of:Semicolons"},
// }},
{__LINE__,
"comma separated but some tags without value",
"a:b,c,d",
Expand All @@ -210,19 +226,23 @@ PARSE_UTIL_TEST("parse_tags") {
{"a", ""},
{"1", ""},
}},
{__LINE__,
"no separator",
"a:b:c:d",
{
{"a", "b:c:d"},
}},
{__LINE__,
"input is trimed",
"key1:val1, key2 : val2 ",
{
{"key1", "val1"},
{"key2", "val2"},
}},
{
__LINE__,
"no separator",
"a:b:c:d",
{
{"a", "b:c:d"},
},
},
{
__LINE__,
"input is trimed",
"key1:val1, key2 : val2 ",
{
{"key1", "val1"},
{"key2", "val2"},
},
},
}));

CAPTURE(test_case.line);
Expand Down