python-sdk icon indicating copy to clipboard operation
python-sdk copied to clipboard

PubSub disable_topic_validation=True breaks multiple subscriptions

Open ngiind opened this issue 2 years ago • 5 comments

Adding disable_topic_validation=True to pusub subscription raises ValueError when adding multiple subscriptions. Looks like the issue is that the topic map key in this case will only use the pubsub identifier (dapr\ext\grpc_servicier.py):

        if not disable_topic_validation:
            topic_key = pubsub_name + DELIMITER + topic
        else:
            topic_key = pubsub_name
        pubsub_topic = topic_key + DELIMITER
        if rule is not None:
            path = getattr(cb, '__name__', rule.match)
            pubsub_topic = pubsub_topic + path
        if pubsub_topic in self._topic_map:
            raise ValueError(
                f'{topic} is already registered with {pubsub_name}')
        self._topic_map[pubsub_topic] = cb

ngiind avatar Aug 01 '22 10:08 ngiind

This is also how the Go SDK implementation works today. I am hesitant to change this behavior. See https://github.com/dapr/go-sdk/pull/271

Perhaps it is worth to leave a comment here again instead https://github.com/dapr/dapr/issues/4461 since this is the main tracking issue.

We need consensus on how to handle this across Go SDK and Python SDK.

berndverst avatar Aug 04 '22 00:08 berndverst

I agree that the Go SDK and Python SDK should work the same way. I will add a coment to the main tracking issue.

ngiind avatar Aug 04 '22 07:08 ngiind

How do you expect disable_topic_validation to work @ngiind ?

berndverst avatar Jan 25 '23 01:01 berndverst

What I want to be able to do is to have multiple subscriptions on different wildcard topics in the same dapr app. I have used disable_topic_validation to make wildcard subscriptions to work (as per https://github.com/dapr/dapr/issues/4461), but when combining this with multiple subscriptions, this breaks.

I expect to be able to combine multiple subscriptions and wildcard topics. This does not necessarily need to be by using disable_topic_validation. I agree that the flag name might indicate that multiple subscriptions should not work.

ngiind avatar Jan 25 '23 08:01 ngiind

Same issue here. Just checking in on the status, any progress? @berndverst

Here's my suggestion.

Current Behavior:

When disable_topic_validation is enabled, topic-level validation is disabled. However, the internal subscription callback map in _CallbackServicier only allows a single callback per pubsub component, not per topic. When disable_topic_validation is disabled, topic-level callback registration is allowed. However, wildcards in topics cannot pass validation since they are treated literally. For example, a topic named a/b will not match the string a/+, causing validation to fail.

Issue:

The current implementation of topic validation in Dapr's gRPC has limitations when using wildcards with disable_topic_validation.

Improvement:

Allow topic-level callbacks to be registered even when disable_topic_validation is enabled. Support wildcard matching for topics during validation when disable_topic_validation is disabled.

Example:

With the current implementation, a subscription to a/+ will not receive messages for topic a/b when disable_topic_validation is disabled. With the proposed improvement, the subscription to a/+ will receive messages for topic a/b when disable_topic_validation is disabled.

joohyung-park avatar Feb 26 '24 03:02 joohyung-park