fix(grpc): allow multiple wildcard subscriptions when topic validation disabled#1083
fix(grpc): allow multiple wildcard subscriptions when topic validation disabled#1083Rishabh-git10 wants to merge 3 commits into
Conversation
…n disabled Signed-off-by: Rishabh Dewangan <107680241+Rishabh-git10@users.noreply.github.com>
seherv
left a comment
seherv
left a comment
There was a problem hiding this comment.
Thanks a lot for contributing to Dapr! Here are my comments
| if cb is None: | ||
| context.set_code(grpc.StatusCode.UNIMPLEMENTED) # type: ignore | ||
| raise NotImplementedError(f'topic {request.topic} is not implemented!') | ||
| raise NotImplementedError(f'topic {request.topic} topic not implemented!') |
|
|
||
| self._wildcard_topics: List[Tuple[str, str, str, bool, TopicSubscribeCallable]] = [] | ||
|
|
||
| def _match_topic(self, pattern: str, topic: str) -> bool: |
There was a problem hiding this comment.
We need to use a different approach, this would be a second filter on top of what the pubsub already filters by matching a wildcard pattern to a concrete topic name.
More on the following comments
|
|
||
| sub = registered_topic.subscription | ||
| rules = registered_topic.rules | ||
|
|
There was a problem hiding this comment.
Here you should build an additional route map based on topic to know which handler actually takes it in case you get a wildcard. Something like
if disable_topic_validation and rule is None:
sub.routes.default = topic
self._route_map[(pubsub_name, topic)] = cb| cb = None | ||
| for p_name, t_pattern, path, d_val, w_cb in self._wildcard_topics: | ||
| if p_name == request.pubsub_name and request.path == path: | ||
| if self._match_topic(t_pattern, request.topic) or ( | ||
| d_val and not any(c in t_pattern for c in ['+', '#', '*']) | ||
| ): | ||
| cb = w_cb | ||
| break |
There was a problem hiding this comment.
Instead of doing the regex match you had before, you can look up the cb on the route map
| cb = None | ||
| for p_name, t_pattern, path, d_val, w_cb in self._wildcard_topics: | ||
| if p_name == request.pubsub_name and request.path == path: | ||
| if self._match_topic(t_pattern, request.topic) or ( | ||
| d_val and not any(c in t_pattern for c in ['+', '#', '*']) | ||
| ): | ||
| cb = w_cb | ||
| break | ||
| if cb is None: | ||
| return None # we don't have a handler |
There was a problem hiding this comment.
Same as above. Logic looks pretty similar and could be moved to a helper method too
Signed-off-by: Rishabh Dewangan <107680241+Rishabh-git10@users.noreply.github.com>
|
Thanks for the review and guidance @seherv. |
2 participants
Description
Fixes a
ValueErrorstartup crash and subscription drop when registering multiple wildcard paths on the same pub/sub component withdisable_topic_validation=True.Previously, enabling this flag collapsed internal tracking keys down to the pub/sub identifier alone, causing map collisions and rendering the sidecar blind to subsequent subscriptions. This change standardizes discrete internal dictionary keys across all registration paths. It introduces a regex-based pattern matching fallback evaluation inside
OnTopicEventand_handle_bulk_topic_eventto route events when direct string lookups fail.Issue reference
Closes #436
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: