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

Feature topic #12

Merged
merged 8 commits into from
Jul 11, 2019
Merged

Feature topic #12

merged 8 commits into from
Jul 11, 2019

Conversation

pointlander
Copy link
Contributor

Adds support for topic parameters. For triggers, the output topicParams will be populated with any topic wildcard values. For activities, the input topicParams will be used to set the wildcards in the topic. Topics can take the form of '/x/+/y/#' in which case the topicParams are keyed: '0', '1', etc... For topics of the form '/x/+param1/y/#param2' the keys are 'param1' and 'param2'.

@pointlander
Copy link
Contributor Author

Fixes #9

@fm-tibco
Copy link

For the MQTT activity, should we consider generalizing it? Maybe using a similar mechanism to the rest stuff? If you find any ":paramName", do a replace with the matching value from the topicParams. For example /home/:room/temperature. Also does the wildcard topic syntax make sense in the case of the activity? I see the wildcard stuff making sense in the trigger.

@pointlander
Copy link
Contributor Author

The topic syntax for the trigger is consistent with the topic syntax for the activity (':' syntax can be supported along side the '+'/'#' syntax). For the activity, the alternative would be to use the string concat function to create topics dynamically.

@fm-tibco
Copy link

fm-tibco commented Jul 10, 2019

I guess what I'm saying is do wildcards even make sense in the topic for an mqtt send? We can just use the ':' syntax to dynamically create a send topic (like if you needed to send to an arbitrary sensor based on some data ex. /sensor/:id/settemp)

As for the trigger the :param syntax might make the topic matching a little harder for the topic to handler mappings. So not sure if it's worth it. Also would just returning the topic in addition to the payload be sufficient for the trigger case?

I just read this Publishers are not allowed to use the wildcard characters in their topic names. So I don't think we should have those in the activity.

@pointlander
Copy link
Contributor Author

The wildcard syntax ':' can be supported along side the wildcard syntax '+'/'#'. So, '/sensor/:id/settemp' would be equivalent to the topic '/sensor/+id/settemp' and do the same thing (dynamically create a topic).

Returning an unparsed topic for the trigger ('/sensor/1/settemp') would require the user to parse the topic string using flogo string functions.

@fm-tibco
Copy link

I wouldn't use the wildcard syntax in the activity since it is not allowed in the topic of a publisher and would just be confusing.

I guess for the trigger that would be fine if ":param" syntax essentially maps to "#param". We might also want return the topic in the output of the trigger to satisfy #9

@pointlander
Copy link
Contributor Author

':param' would map to '+param', '#' is the multi level wildcard in mqtt. ':' and '+' have the same semantics. Using ':' syntax in the activity would make the activity inconsistent with the trigger.

@fm-tibco
Copy link

fm-tibco commented Jul 10, 2019

It's ok that they are inconsistent, because you should not have "+" or "#" in a publish topic, so it doesn't make sense to have that in the activity.

We can avoid the ":" param syntax in the trigger so not confuse the two, so they are sufficiently different so people won't get confuse the two.

@pointlander
Copy link
Contributor Author

'/sensor/:id/settemp' would also be an invalid publish topic.

@fm-tibco
Copy link

fm-tibco commented Jul 10, 2019

Not really, if we do like the REST activity URI which basically applies the param map to it. It's not a wild card, it is just a substitution syntax. So if you have '/sensor/:id/settemp' with a param map of {"id":"upstairs"}, it would resolve it to '/sensor/upstairs/settemp' before it does the publish.

It would be how the path is created using the path parameters in the REST activity https://github.com/project-flogo/contrib/tree/master/activity/rest.

@pointlander
Copy link
Contributor Author

The same thing currently happens with a publish topic of '/sensor/+id/settemp'.

What if a user wants to use a ':' in the publish topic? The sent topic would be '/sensor/:id/settemp' (no substitutions). '+' and '#' are nice because they are reserved.

@fm-tibco
Copy link

fm-tibco commented Jul 10, 2019

The issue is that the "+" and "#" are wildcards, which are not intended to be used in the publish topic. Seeing those in the publish topic is more likely to confuse people than seeing some substitution syntax that we use elsewhere. If your concern isn't that ':' is not reserved, We can leave a value as is if there are no topic parameters set (or require escaping the colon if it's meant to be used as is?). I would be also open to some other substitution syntax, but would definitely avoid '+' and '#' since they have a particular meaning associated with them in the mqtt context.

@pointlander
Copy link
Contributor Author

change made @fm-tibco

@pointlander
Copy link
Contributor Author

will add topic to trigger output next

@pointlander
Copy link
Contributor Author

I think that is all of the changes that you wanted? @fm-tibco

@mellistibco
Copy link
Member

I agree, I don't think we should use wildcards in the activity but substitution within the topic does make sense. I can see how someone would want to dynamically build the topic without using string functions.

@fm-tibco fm-tibco merged commit 7f84828 into project-flogo:master Jul 11, 2019
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.

None yet

4 participants