Clean up Matcher::MatchResult's unnecessary templates by ravenblackx · Pull Request #39673 · envoyproxy/envoy

ravenblackx · 2025-05-29T21:49:53Z

Commit Message: Clean up Matcher::MatchResult's unnecessary templates
Additional Description: Since #38726 refactored Matcher impls to recurse inside the match function, there is no longer any need for MatchResults to have the capacity to return sub-match-trees, which means they also don't need to be templates. Cleaning this up makes the code much easier to follow. This may have also caught a few locations where the internal recursion wasn't fully updated correctly (minor negative, it does make one doMatch() implementation slightly more complicated, but the corresponding match() implementation is made much simpler, so still an improvement).
Risk Level: Small, existing tests should be ensuring that behaviors were unchanged.
Testing: Covered by existing tests, which are also cleaner.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Raven Black <[email protected]>

repokitteh-read-only · 2025-05-29T21:49:58Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #39673 was opened by ravenblackx.

see: more, trace.

Signed-off-by: Raven Black <[email protected]>

ravenblackx · 2025-05-30T20:47:23Z

/retest

keithmattix

I built a patch off of this branch and it does appear to fix the regression we saw in Istio. Thanks for this!

keithmattix · 2025-06-03T15:41:27Z

    const auto itr = children_.find(key);
    if (itr != children_.end()) {
-      return {MatchState::MatchComplete, itr->second};
+      MatchResult result =


Looks like this is the part that fixes the regression we saw in Istio. Could a test be added/adapted to ensure on_no_match for the different tree types works as expected?

I was planning on making a common "matcher tree implementation test" for this purpose that all matcher tree implementations are required to pass, because I agree that it being possible to have this regression was bad. But I would rather add that as a separate PR.

Yeah that's fair

kyessenov · 2025-06-03T17:20:38Z

+// The result of a match. There are three possible results:
+// - The match could not be completed due to lack of data (InsufficientData)
+// - The match was completed, no match found (NoMatch)
+// - The match was completed, match found (onMatch() will return the OnMatch).


onMatch is no longer present, please update the comment.

kyessenov · 2025-06-03T17:25:42Z

-      return {MatchState::MatchComplete, absl::nullopt};
+      return MatchResult::noMatch();
    }
    if (on_match->matcher_) {


on_match could use a variant! then a switch or a visit would make it easier to parse.

Yes, but doesn't have to be in this PR. (And that one isn't as big of an improvement since it doesn't get rid of the template. I think it's possible to get rid of the template too, but that's a much more aggressive change.)

Signed-off-by: Raven Black <[email protected]>

wbpcode

LGTM overall. This change seems reasonable for me. Only minor comments to the naming, bla, bla...

wbpcode · 2025-06-04T02:08:24Z

+struct MatchResult {
+private:
+  struct InsufficientData {};
+  struct NoMatch {};
+  using Result = absl::variant<ActionFactoryCb, NoMatch, InsufficientData>;
+  Result result_;
+  MatchResult(NoMatch) : result_(NoMatch{}) {}
+  MatchResult(InsufficientData) : result_(InsufficientData{}) {}
+
+public:
+  MatchResult(ActionFactoryCb cb) : result_(std::move(cb)) {}
+  static MatchResult noMatch() { return MatchResult(NoMatch{}); }
+  static MatchResult insufficientData() { return MatchResult(InsufficientData{}); }
+  bool isInsufficientData() const { return absl::holds_alternative<InsufficientData>(result_); }
+  bool isComplete() const { return !isInsufficientData(); }
+  bool isNoMatch() const { return absl::holds_alternative<NoMatch>(result_); }
+  bool isMatch() const { return absl::holds_alternative<ActionFactoryCb>(result_); }
+  ActionFactoryCb actionFactory() const { return absl::get<ActionFactoryCb>(result_); }
+  ActionPtr action() const { return actionFactory()(); }


nit: we may prefer place the public member at top?

Ah, I thought the structs had to be declared before they're used, but apparently not the case in a class declaration. Done.

wbpcode · 2025-06-04T02:09:24Z

+  static MatchResult noMatch() { return MatchResult(NoMatch{}); }
+  static MatchResult insufficientData() { return MatchResult(InsufficientData{}); }
+  bool isInsufficientData() const { return absl::holds_alternative<InsufficientData>(result_); }
+  bool isComplete() const { return !isInsufficientData(); }


I think this isComplete is unncessary.

It's unnecessary but makes it more readable at the user end (!isInsufficientData doesn't describe what the user end cares about in many cases), and also makes it more resilient for future changes - if a fourth state were to be added that also means the match wasn't resolved yet, every location that wrote !isInsufficientData() to mean this would need to update, or if a fourth state was added that also means the match was resolved then every location that wrote isMatch() || isNoMatch() for the same meaning would need to be updated. Having a function for the specific thing that the user cares about means any change can be localized here, and also makes it unambiguous which thing you should use (removing the question "should I use isMatch() || isNoMatch() or should it be !isInsufficientData()?").

Also !isInsufficientData() being a double negative makes it horrible to read. :)

