[EXPORTER] fix clang-tidy warnings in UrlParser

[sjinks]

Changes

This PR fixes issues found by clang-tidy in UrlParser.

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	40da6ba
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673cf58cdbb9a7000850c155

[codecov]

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.86%. Comparing base (497eaf4) to head (e59a4b1).
Report is 169 commits behind head on main.

Files with missing lines	Patch %	Lines
...include/opentelemetry/ext/http/common/url_parser.h	93.94%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3146      +/-   ##
==========================================
+ Coverage   87.12%   87.86%   +0.74%     
==========================================
  Files         200      195       -5     
  Lines        6109     6151      +42     
==========================================
+ Hits         5322     5404      +82     
+ Misses        787      747      -40     

Files with missing lines	Coverage Δ
...include/opentelemetry/ext/http/common/url_parser.h	94.74% <93.94%> (+2.74%)	⬆️

... and 112 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[sjinks]

Here and below: strictly speaking, it should be (cpos, url_.length - cpos) but because we want all remaining characters, we can save a few CPU cycles by not subtracting cpos.

[marcalff]

Even simpler, when we want all the remaining substring:

path_ = url_.substr(cpos);

https://en.cppreference.com/w/cpp/string/basic_string/substr

(e.g. if count == npos), the returned substring is [pos, size())

[sjinks]

Updated the code.

[marcalff]

Thanks, much cleaner. Waiting for CI to complete, then merging.

[sjinks]

I also tried to optimize UrlDecoder::Decode() a bit: https://godbolt.org/z/qnv1oqhcn (Decode2 is the original version, Decode is the first attempt, Decode3 is the final code)

[sjinks]

@marcalff a bit of offtopic: I would like to make tests more readable by using parametrized tests. Among other things, this will show which of the test data failed.

Something like this:

class UrlDecoderTests : public ::testing::TestWithParam<std::tuple<std::string, std::string>>
{};

INSTANTIATE_TEST_SUITE_P(
    SampleValues,
    UrlDecoderTests,
    testing::Values(std::make_tuple("Authentication=Basic xxx", "Authentication=Basic xxx"),
                    std::make_tuple("Authentication=Basic%20xxx", "Authentication=Basic xxx"),
                    std::make_tuple("%C3%B6%C3%A0%C2%A7%C3%96abcd%C3%84",
                                    "\xc3\xb6\xc3\xa0\xc2\xa7\xc3\x96\x61\x62\x63\x64\xc3\x84"),
                    std::make_tuple("%2x", "%2x"),
                    std::make_tuple("%20", " "),
                    std::make_tuple("text%2", "text%2")));

TEST_P(UrlDecoderTests, BasicTests)
{
  const auto encoded  = std::get<0>(GetParam());
  const auto expected = std::get<1>(GetParam());
  const auto actual   = http_common::UrlDecoder::Decode(encoded);

  EXPECT_EQ(actual, expected);
}

instead of

TEST(UrlDecoderTests, BasicTests)
{
  std::map<std::string, std::string> testdata{
      {"Authentication=Basic xxx", "Authentication=Basic xxx"},
      {"Authentication=Basic%20xxx", "Authentication=Basic xxx"},
      {"%C3%B6%C3%A0%C2%A7%C3%96abcd%C3%84",
       "\xc3\xb6\xc3\xa0\xc2\xa7\xc3\x96\x61\x62\x63\x64\xc3\x84"},
      {"%2x", "%2x"},
      {"%20", " "},
      {"text%2", "text%2"},
  };

  for (auto &testsample : testdata)
  {
    ASSERT_EQ(http_common::UrlDecoder::Decode(testsample.first), testsample.second);
    ASSERT_TRUE(http_common::UrlDecoder::Decode(testsample.first) == testsample.second);
  }
}

The output will look like this (I intentionally made the test fail):

[----------] 6 tests from SampleValues/UrlDecoderTests
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/0
[       OK ] SampleValues/UrlDecoderTests.BasicTests/0 (0 ms)
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/1
[       OK ] SampleValues/UrlDecoderTests.BasicTests/1 (0 ms)
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/2
[       OK ] SampleValues/UrlDecoderTests.BasicTests/2 (0 ms)
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/3
[       OK ] SampleValues/UrlDecoderTests.BasicTests/3 (0 ms)
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/4
/home/volodymyr/work/github/opentelemetry-cpp/ext/test/http/url_parser_test.cc:252: Failure
Expected equality of these values:
  actual
    Which is: " "
  expected
    Which is: "-"

[  FAILED  ] SampleValues/UrlDecoderTests.BasicTests/4, where GetParam() = ("%20", "-") (0 ms)
[ RUN      ] SampleValues/UrlDecoderTests.BasicTests/5
[       OK ] SampleValues/UrlDecoderTests.BasicTests/5 (0 ms)
[----------] 6 tests from SampleValues/UrlDecoderTests (0 ms total)

[----------] Global test environment tear-down
[==========] 7 tests from 2 test suites ran. (0 ms total)
[  PASSED  ] 6 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SampleValues/UrlDecoderTests.BasicTests/4, where GetParam() = ("%20", "-")

See http://google.github.io/googletest/advanced.html#value-parameterized-tests

Is this something you would accept as a PR?

[marcalff]

@marcalff a bit of offtopic: I would like to make tests more readable by using parametrized tests. Among other things, this will show which of the test data failed.

(...)

Is this something you would accept as a PR?

For a new test case added, I would definitively take it, this is good code.

For a PR that refactors test cases alone, say to investigate a hard to reproduce spurious failure to pinpoint which case is failing exactly, I would also take it.

The problem here is that we can not have refactoring both of production code (the url parser) and of unit tests for it, at the same time in the same PR.

This introduces the risk of adding a bug that goes undetected, even when admittedly the test case refactoring looks clean, so the probability of a bug in the unit test alone hiding a bug in the production code is very low.

That being said, I like the refactored test, as it will show how to write parameterized tests in opentelemetry-cpp in general, serving as an example for others, so I think it is important to have it.

As long as the unit test is refactored in a different PR (I think this is what you meant), I will (gladly) take it.
While at it, please add a test case for Invalid input: no valid hex characters after '%', so the current behavior is captured and covered.

The test PR can be reviewed and merged immediately.

[marcalff]

I also tried to optimize UrlDecoder::Decode() a bit: https://godbolt.org/z/qnv1oqhcn (Decode2 is the original version, Decode is the first attempt, Decode3 is the final code)

The URL parser is used once at program startup, to parse the configuration of a OTLP exporter for example, so performance is hardly a concern, as this is not in any critical path.

The refactored code is also much cleaner, so even if this is a bit more than just fixing a clang-tidy warning, it is a good change to have (TLDR: approved).

[sjinks]

While at it, please add a test case for Invalid input: no valid hex characters after '%', so the current behavior is captured and covered.

Done: this one is a test for invalid hex chars; this one is for the case when there are less than two chars available.

[marcalff]

LGTM, thanks for the cleanup