Add method to ensure a valid topic name #1238
Conversation
kafka/consumer/subscription_state.py
Outdated
| if not isinstance(topic, six.string_types): | ||
| raise TypeError('All topics must be strings') | ||
| if len(topic) == 0: | ||
| raise TypeError('All topics must be non-empty strings') |
There was a problem hiding this comment.
I'd probably make the bottom 4 exceptions ValueError. (The first two make sense w/ TypeError)
|
Awesome! |
There was a problem hiding this comment.
Thanks for this. A few comments on the docstrings.
What do you think about adding a test for this new method as well, I would think it'd be very straightforward, especially if you use pytest.mark.parametrize()... ?
kafka/consumer/subscription_state.py
Outdated
| """ Ensures that the topic name is valid according to the kafka source. | ||
|
|
||
| Raises: | ||
| TypeError: if the topic is |
There was a problem hiding this comment.
nit: now that you're raising ValueError, this docstring should also be updated. Either clarify what you raise, or just say that you raise on X condidtions, without specifying what gets raise (I'm fine with either given that this is a somewhat buried docstring, other maintainers may have other opinions).
There was a problem hiding this comment.
I tried to adapt to the style of the stuff of the project that I removed (see this).
So I'd remove the description of why a ValueError/TypeError is thrown. Is this okay?
There was a problem hiding this comment.
I usually only add a docstring to a private method if it isn't obvious from the code what or why it is doing. The audience for private methods is primarily developers with access to the source code. But the public methods end up getting pulled out into HTML docs on kafka-python.readthedocs.org , so those are much more important.
There was a problem hiding this comment.
@dpkp right now the SubscriptionState docs are not visible on http://kafka-python.readthedocs.io/en/master/apidoc/modules.html, do you want to change that?
I'm fine with dropping the docstring from the private method, it reduces the risk of the docstring going out of sync with the code.
kafka/consumer/subscription_state.py
Outdated
| - a non-str or | ||
| - an empty string or | ||
| - the topic name is '.' or '..' or | ||
| - the topic name does not consist of ASCII-characters/'-'/'_'/'.' |
There was a problem hiding this comment.
I would probably entirely remove this part of the docstring since the raising for invalid values now happens in _ensure_valid_topic_name()
There was a problem hiding this comment.
The internal method raises something. Shouldn't this be documented in the public method that uses this method (since it uses it only for that)? The checks are only encapsulated in a separate method to make it easier to reuse the checks later on.
I would not remove these raise-Docstrings but update them accordingly (matching the style mentioned above).
There was a problem hiding this comment.
I would keep them in the docstring here, but update re ValueError
|
@dpkp @jeffwidman Can you take another look on this? |
|
thanks! |
As described in #961, the consumer does not check whether topic names are valid. It does however check if the topic name is a string.
This PR adds a check according to the current kafka source:
https://github.com/apache/kafka/blob/39eb31feaeebfb184d98cc5d94da9148c2319d81/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L29
That means:
.or..a-z,A-Z, '0-9',.,-and_