wbpcode · 2025-06-04T02:09:56Z

+  bool isInsufficientData() const { return absl::holds_alternative<InsufficientData>(result_); }
+  bool isComplete() const { return !isInsufficientData(); }
+  bool isNoMatch() const { return absl::holds_alternative<NoMatch>(result_); }
+  bool isMatch() const { return absl::holds_alternative<ActionFactoryCb>(result_); }


isMatch() -> isMatched. To keep same with the FieldMatchResult

They have different meanings - isMatched() means the result was "it matched", a bool, vs. isMatch() means the result was a match, with an object.

(I would be happier to change it to isAction or isActionCb than to isMatched, but I'd rather not change it at all and get it landed so the regression is fixed.)

hmmm, I feel a little weird to these naming and method, but okay.

Signed-off-by: Raven Black <[email protected]>

wbpcode

LGTM. Thanks.

ravenblackx · 2025-06-05T18:07:02Z

/retest

zirain · 2025-06-06T00:40:02Z

/retest

Clean up Matcher::MatchResult's unnecessary templates

8f85394

Signed-off-by: Raven Black <[email protected]>

ravenblackx added 3 commits May 30, 2025 14:14

trie_matcher

066b142

Signed-off-by: Raven Black <[email protected]>

logline

2a28bd3

Signed-off-by: Raven Black <[email protected]>

piddling errors

3ef91f3

Signed-off-by: Raven Black <[email protected]>

ravenblackx assigned wbpcode May 30, 2025

ravenblackx marked this pull request as ready for review May 30, 2025 15:02

ravenblackx requested review from botengyao, mattklein123, tyxia, wbpcode and yanavlasov as code owners May 30, 2025 15:02

inputs_integration_test

98d3474

Signed-off-by: Raven Black <[email protected]>

ravenblackx requested a review from kyessenov as a code owner May 30, 2025 17:23

ravenblackx added 2 commits May 30, 2025 18:35

cel_matcher_test

2a5c870

Signed-off-by: Raven Black <[email protected]>

Moar touched tests

f2cee5d

Signed-off-by: Raven Black <[email protected]>

ravenblackx requested a review from adisuissa as a code owner May 30, 2025 19:01

keithmattix mentioned this pull request May 31, 2025

Matcher + MatcherTree support for a keep_matching field in OnMatch #38726

Merged

keithmattix reviewed Jun 3, 2025

View reviewed changes

kyessenov reviewed Jun 3, 2025

View reviewed changes

Update comment for MatchResult

b8e4d62

Signed-off-by: Raven Black <[email protected]>

wbpcode reviewed Jun 4, 2025

View reviewed changes

Move private fields to bottom

77e90fa

Signed-off-by: Raven Black <[email protected]>

wbpcode approved these changes Jun 5, 2025

View reviewed changes

ravenblackx enabled auto-merge (squash) June 5, 2025 18:07

ravenblackx merged commit 2fddebb into envoyproxy:main Jun 6, 2025

ravenblackx deleted the matcher_cleanup branch June 6, 2025 14:58

yuval-k mentioned this pull request Jul 10, 2025

update to 1.35 solo-io/envoy-gloo#446

Merged

Uh oh!

Conversation

ravenblackx commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented May 29, 2025

Uh oh!

ravenblackx commented May 30, 2025

Uh oh!

keithmattix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravenblackx Jun 5, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

ravenblackx commented Jun 5, 2025

Uh oh!

zirain commented Jun 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ravenblackx commented May 29, 2025 •

edited

Loading

ravenblackx Jun 5, 2025 •

edited

Loading