JSON APIs and bug fixes #717
Conversation
…ges (json api for viewAllMessages)
…lh) 2. sorted messages in searchMessages
Bert-R
left a comment
Bert-R
left a comment
There was a problem hiding this comment.
@ndanilin Thank you for this PR and sorry for the long delay in reviewing it.
It generally looks good. I have added a few comments regarding imports and code duplication. After these are resolved, the PR can be merged. Then we'll bring a release of Kafdrop.
| import kafdrop.model.TopicPartitionVO; | ||
| import kafdrop.model.TopicVO; | ||
| import kafdrop.form.SearchMessageFormForJson; | ||
| import kafdrop.model.*; |
There was a problem hiding this comment.
In our coding convention, we don't use wildcard imports. Please restore the original specific imports.
|
|
||
|
|
||
| import kafdrop.util.Serializers; | ||
| import kafdrop.util.*; |
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import kafdrop.model.CreateMessageVO; | ||
| import java.io.File; | ||
| import java.util.*; |
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
| import org.springframework.web.bind.annotation.ResponseBody; | ||
| import org.springframework.web.bind.annotation.*; |
| final var deserializers = new Deserializers( | ||
| getDeserializer(topicName, defaultKeyFormat, "", "", protobufProperties.getParseAnyProto()), | ||
| getDeserializer(topicName, defaultFormat, "", "", protobufProperties.getParseAnyProto())); | ||
|
|
||
| final List<MessageVO> messages = new ArrayList<>(); | ||
|
|
||
| for (TopicPartitionVO partition : topic.getPartitions()) { | ||
| messages.addAll(messageInspector.getMessages(topicName, | ||
| partition.getId(), | ||
| partition.getFirstOffset(), | ||
| size, | ||
| deserializers)); | ||
| } | ||
|
|
||
| messages.sort(Comparator.comparing(MessageVO::getTimestamp)); |
There was a problem hiding this comment.
This duplicates the lines 102-116 of viewAllMessages. Extract these into a private method and call that method here instead of duplicating them.
| final var deserializers = new Deserializers( | ||
| getDeserializer(topicName, searchMessageForm.getKeyFormat(), null, null, | ||
| protobufProperties.getParseAnyProto()), | ||
| getDeserializer(topicName, searchMessageForm.getFormat(), null, null, | ||
| protobufProperties.getParseAnyProto()) | ||
| ); | ||
|
|
||
| var searchResultsVO = kafkaMonitor.searchMessages( | ||
| topicName, | ||
| searchMessageForm.getSearchText(), | ||
| searchMessageForm.getPartition(), | ||
| searchMessageForm.getMaximumCount(), | ||
| searchMessageForm.getStartTimestamp(), | ||
| deserializers); |
There was a problem hiding this comment.
These lines duplicate the lines 325-339 of searchMessageForm, except that descFile and msgTypeName are set to null. Extract the lines into a method that takes descFile and msgTypeName as parameters and call the method here and above.
| private String message; | ||
| private String key; | ||
| private Map<String, String> headers; | ||
| @JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss.SSS") |
There was a problem hiding this comment.
Why this format instead of ISO 8601?
There was a problem hiding this comment.
@Bert-R it's the same as for "SearchMessageForm" and "SearchMessageFormForJson" (my comment below)
|
@ndanilin Thanks for the import rearrangement! Any chance you get to addressing the other comments soon? |
| private MessageFormat keyFormat; | ||
| @Schema(type = "string", example = "1970-01-01 03:00:00.000") | ||
| @DateTimeFormat(pattern = "yyyy-MM-dd HH:mm:ss.SSS") | ||
| @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd HH:mm:ss.SSS") |
There was a problem hiding this comment.
@ndanilin Why this format instead of the regular ISO 8601 format?
There was a problem hiding this comment.
Yes, please change it to the ISO format. That's a better fit for an API.
There was a problem hiding this comment.
@Bert-R
fixed this moment:
- APIs work with iso format (input and output)
- UI still works with "yyyy-MM-dd HH:mm:ss.SSS" (haven't dealt with ftlh files before it, so check please)
There was a problem hiding this comment.
@ndanilin Sorry, that's not what I meant. My comment only applied to the JSON API. Just this change should suffice:
- @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd HH:mm:ss.SSS")
+ @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'", timezone = "UTC")There was a problem hiding this comment.
@Bert-R sorry to complicate your idea.
- canceled a previous commit
- replaced the "searchMessages" API with the POST method (because GET doesn't work with @JSON format)
- corrected documentation for "searchMessages"
- changed @jsonformat to "SearchMessageFormForJson" and "MessageVO", as you said
|
@ndanilin I done the deduplication that I asked for earlier. Can you have a look to see whether the API still matches your expectations? |
|
@Bert-R I'm really sorry that I forgot to push the second part of my changes about code duplication. Thank you so much for your help. |
Bert-R
left a comment
Bert-R
left a comment
There was a problem hiding this comment.
I've corrected the imports and wrapped the lines that were longer than our rules. With that the change is OK.
Thanks for the contribution!
2 participants
Bug fixes:
viewAllMessages(I think it's a critical bug).searchRecords. Search messages by timestamp didn't work becauseseekToTimestampcan't guarantee needed result.New features:
getAllMessages(equivalent ofviewAllMessagesthat returns JSON array of messages).searchMessages(equivalent ofsearchMessageFormthat returns JSON array of messages). Here I made customSearchMessageFormForJsonand now all fields of form are not required. Also added here search by keys (It might overload this method, but seems very helpful for guaranteed results)