Skip to content
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

Utility class supporting dynamic topics using topic templates #1932

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

simonthum
Copy link
Contributor

@simonthum simonthum commented Feb 17, 2024

This is essentially some support code to handle dynamic topic subscription, publication and related functonality without falling back to String.Format() and other cumbersome handcrafted magic.

The code has seen a bit of production use and I figured it might be a useful addition to the library. I realize it could well live outside of MqttNet, but OTOH I've seen several failures indicating that topic parsing/generation is generally quite error-prone.

The MqttTopicTemplate class processes "topic templates" like A/B/{parameter}/D and provides a fluent interface to operate on the named parameter(s). A bunch of extension methods hook up with related MqttNet APIs, resulting in code like

var topicTemplate = new MqttTopicTemplate("A/v1/{param}/F");
var filter = topicTemplate.BuildFilter()
    .WithAtLeastOnceQoS()
    .WithNoLocal()
    .Build();  // subscribes A/v1/+/F

var msg = new MqttApplicationMessageBuilder()
    .WithTopicTemplate(
        topicTemplate.WithParameter("param", "foo"))
    .Build();  // A/v1/foo/F

Effectively, this enables one to handle subscription and publication from one topic template. See the unit tests for more advanced usage, like routing and canonical prefix derivation.

The syntax is simple and fairly common, inside and outside of pub/sub. It is not standardized, but AsyncAPI calls this a "channel address expression".

The unit tests did not run so far due to technicalties, but the code works in the internal codebase. I made some effort to align it to MqttNet conventions (as I perceive them). To proceed, I mainly would like to know if such an addition is considered a good fit for MqttNet at all.

@simonthum
Copy link
Contributor Author

@dotnet-policy-service agree [company="medDV GmbH'"]

@simonthum simonthum changed the title Utility class supporting topic templates / dynamic topics Utility class supporting dynamic topics using topic templates Feb 19, 2024
@simonthum
Copy link
Contributor Author

@dotnet-policy-service agree company="medDV GmbH"

@simonthum
Copy link
Contributor Author

@chkr1011 It seems the CI failures require your attention.

@chkr1011
Copy link
Collaborator

chkr1011 commented Mar 2, 2024

I like the idea of working with templates but in my opinion it should not be part of the core library. I would rather move it to an extension library like "MQTTnet.Extensions.TopicTemplates".

@simonthum
Copy link
Contributor Author

You mean alongside Rpc and Managed Client? That sounds fine.

@simonthum
Copy link
Contributor Author

I'd need to know what can be done about the tests though.

The topic template class is a way to deal with dynamic topics that
is less cumbersome and error-prone than String.Format or
other string fu one might use to handle dynamic topics.

Example:

  MyCity/{street}/{crossing}/lights

'street' and 'crossing' would be parameters.

The class and its extension methods are intended to support
dynamic subscription/publication, message-topic matching and
routing. The template syntax is modelled after the well-known
moustache syntax, which conincidentally is also published
as the 'AsyncAPI channel address expressions'.
@simonthum simonthum force-pushed the feature/topic-templates branch from 7d48ef8 to 39ad866 Compare March 15, 2024 15:08
@simonthum
Copy link
Contributor Author

I have restructured the code for publication as an extension.

I would like to add more tests and examples, but first the build has to work. It seems that the failing security checks are unrelated. Is there anything more I can do but waiting?

@chkr1011 chkr1011 self-requested a review April 19, 2024 15:15
@chkr1011
Copy link
Collaborator

@simonthum I applied the code style from the team shared settings in Rider and also extended the release notes. If you agree with the changes I will merge the PR

@simonthum
Copy link
Contributor Author

Hi, sorry I did not use Rider. I am OK with the formatting changes, please go ahead and merge.

One minor thing: You also updated one file in the RPC extension which I did not change (at least not intentionally).

@simonthum
Copy link
Contributor Author

I should add that unit tests have not yet been run on my side, so they might still contain glitches. I reworked them to be more instructive, but was unable to run them locally. I can probably fix things in a few days if required.

@chkr1011 chkr1011 merged commit 2410d7b into dotnet:master Apr 23, 2024
1 of 2 checks passed
@simonthum simonthum deleted the feature/topic-templates branch April 23, 2024 20:06
@simonthum
Copy link
Contributor Author

simonthum commented Apr 23, 2024

Hi, thanks for merging!

I installed rider, got unit tests to work and go green 🟢

https://github.com/simonthum/MQTTnet/tree/fix-pr-1932

Should I make another PR or do you prefer to fetch them?

@simonthum simonthum restored the feature/topic-templates branch April 23, 2024 20:12
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.

2 participants