Skip to content

fix(microservices): Revisit RMQ pattern matching with wildcards #15305

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

getlarge
Copy link

@getlarge getlarge commented Jun 18, 2025

close #15304

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: 15304

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Before starting I asked myself, what would be the best approach?

Option 1: Keep Current Regex but Make it Safer

Tradeoffs:

  • ✅ Minimal change, maintains current performance, backward compatible
  • ❌ Still vulnerable to other typical regex issues, complex escaping logic, harder to maintain

Option 2: Use Map and Nested For Loop same as SeerverMQTT

Based on MQTT approach (matchMqttPattern in server-mqtt.ts:180-210):

  • Split patterns and topics by separator
  • Loop through segments comparing each one
  • Handle wildcards with simple string comparison

Tradeoffs:

  • ✅ No regex vulnerabilities, easier to understand, similar to existing MQTT code
  • ✅ More predictable performance (O(n) where n = segments)
  • ❌ Different implementation from current regex approach
  • ❌ Might be less efficient when there are many handlers and topic segments

Option 3: Use Trie to store patterns and handlers (such as https://github.com/davedoesdev/qlobber)

Based on qlobber library:

  • Stores patterns in a trie data structure
  • Highly optimized for topic matching
  • Based on the same algorithm used for RabbitMQ exchange routing

Tradeoffs:

  • ✅ Best performance for many patterns (O(log n) lookup)
  • ✅ Handles complex wildcard scenarios elegantly
  • ✅ Would work for ServerMQTT too
  • ❌ Adds external dependency
  • ❌ Larger implementation change

I went with option 2: it seems more reasonable.

@coveralls
Copy link

coveralls commented Jun 18, 2025

Pull Request Test Coverage Report for Build 6946ab43-e618-4eb3-abcb-5a9d77424269

Details

  • 21 of 26 (80.77%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.09%) to 89.011%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-rmq.ts 18 23 78.26%
Files with Coverage Reduction New Missed Lines %
packages/microservices/server/server-rmq.ts 1 68.52%
Totals Coverage Status
Change from base Build d1bcf6a0-b4c6-48df-a154-e75b2d7bfac9: 0.09%
Covered Lines: 7217
Relevant Lines: 8108

💛 - Coveralls

@robertsLando
Copy link

@getlarge The linked PR number is wrong

Copy link

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM, this it's not just safer but also more performant compared to pervious approach 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unescaped $ sign in pattern prevent pattern matching in ServerRmq
3 participants