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

PubSub disable_topic_validation=True breaks multiple subscriptions #436

Open
ngiind opened this issue Aug 1, 2022 · 5 comments
Open

PubSub disable_topic_validation=True breaks multiple subscriptions #436

ngiind opened this issue Aug 1, 2022 · 5 comments
Labels
help wanted Extra attention is needed

Comments

@ngiind
Copy link

ngiind commented Aug 1, 2022

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
@berndverst
Copy link
Member

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

Perhaps it is worth to leave a comment here again instead dapr/dapr#4461 since this is the main tracking issue.

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

@ngiind
Copy link
Author

ngiind commented Aug 4, 2022

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

@berndverst berndverst added the help wanted Extra attention is needed label Sep 19, 2022
@berndverst berndverst added this to the v1.9 milestone Oct 17, 2022
@berndverst
Copy link
Member

How do you expect disable_topic_validation to work @ngiind ?

@ngiind
Copy link
Author

ngiind commented Jan 25, 2023

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 dapr/dapr#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.

@berndverst berndverst modified the milestones: v1.9, v1.10 Jan 31, 2023
@berndverst berndverst modified the milestones: v1.10, v1.11 May 30, 2023
@berndverst berndverst modified the milestones: v1.11, v1.13 Oct 31, 2023
@berndverst berndverst removed this from the v1.13 milestone Feb 13, 2024
@joohyung-park
Copy link

joohyung-park commented Feb 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants