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

Fix rendering of events in single message view. #6156

Merged
merged 1 commit into from Jul 18, 2019

Conversation

@dennisoelkers
Copy link
Member

commented Jul 17, 2019

Description

Motivation and Context

Before this change, rendering a message generated by an event using the ShowMessagePage component, e.g. using the /messages/{index}/{id} endpoint failed, due to a missing source input id field in the event message.

This change is fixing this and a couple of other issues with rendering event messages.

Fixes #6142.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Fix rendering of events in single message view.
Before this change, rendering a message generated by an event using the
`ShowMessagePage` component, e.g. using the `/messages/{index}/{id}`
endpoint failed, due to a missing source input id field in the event
message.

This change is fixing this and a couple of other issues with rendering
event messages.

Fixes #6142.

@dennisoelkers dennisoelkers requested a review from bernd Jul 17, 2019

@dennisoelkers dennisoelkers changed the title 3.1.0 Fix rendering of events in single message view. Jul 17, 2019

@dennisoelkers dennisoelkers added this to the 3.1.0 milestone Jul 17, 2019

@bernd bernd self-assigned this Jul 17, 2019

@bernd
Copy link
Member

left a comment

The fix works for me! 👍

I noticed that we don't show the "Routed into streams" part when showing a single message. This was broken before and has nothing to do with this PR.

I tried to fix it and came up with the following diff. The issue is that the MessageResource directly returns the ResultMessage object instead of the ResultMessageSummary. Due to that the resource is serializing the regular Message object to JSON. (🙈) Including all internal fields. lol

Because of that the stream detection doesn't work (streams vs stream_ids field) and the "Routed into streams" doesn't show up.

This is the diff I was testing with. Please let me know if you want to include this fix with this PR or if I should open a new PR. Thanks!

diff --git graylog2-server/src/main/java/org/graylog2/rest/resources/messages/MessageResource.java graylog2-server/src/main/java/org/graylog2/rest/resources/messages/MessageResource.java
index d8518c7fa..7ceb10d6a 100644
--- graylog2-server/src/main/java/org/graylog2/rest/resources/messages/MessageResource.java
+++ graylog2-server/src/main/java/org/graylog2/rest/resources/messages/MessageResource.java
@@ -40,6 +40,7 @@ import org.graylog2.plugin.inputs.codecs.Codec;
 import org.graylog2.plugin.journal.RawMessage;
 import org.graylog2.rest.models.messages.requests.MessageParseRequest;
 import org.graylog2.rest.models.messages.responses.MessageTokens;
+import org.graylog2.rest.models.messages.responses.ResultMessageSummary;
 import org.graylog2.shared.rest.resources.RestResource;
 import org.graylog2.shared.security.RestPermissions;
 import org.slf4j.Logger;
@@ -92,17 +93,17 @@ public class MessageResource extends RestResource {
             @ApiResponse(code = 404, message = "Specified index does not exist."),
             @ApiResponse(code = 404, message = "Message does not exist.")
     })
