Check ParseTopicName() return err#517
Check ParseTopicName() return err#517EAHITechnology wants to merge 13 commits intoapache:masterfrom EAHITechnology:master
Conversation
Fix parse topic name return err
parses topic and check its return error
| matching := make([]string, 0) | ||
| for _, t := range topics { | ||
| tn, _ := internal.ParseTopicName(t) | ||
| tn, err := internal.ParseTopicName(t) |
There was a problem hiding this comment.
I'm not sure if this good thing to error out here. The list of topic passed here usually comes from a response from the broker, right?
There was a problem hiding this comment.
I'm not sure too.Although this is returned by the broker.
| options.BatchingMaxPublishDelay = defaultBatchingMaxPublishDelay | ||
| } | ||
|
|
||
| ms, err := client.metrics.GetTopicMetrics(options.Topic) |
There was a problem hiding this comment.
This seems like a strange way of testing the topic. I don't think we should rely on calling a metrics function. Why not just call internal.ParseTopicName(topic). Same for the consumer_impl and tests.
There was a problem hiding this comment.
The CreateProducer() passes an unchecked topic.And then use GetTopicMetrics() func (Use ParseTopicName func but uncheck err. When use as object transfer internal members ,you will get a runtime panic)
There was a problem hiding this comment.
If I verify the legitimacy on the outer layer. GetTopicMetrics() passes in *TopicName, is it reasonable? In the same way, the following operations do not need to verify the legitimacy
| var metadata map[string]interface{} | ||
| err := httpGet("admin/v2/persistent/"+topicPath(topic)+"/stats", &metadata) | ||
| tp, err := topicPath(topic) | ||
| if err != nil { |
There was a problem hiding this comment.
Not sure if this is needed since it's a test helper method. The test should fail without this check if a bad topic name is passed. Same for the topicPath function.
There was a problem hiding this comment.
This is just a protective measure to prevent modification of test cases. It's just that I will do it. I'm not sure that others are the same
fix consumer_regex_test return err
…nto fix_ParseTopicName_return_err
Fix parse topic name return err
wolfstudy
left a comment
wolfstudy
left a comment
There was a problem hiding this comment.
Thanks @EAHITechnology work for this, thc change lgtm +1, just a little comments, please check.
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do we need to parse topic name here? In the GetTopicMetrics method, we have called the ParseTopicName method, is it enough to directly handle the error value returned by that method?
|
@EAHITechnology Any update for this? |
|
I think we already tested the topics with |
hello @freeznet In ParseTopicName, we threw an error, but during the call, we ignored the error. such as: in in |
5 participants
No description provided.