Kafka topics should not contain special characters #492
Conversation
This is my observation while playing with createTopics function. I later realised that topic names which I am creating contains special characters such as :,// etc which is causing the issue. I tried without them and it works. It does work with other number and string combinations so no test required for that. Hope this PR is help.
Fixes for build failure SOHU-Co#1.
Fixed for build SOHU-Co#2.
Fixes for build SOHU-Co#3.
| @@ -349,12 +349,25 @@ Client.prototype.loadMetadataForTopics = function (topics, cb) { | |||
| }; | |||
|
|
|||
| Client.prototype.createTopics = function (topics, isAsync, cb) { | |||
| function isValid (str) { | |||
There was a problem hiding this comment.
Maybe we could use validateConfig in lib/utils.js?. And it's might be easier to just check for legal characters than invalid ones. Check out the validation used in Topic.scala
Bonus points for:
- Check length
- Add topic validation to
Consumer,HighLevelConsumer, andConsumerGroup
There was a problem hiding this comment.
Sure. Make sense.
| }); | ||
|
|
||
| if (!validTopics) { | ||
| return cb(null, 'Some topics contains special characters, please remove them'); |
There was a problem hiding this comment.
Lets continue with the error first style and call the callback with a new Error as the first argument.
| topics = typeof topics === 'string' ? [topics] : topics; | ||
|
|
||
| if (typeof isAsync === 'function' && typeof cb === 'undefined') { | ||
| cb = isAsync; | ||
| isAsync = true; | ||
| } | ||
| var validTopics = true; | ||
| Object.keys(topics).map(function (elem) { |
There was a problem hiding this comment.
Array.some could be used here and may not need to iterate through the entire list.
|
@hyperlink I did made the change and created a function in utils. To pass this build, you need to merge the pull no 17. Sorry for confusion. |
Updating the name of function as per changes in pull req no 17.
|
Seems it passed the checks, please merge. |
| @@ -355,8 +356,8 @@ Client.prototype.createTopics = function (topics, isAsync, cb) { | |||
| cb = isAsync; | |||
| isAsync = true; | |||
| } | |||
| validateKafkaTopics(topics); | |||
There was a problem hiding this comment.
Just realized something. if this fails validation and isAsync is true then user will never be notified of the error through the callback. I think we should change this to account for that case.
There was a problem hiding this comment.
I used Error() object with throw keyword in the validateKafkaTopics topics function. So even though isAsync is true, error will be thrown.
There was a problem hiding this comment.
When users call it as async method they will expect to catch errors in the callback. So this will end up being an uncaught exception which is unexpected. We should wrap it in a try catch and if it's isAsync is true call the callback with the error otherwise throw it again.
There was a problem hiding this comment.
I am not getting it completely. something like this ?
try {
validateKafkaTopics(topics);
} catch(e) {
return isAsync ? cb(null,e) : throw new Error(e);
}There was a problem hiding this comment.
I was thinking of this:
try {
validateKafkaTopics(topics);
} catch (e) {
if (isAsync) return cb(e);
throw new e;
}That way using async you can expect the normal error first callback.
There was a problem hiding this comment.
Cool. Make sense. Updating it.
Build fix.
Build fix.
| validateKafkaTopics(topics); | ||
| } catch (e) { | ||
| if (isAsync) return cb(e); | ||
| throw new e; |
There was a problem hiding this comment.
validateTopicNames throws a new instance so there's no need to new the error here. Looks good otherwise.
For build fix.
|
Thanks. |
This is my observation while playing with createTopics function. I later realised that topic names which I am creating contains special characters such as :,// etc which is causing the issue. I tried without them and it works. It does work with other number and string combinations so no test required for that.
Hope this PR is help.