-    public ResultMessage search(@ApiParam(name = "index", value = "The index this message is stored in.", required = true)
-                                @PathParam("index") String index,
-                                @ApiParam(name = "messageId", required = true)
-                                @PathParam("messageId") String messageId) throws IOException {
+    public ResultMessageSummary search(@ApiParam(name = "index", value = "The index this message is stored in.", required = true)
+                                       @PathParam("index") String index,
+                                       @ApiParam(name = "messageId", required = true)
+                                       @PathParam("messageId") String messageId) throws IOException {
         checkPermission(RestPermissions.MESSAGES_READ, messageId);
         try {
             final ResultMessage resultMessage = messages.get(messageId, index);
             final Message message = resultMessage.getMessage();
             checkMessageReadPermission(message);
 
-            return resultMessage;
+            return ResultMessageSummary.create(resultMessage.highlightRanges, resultMessage.getMessage().getFields(), resultMessage.getIndex());
         } catch (DocumentNotFoundException e) {
             final String msg = "Message " + messageId + " does not exist in index " + index;
             LOG.error(msg, e);
@@ -138,7 +139,7 @@ public class MessageResource extends RestResource {
             @ApiResponse(code = 400, message = "Could not decode message.")
     })
     @NoAuditEvent("only used to parse a test message")
-    public ResultMessage parse(@ApiParam(name = "JSON body", required = true) MessageParseRequest request) {
+    public ResultMessageSummary parse(@ApiParam(name = "JSON body", required = true) MessageParseRequest request) {
         Codec codec;
         try {
             final Configuration configuration = new Configuration(request.configuration());
@@ -151,8 +152,9 @@ public class MessageResource extends RestResource {
 
         final RawMessage rawMessage = new RawMessage(0, new UUID(), Tools.nowUTC(), remoteAddress, request.message().getBytes(StandardCharsets.UTF_8));
         final Message message = decodeMessage(codec, remoteAddress, rawMessage);
+        final ResultMessage resultMessage = ResultMessage.createFromMessage(message);
 
-        return ResultMessage.createFromMessage(message);
+        return ResultMessageSummary.create(resultMessage.highlightRanges, resultMessage.getMessage().getFields(), resultMessage.getIndex());
     }
 
     private Message decodeMessage(Codec codec, ResolvableInetSocketAddress remoteAddress, RawMessage rawMessage) {
diff --git graylog2-web-interface/src/logic/message/MessageFormatter.js graylog2-web-interface/src/logic/message/MessageFormatter.js
index 2f17d1653..f1108cfa6 100644
--- graylog2-web-interface/src/logic/message/MessageFormatter.js
+++ graylog2-web-interface/src/logic/message/MessageFormatter.js
@@ -4,25 +4,20 @@ import MessageFieldsFilter from 'logic/message/MessageFieldsFilter';
 const MessageFormatter = {
   formatMessageSummary(messageSummary) {
     const { message } = messageSummary;
-    return this.formatMessage(message._id, messageSummary.index, message, message, messageSummary.highlight_ranges, messageSummary.decoration_stats);
+    return this.formatMessage(message._id, messageSummary.index, message, messageSummary.highlight_ranges, messageSummary.decoration_stats);
   },
 
-  formatResultMessage(resultMessage) {
-    const { message } = resultMessage;
-    return this.formatMessage(message.id, resultMessage.index, message, message.fields, resultMessage.highlight_ranges, resultMessage.decoration_stats);
-  },
-
-  formatMessage(id, index, message, fields, highlightRanges, decorationStats) {
-    const filteredFields = MessageFieldsFilter.filterFields(fields);
+  formatMessage(id, index, message, highlightRanges, decorationStats) {
+    const filteredFields = MessageFieldsFilter.filterFields(message);
     return {
       id: id,
       timestamp: moment(message.timestamp).unix(),
       filtered_fields: filteredFields,
       formatted_fields: filteredFields,
-      fields: fields,
+      fields: message,
       index: index,
-      source_node_id: fields.gl2_source_node,
-      source_input_id: fields.gl2_source_input,
+      source_node_id: message.gl2_source_node,
+      source_input_id: message.gl2_source_input,
       stream_ids: message.streams,
       highlight_ranges: highlightRanges,
       decoration_stats: decorationStats,
diff --git graylog2-web-interface/src/stores/messages/MessagesStore.js graylog2-web-interface/src/stores/messages/MessagesStore.js
index 68e6e6990..f7a9143a9 100644
--- graylog2-web-interface/src/stores/messages/MessagesStore.js
+++ graylog2-web-interface/src/stores/messages/MessagesStore.js
@@ -23,7 +23,7 @@ const MessagesStore = Reflux.createStore({
     const { url } = ApiRoutes.MessagesController.single(index.trim(), messageId.trim());
     const promise = fetch('GET', URLUtils.qualifyUrl(url))
       .then(
-        response => MessageFormatter.formatResultMessage(response),
+        response => MessageFormatter.formatMessageSummary(response),
         (errorThrown) => {
           UserNotification.error(`Loading message information failed with status: ${errorThrown}`,
             'Could not load message information');
@@ -58,7 +58,7 @@ const MessagesStore = Reflux.createStore({
 
     const promise = fetch('POST', URLUtils.qualifyUrl(url), payload)
       .then(
-        response => MessageFormatter.formatResultMessage(response),
+        response => MessageFormatter.formatMessageSummary(response),
         (error) => {
           if (error.additional && error.additional.status === 400) {
             UserNotification.error('Please ensure the selected codec and its configuration are right. '
@dennisoelkers

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

@bernd: Good catch. I noticed the missing streams list for a moment, but thought immediately that it must be a problem with my local installation, because otherwise someone else would have noticed it already. 🤦‍♂

I would prefer to do it in a separate PR for cleanliness. Do you have time to do that?

@bernd

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@bernd: Good catch. I noticed the missing streams list for a moment, but thought immediately that it must be a problem with my local installation, because otherwise someone else would have noticed it already.

I would prefer to do it in a separate PR for cleanliness. Do you have time to do that?

I will just put my diff into a separate PR, no problem. 😃

@bernd
bernd approved these changes Jul 18, 2019

@bernd bernd merged commit b221c7a into master Jul 18, 2019

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 3921 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 4744 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the fix-rendering-of-events-with-show-message-page branch Jul 18, 2019

@bernd

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@dennisoelkers PR is here: #6160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.