Skip to content

Conversation

lupaulus
Copy link
Contributor

@lupaulus lupaulus commented Aug 7, 2025

📋 Overview

I used an LLM for the contributions Assisted by Copilot but need to readapt

One potential Issue:

With wildcards, many simultaneous messages arriving to the client, can slow the process of handling and impact.
The added regex check doesn't help. I am not good enough in JS to produce some enhancement for performance

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • [] 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify): Test Regex MQTT

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any)
  • ✅ I ran ESLint and other code linters for modified files. (not local)
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

There are a few parts regarding formating, where CI is not happy. Those should be striaght forward enough to fix.

static mqttTopicRegex(subcribedTopic) {
subcribedTopic = subcribedTopic.replace(/([$.|?*{}()\[\]\\])/g, '\\$1'); // Escape special regex chars except + and #
subcribedTopic = subcribedTopic.replace(/\+/g, '[^/]+'); // Replace + with regex for one or more characters except slash
subcribedTopic = subcribedTopic.replace(/#/g, '.*'); // Replace # with regex for zero or more characters until next slash
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think \# would also be affected under the current implementation.
Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, i don't know to much of the js regex engine but # is not considered as special char

I can add a test to check it

const { log, UP } = require("../../src/util");
const mqtt = require("mqtt");
const jsonata = require("jsonata");
const { regex } = require("nostr-tools/nip30");
Copy link
Collaborator

Choose a reason for hiding this comment

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

MQtt and nostr don't seem really connected.
Can you add a comment explaining this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrong import, auto import from vs-code ...

assert.ok(regex.test('sensor.pomme/temperature') === true);
assert.ok(regex.test('sensor.pomme/humidity') === false);
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a testcase specifically for the escaping of #, + and the other regex characters.

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Aug 7, 2025
*/
static mqttTopicRegex(subcribedTopic) {
subcribedTopic = subcribedTopic.replace(/([$.|?*{}()\[\]\\])/g, '\\$1'); // Escape special regex chars except + and #
subcribedTopic = subcribedTopic.replace(/\+/g, '[^/]+'); // Replace + with regex for one or more characters except slash
Copy link
Collaborator

@CommanderStorm CommanderStorm Aug 7, 2025

Choose a reason for hiding this comment

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

I don't think this quite works when the start of the string is "+", or am I misunderstanding the syntax here wrong?

This would meann that \+ => \[^/]+ which likely won't work as you want it to.

@CommanderStorm CommanderStorm requested a review from Copilot August 9, 2025 09:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds MQTT wildcard support (+ and #) to the MQTT monitor type, allowing subscription to multiple topics using MQTT wildcard patterns. The implementation converts MQTT wildcard patterns into regular expressions for topic matching.

  • Implements MQTT wildcard support by adding regex-based topic matching
  • Replaces exact string matching with regex pattern matching for topic subscription
  • Adds comprehensive test coverage for wildcard functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
server/monitor-types/mqtt.js Adds mqttTopicRegex() method and replaces exact topic matching with regex-based matching
test/backend-test/test-mqtt.js Adds comprehensive test cases for wildcard matching scenarios

clientId: "uptime-kuma_" + Math.random().toString(16).substr(2, 8)
});

var regexTopic;
Copy link
Preview

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

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

Use 'let' instead of 'var' for block-scoped variable declaration following modern JavaScript best practices.

Suggested change
var regexTopic;
let regexTopic;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOPE here explicit usage of var for global scope of the instance

@lupaulus
Copy link
Contributor Author

need to check and test with regex engine

image

@lupaulus
Copy link
Contributor Author

lupaulus commented Aug 13, 2025

For me they are wrong positive from Eslint on 101regex the pattern are not correct recognized without escaping chars!

Don't merge until my go, I will do real tests with the app next week :)

image

@lupaulus
Copy link
Contributor Author

Still in progress, please don't close

@CommanderStorm
Copy link
Collaborator

We generally only close PRs which are irrevocably stuck.
Those obviously also can be reopened.
This one is not stuck, it just needs a testcase and maybe a bugfix depending on said testcase.

@CommanderStorm CommanderStorm marked this pull request as draft August 27, 2025 01:21
@lupaulus lupaulus marked this pull request as ready for review September 11, 2025 20:48
@CommanderStorm CommanderStorm added pr:needs review this PR needs a review by maintainers or other community members and removed pr:please address review comments this PR needs a bit more work to be mergable labels Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs review this PR needs a review by maintainers or other community members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mqtt] Add Monitoring of Wildcard Topics like "/device/#"
2 participants