Change CommandParser::parse to return StatusOr by yanavlasov · Pull Request #45752 · envoyproxy/envoy

yanavlasov · 2026-06-22T18:30:22Z

Change CommandParser::parse return value to StatusOr<>. This is preparation for removing exceptions from this method. Presently no parse overrides return error status and C++ exceptions still in use.

Risk Level: low, noop
Testing: unit tests
Docs Changes: no
Release Notes: no
Platform Specific Features: no

Signed-off-by: Yan Avlasov <yavlasov@google.com>

kyessenov

Thanks, this is a good change!

botengyao

could you fix the CI?

/wait

Signed-off-by: yavlasov <yavlasov@google.com>

ravenblackx · 2026-06-23T19:33:23Z

   */
-  virtual FormatterProviderPtr parse(absl::string_view command, absl::string_view command_arg,
-                                     absl::optional<size_t> max_length) const PURE;
+  virtual FormatterProviderResult parse(absl::string_view command, absl::string_view command_arg,


I think this would be more readable as a straight absl::StatusOr<FormatterProviderPtr> return type. At the call-sites using FormatterProviderResult sounds like a result from a FormatterProvider, not a result that may be a FormatterProvider.

Alternatively, MaybeFormatterProvider or FormatterProviderOrStatus would also work, but just doing it straight with the StatusOr and FormatterProviderPtr helper to avoid double-nesting seems simple and self-explanatory.

ravenblackx · 2026-06-23T19:36:16Z

  // Try built-in command parsers to create the formatter.
  for (const auto& parser : BuiltInCommandParserFactoryHelper::commandParsers()) {
-    auto formatter = parser->parse(command, param, max_length);
+    auto formatter_result = parser->parse(command, param, max_length);


I usually get berated for auto and I think this one is a good case for not doing it since it's again not clear without reading more of the function for context what a formatter_result is.
(The auto below is fine since it's explained by this one being a StatusOr)

Fixed, including other places

ravenblackx · 2026-06-23T19:36:52Z

+    auto formatter = std::move(formatter_result).value();
    if (formatter != nullptr) {
      return formatter;
    }


Should this be just ASSERT that it's not nullptr, or is there a valid case where a non-error-status is returned, but also no pointer?

Currently, it can return nullptr. Not sure if this is an error or not. I will be looking into these case then removing exceptions from parse overrides.

ravenblackx · 2026-06-23T19:41:26Z

+  auto provider = commands[0]->parse("COMMAND_EXTENSION", "", max_length).value();
  ASSERT_TRUE(provider != nullptr);


That's a test-crash in the case of parse returning an error status.

Recommend instead

Suggested change

auto provider = commands[0]->parse("COMMAND_EXTENSION", "", max_length).value();

ASSERT_TRUE(provider != nullptr);

auto status_or_provider = commands[0]->parse("COMMAND_EXTENSION", "", max_length);

ASSERT_THAT(status_or_provider, IsOkAndHolds(NotNull());

(With #include "test/test_common/status_utility.h" and appropriate usings)

Same comment twice more in this file, once in test/common/formatter/substitution_formatter_test.cc, and many times in test/extensions/filters/udp/dns_filter/dns_filter_access_log_test.cc (arguably not in the helper function), and many in cel_test.

For these later 'many' cases I would suggest bumping the provider-collection up into the test class's initialization or a helper function, to cut down on repetition and make it clearer that the tests aren't really testing the return value of the parse function.

Fixed, including other cases. Thanks for suggestion.

ravenblackx · 2026-06-23T19:49:36Z

+  auto provider = parser->parse("NOT_SECRET", "my-token", absl::nullopt).value();
  EXPECT_EQ(nullptr, provider);


Hm, so it is expected to sometimes return a non-error null?

Signed-off-by: yavlasov <yavlasov@google.com>

- Change CommandParser::parse to return absl::StatusOr<FormatterProviderPtr> - Update all formatter implementations and extensions - Update all callers and tests to use new StatusOr return type - Preserve CVE-2026-47220 fix (null checks in getHostFromHeaders) - Keep CVE tests for empty headers in substitution_formatter_test.cc Co-authored-by: yanavlasov <6360027+yanavlasov@users.noreply.github.com>

Change CommandParser::parse to return StatusOr

b2462c2

Signed-off-by: Yan Avlasov <yavlasov@google.com>

kyessenov previously approved these changes Jun 22, 2026

View reviewed changes

botengyao reviewed Jun 23, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 23, 2026

botengyao self-assigned this Jun 23, 2026

yanavlasov added 2 commits June 23, 2026 11:25

Merge branch 'main' into remove-exceptions-5

2b5024a

Fix build errors

0d347a1

Signed-off-by: yavlasov <yavlasov@google.com>

yanavlasov dismissed kyessenov’s stale review via 0d347a1 June 23, 2026 18:04

yanavlasov requested review from adisuissa, agrawroh, ggreenway, mathetake, mattklein123, nezdolik, ravenblackx, wbpcode and yanjunxiang-google as code owners June 23, 2026 18:04

repokitteh-read-only Bot removed the waiting label Jun 23, 2026

yanavlasov enabled auto-merge (squash) June 23, 2026 18:50

ravenblackx requested changes Jun 23, 2026

View reviewed changes

ravenblackx self-assigned this Jun 23, 2026

ravenblackx added the waiting label Jun 23, 2026

Address comments

4982afe

Signed-off-by: yavlasov <yavlasov@google.com>

repokitteh-read-only Bot removed the waiting label Jun 23, 2026

Copilot AI mentioned this pull request Jun 23, 2026

Fix compile_time_options CI failure: apply CommandParser::parse StatusOr change on top of CVE-2026-47220 fix #45819

Closed

ravenblackx approved these changes Jun 24, 2026

View reviewed changes

yanavlasov merged commit 4430600 into envoyproxy:main Jun 24, 2026
28 checks passed

yanavlasov deleted the remove-exceptions-5 branch June 26, 2026 14:16

kamalmarhubi mentioned this pull request Jul 2, 2026

formatter: expose CEL expression config in header mutations #45815

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change CommandParser::parse to return StatusOr#45752

Change CommandParser::parse to return StatusOr#45752
yanavlasov merged 4 commits into
envoyproxy:mainfrom
yanavlasov:remove-exceptions-5

yanavlasov commented Jun 22, 2026

Uh oh!

kyessenov left a comment

Uh oh!

botengyao left a comment

Uh oh!

ravenblackx Jun 23, 2026 •

edited

Loading

Uh oh!

yanavlasov Jun 23, 2026

Uh oh!

ravenblackx Jun 23, 2026 •

edited

Loading

Uh oh!

yanavlasov Jun 23, 2026

Uh oh!

ravenblackx Jun 23, 2026

Uh oh!

yanavlasov Jun 23, 2026

Uh oh!

ravenblackx Jun 23, 2026

Uh oh!

yanavlasov Jun 23, 2026

Uh oh!

ravenblackx Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		auto provider = commands[0]->parse("COMMAND_EXTENSION", "", max_length).value();
		ASSERT_TRUE(provider != nullptr);

		auto provider = parser->parse("NOT_SECRET", "my-token", absl::nullopt).value();
		EXPECT_EQ(nullptr, provider);

Uh oh!

Conversation

yanavlasov commented Jun 22, 2026

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ravenblackx Jun 23, 2026 •

edited

Loading

ravenblackx Jun 23, 2026 •

edited

Loading