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.

Sign up for GitHub

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

Jump to bottom

[CHANGED] MQTT: Support for topics with . character. #4243

Merged
merged 2 commits into from Jun 14, 2023
Merged

[CHANGED] MQTT: Support for topics with . character. #4243

merged 2 commits into from Jun 14, 2023

Conversation

kozlovic
Copy link
Member

The . character will be transformed to // in NATS subject. For instance an MQTT message published on spBv1.0/plant1 would be received by a NATS subscriber as spBv1//0.plant1.

Conversely, a NATS message published on spBv1//0.plant1 would be received by an MQTT subscriber as spBv1.0/plant1.

Resolves #1879
Resolves #3482

Signed-off-by: Ivan Kozlovic ivan@synadia.com

@kozlovic
[CHANGED] MQTT: Support for topics with . character.
1ac99fd 
The `.` character will be transformed to `//` in NATS subject. For
instance an MQTT message published on `spBv1.0/plant1` would
be received by a NATS subscriber as `spBv1//0.plant1`.

Conversely, a NATS message published on `spBv1//0.plant1` would
be received by an MQTT subscriber as `spBv1.0/plant1`.

Resolves #1879
Resolves #3482

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic requested a review from a team as a code owner June 13, 2023 19:21
derekcollison
derekcollison reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Is // ilegal in a MQTT topic today? Meaning we will not encounter it natively in an MQTT application yes?

@kozlovic
Copy link
Member Author

Is // illegal in a MQTT topic today? Meaning we will not encounter it natively in an MQTT application yes?

It is legal actually, but the point of this PR is converting . from an MQTT topic into // in a NATS subject. The reason I had to select // as opposed to / is because of the need to convert valid MQTT topic such as /myhome in MQTT to /.myhome in NATS (as described here: https://docs.nats.io/running-a-nats-service/configuration/mqtt#mqtt-topics-and-nats-subjects). There would be cases where I would not be able to know how to translate a NATS subject back into MQTT (meaning, there would be a risk of converting to the wrong original MQTT topic).

Btw, once that is approved/merged, I will have to do a PR against the documentation mentioned above to update the table.

@derekcollison
Copy link
Member

So what happens if we encounter // natively in an MQTT subject that moves into the NATS space (say a microservice) and then comes back in? Will we handle properly on convert it to a .?

@kozlovic
Copy link
Member Author

kozlovic commented Jun 13, 2023 •
edited

So what happens if we encounter // natively in an MQTT subject that moves into the NATS space (say a microservice) and then comes back in? Will we handle properly on convert it to a .?

No, if it comes from MQTT, say as a//b, following the table I pointed to, we would convert to a./.b, which on the way back would be translated to a//b. However, if MQTT topic is a.b, then it would be translated to a//b, and converted back to a.b.
I have test that check conversions from MQTT to NATS and back that handles // in MQTT (that has been extended in this PR). For instance, for the a//b case, this is the line:

{"foo//bar", "foo//bar", "foo./.bar", ""},

in the TestMQTTTopicAndSubjectConversion test.

@kozlovic
fix test
f2d009b 
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

See nats.docs PR: nats-io/nats.docs#601

@derekcollison derekcollison self-requested a review June 14, 2023 03:41
derekcollison
derekcollison approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 91d0b6a into dev Jun 14, 2023
2 checks passed
@derekcollison derekcollison deleted the mqtt_dot_in_topics branch June 14, 2023 03:44
tomhollingworth pushed a commit to tomhollingworth/nats-server that referenced this pull request Dec 19, 2023
[FIXED] Fix unsupported MQTT characters error message
0ca2e47 
Previously ' ' and '.' were not supported for MQTT topics until nats-io#4243 brought in support for '.'. However the error message for unsupported characters appears to not have been updated. This simply updates the error message.

Had an example of a MQTT client trying to publish to `spBv1.0/Field Devices/Some Device/Some Topic` and the error message indicated that both '.' and ' ' were unsupported which was misleading as only ' ' is unsupported.
@tomhollingworth tomhollingworth mentioned this pull request Dec 19, 2023
Merged
derekcollison added a commit that referenced this pull request Dec 19, 2023
@derekcollison
[FIXED] Fix unsupported MQTT topic characters error message (#4903)
3d8a017 
Previously ' ' and '.' were not supported for MQTT topics until
[#4243](#4243) brought in
support for '.'. However the error message for unsupported characters
appears to not have been updated. This simply updates the error message.

Had an example of a MQTT client trying to connect to `spBv1.0/Field
Devices` and the error message indicated that both '.' and ' ' were
unsupported which was misleading as only ' ' is unsupported.

Signed-off-by: Tom Hollingworth <tom.hollingworth@spruiktec.com>
wallyqs pushed a commit that referenced this pull request Dec 26, 2023
@wallyqs
[FIXED] Fix unsupported MQTT characters error message
1cddfa9 
Previously ' ' and '.' were not supported for MQTT topics until #4243 brought in support for '.'. However the error message for unsupported characters appears to not have been updated. This simply updates the error message.

Had an example of a MQTT client trying to publish to `spBv1.0/Field Devices/Some Device/Some Topic` and the error message indicated that both '.' and ' ' were unsupported which was misleading as only ' ' is unsupported.
wallyqs pushed a commit that referenced this pull request Dec 26, 2023
@wallyqs
[FIXED] Fix unsupported MQTT characters error message
1cdcafa 
Previously ' ' and '.' were not supported for MQTT topics until #4243 brought in support for '.'. However the error message for unsupported characters appears to not have been updated. This simply updates the error message.

Had an example of a MQTT client trying to publish to `spBv1.0/Field Devices/Some Device/Some Topic` and the error message indicated that both '.' and ' ' were unsupported which was misleading as only ' ' is unsupported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kozlovic @derekcollison