NOISSUE: add subtopic support for nats messages #565
Conversation
Codecov Report
@@ Coverage Diff @@
## master #565 +/- ##
==========================================
- Coverage 87.13% 87.11% -0.03%
==========================================
Files 62 62
Lines 3437 3391 -46
==========================================
- Hits 2995 2954 -41
+ Misses 300 298 -2
+ Partials 142 139 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good and very nice fix, I just need to re-check this regexp in little more detail.
@nmarcetic please review
beres
force-pushed
the
master
branch
4 times, most recently
from
e326b09
to
2c26751
Compare
|
@beres Can you please update branch? |
There was a problem hiding this comment.
@beres Maybe is a better idea if you can align it with authorizePublish maybe even move this parse logic to separate helper function and just use it in both publish and subscribe, its exactly the same. I don't like the idea to repeat the code and do the same thing in 2 different ways, maybe I am wrong but looks like the same logic no? What do you think?
Signed-off-by: ale <ale@beffect.net>
- nat.subsscribe: rewrite simplier regexp - nat.subscribe: fix full topic name composition
|
@beres this looks good, please update the branch. |
| if (m && m.protocol !== 'mqtt') { | ||
| // rexexp has two element, the first one is the channelId, wile | ||
| // the second is undefined or the subtopic parts starting from the dot | ||
| channelParts = /^([\w\-]+)(\..+[^\.]$)*$/.exec(m.channel) |
There was a problem hiding this comment.
@drasko AFAIK earlier we used channel only for channel ID. This means that we'll store whole topic in channel field, which is not only channel ID. Just want you to be aware of this. Also, it won't work with the current system (I think).
There was a problem hiding this comment.
Sorry, didn't realized that there was a dot there. But still, I didn't know that you've already decided that you're going to pass subtopic here too. Maybe the rest of the team should be in sync with this change too.
There was a problem hiding this comment.
Yep, subtopics will be enabled for all 4 adapters. @nmarcetic - remainder to open the issues.
There was a problem hiding this comment.
That's OK, but there is no spec how will this be done, that's why I don't know if this is how we are going to do it. So we are going to pass <channel_id>.subtopic in channel field? Why not adding separate field that will be called subtopic, and leave channel field as is? Then there will be no need to extract channel_id from string.
There was a problem hiding this comment.
Yes, that would make code probably cleaner. But to see exactly what you propose, can you write here a small snippet example please.
There was a problem hiding this comment.
So, instead of using current channel field from here, I would add a new field: subtopic. So our structure would look like this:
message RawMessage {
string channel = 1;
string subtopic = 2;
string publisher = 3;
string protocol = 4;
string contentType = 5;
bytes payload = 6;
}
In channel, we would store just channel ID, like we've done so far, and in subtopic we would store everything else (subtopic).
| publish(4); // Bad username or password | ||
| return; | ||
| } | ||
| var channelId = channel[1], | ||
|
|
||
| var channelId = channelParts[1], |
There was a problem hiding this comment.
What if channelParts has length 1.
| if (m && m.protocol !== 'mqtt') { | ||
| // rexexp has two element, the first one is the channelId, wile | ||
| // the second is undefined or the subtopic parts starting from the dot | ||
| channelParts = /^([\w\-]+)(\..+[^\.]$)*$/.exec(m.channel) |
|
What do you think to reduce the valid characters in the subtopic to [a-zA-Z0-9_] and - like in channelId? But if we limit the characters as in the case of the channelId we should do two regexp one for publish and one for subscribe, this should also accept the asterisk character. PS: at the moment the regexp does not accept * in the subtopic |
|
Duplicated #642 |
What does this do?
Nats listener subscribe to "channels.>" to catch all messages to manage also subtopic.
Which issue(s) does this PR fix/relate to?
List any changes that modify/break current functionality
Have you included tests for your changes?
Did you document any new/modified functionality?
Notes