Adding hyperlinked fields in search result details #4669

pbr0ck3r · 2018-03-19T16:40:32Z

Description

This PR allows graylog to have hyperlinked search result fields. A user can create a decorator for a field that they want to allow to be a hyperlink.

Closes this issue/feature request: #1914

Motivation and Context

How Has This Been Tested?

I built graylog via mvn compile
I then ran the frontend and backend servers.
Tested saving config with several fields, not fields, etc.
Made sure that the search page details rendered accordingly.

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.

joschi · 2018-03-19T17:11:44Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

@@ -54,6 +55,13 @@ const MessageFieldDescription = React.createClass({

    return termsMarkup;
  },
+  _getFieldValue() {
+    if (this.props.linkedFields.includes(this.props.fieldName)) {
+      return <a href={this.props.possiblyHighlight(this.props.fieldName)}>{this.props.possiblyHighlight(this.props.fieldName)}</a>


You probably don't want to use the highlighted version of the field name as hyperlink reference. 😉

Good catch. Will make that fix.

joschi · 2018-03-23T14:11:15Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

@@ -54,6 +55,13 @@ const MessageFieldDescription = React.createClass({

    return termsMarkup;
  },
+  _getFieldValue() {
+    if (this.props.linkedFields.includes(this.props.fieldName)) {
+      return <a href={this.props.fieldName}>{this.props.fieldName}</a>


Ah, now you're missing the highlight in the content of the <a> tag. 😉

return <a href={this.props.fieldName}>{this.props.possiblyHighlight(this.props.fieldName)}</a>

Fixed this!

joschi · 2018-03-23T14:11:47Z

graylog2-server/src/main/java/org/graylog2/indexer/searches/SearchesClusterConfig.java

@@ -63,6 +63,8 @@
            .add("file")
            .add("source_file")
            .build();
+    private static final Set<String> DEFAULT_LINKED_SEARCH_FIELDS = ImmutableSet.<String>builder()


You could simply write ImmutableSet.of() or Collections.emptySet().

joschi · 2018-03-23T15:24:57Z

graylog2-server/src/main/java/org/graylog2/indexer/searches/SearchesClusterConfig.java

@@ -63,6 +63,7 @@
            .add("file")
            .add("source_file")
            .build();
+    private static final Set<String> DEFAULT_LINKED_SEARCH_FIELDS = Collections.emptySet();


graylog2-server/graylog2-server/src/main/java/org/graylog2/indexer/searches/SearchesClusterConfig.java:66: error: cannot find symbol private static final Set<String> DEFAULT_LINKED_SEARCH_FIELDS = Collections.emptySet();

This won't work unless you import the Collections class.

edmundoa · 2018-04-02T14:22:03Z

Hi @pbr0ck3r,

In first place, thank you very much for your contribution! 🙂

We would love to add this feature to the product, but we have been discussing a bit about the implementation and we think this would be a better fit for a frontend decorator. Would you be willing to rework the feature so it uses a decorator to convert a field into a link?

Please also create future PRs based on the current master branch, as that contains the latest code under development, making it easier to integrate your changes into the product. We also do not add new features to version branches, they are kept to let people build a certain Graylog version exactly as it was. Thank you!

pbr0ck3r · 2018-04-02T14:29:35Z

Sorry I ddin't branch off master. I was using the gitlab-cli which had me by default on 2.4 branch. I will branch off master and push the new code to this PR when I have time to re-write it in the coming weeks.

pbr0ck3r · 2018-04-02T14:56:39Z

I am assuming you all want me to make a decorator plugin that I would maintain myself. If so then this PR can then be closed.

edmundoa · 2018-04-02T17:10:07Z

@pbr0ck3r we are happy to merge the decorator in our code and maintain it (we already have a few decorators)! I'll keep the PR open if you want to work on it. You are of course free to create a plugin with the decorator if you want to.

pbr0ck3r · 2018-04-02T19:05:11Z

How would users specify fields they want decorated? Currently I have a list in the configuration page that allows users to specific fields. In a decorator, that list would have to be hard coded in the config. Since decorators don't have any UI components, how would a user change the fields?

pbr0ck3r · 2018-04-02T19:08:20Z

Never mind... forgot how decorators work. A user can specify the field they want decorated in the pipeline rule. I will work on refactoring this code.

pbr0ck3r · 2018-04-02T22:26:48Z

@edmundoa After starting to refactor my code into a Decorator. I noticed that even if I create a decorator to make a field a link. It does not render as a link. If you look at the current PR I had to modify the MessageField.jsx file in order to get fields to render as hyperlinks (anchor tags). From the decorator even if I pass back a anchor tag for the field, react does not render it as that. I just renders it as a string.

edmundoa · 2018-04-03T10:42:55Z

@pbr0ck3r decorators are not related to pipeline rules (except for Pipelines Processor Decorator), and are not configured in pipeline rules, but in the search (or stream search) UI:

I would start by creating a new decorator in Java, i.e. Link Field Converter, that takes a single field as parameter and converts it into something we can use to render an anchor with the field value used as text and href of the link. In the future we could allow users to customise the title or href of the link, maybe using other fields in the message, but I would leave it out of the first version.

As you said, React will not set any strings as HTML for security reasons. Formatting the link as HTML in the backend and rendering it with dangerouslySetInnerHTML would be an option, but for me it still feels too insecure as that will interpret any HTML in the field and potentially do very bad things.

I think it would be safer to use React.createElement() to create the anchor element and set it's href and text, as it will at least escape the children and (potentially) only execute code when you click on the link. An example of rendering a link with createElement would be: React.createElement('a', { href: 'http://example.org' }, 'http://example.org'). So, the Link Field Converter could provide an object like {decoratedField: {type: 'a', props: { href: 'http://example.org' }, children: 'http://example.org' }} that we can use to call to createElement with the right arguments and create the link.

What do you think?

pbr0ck3r · 2018-04-03T13:04:17Z

@edmundoa before I go and refactor my frontend react code. Is it fine for to be modifying the MessageField.jsx to use React.createElement instead of just creating a anchor tag like I was doing? I see you all's point of making a decorator instead of the configuration list I had before.

edmundoa · 2018-04-05T09:52:01Z

@pbr0ck3r I'm not sure if I understood what you meant, so please ask again if it's not clear :) Modifying MessageField is necessary to go with either option, so it's fine changing it. As I commented before, I think using React.createElement would be a safer choice than creating the anchor tag directly from the message field as innerHTML. It would also enable other decorators to create other kind of elements as well.

pbr0ck3r · 2018-04-06T18:39:16Z

@edmundoa.. I have pushed up the new LinkFieldConvertor decorator and also made the changes we discussed to have the reactjs use React.createElement.

edmundoa · 2018-04-18T08:54:19Z

@pbr0ck3r sorry it's taking so long for me to review this, I hope I can make by the end of this week. Thank you for your patience!

edmundoa

I was testing the changes and it looks really good, great job @pbr0ck3r! 👍
Inline there are a couple of improvements and linting errors I found that should be fixed before merging the changes.

edmundoa · 2018-04-19T11:44:39Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldConverter.java

+    }
+
+    @Inject
+    public LinkFieldConverter(@Assisted Decorator decorator, Engine templateEngine) {


We don't use the templateEngine parameter and we can remove it from here :)

edmundoa · 2018-04-19T11:47:47Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldConverter.java

+
+import static java.util.Objects.requireNonNull;
+
+public class LinkFieldConverter implements SearchResponseDecorator {


Small nitpick: I would append Decorator to the class name, making it easier to identify it, and also keeping consistency with other existing decorators.

edmundoa · 2018-04-19T11:55:17Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldConverter.java

+        final List<ResultMessageSummary> summaries = searchResponse.messages().stream()
+                .map(summary -> {
+                    final Message message = new Message(ImmutableMap.copyOf(summary.message()));
+                    if (summary.message().containsKey(linkField)) {


I think it would be better to revert this condition and short-circuit the map method to avoid doing unnecessary work. Something like:

.map(summary -> { if (!summary.message().containsKey(linkField)) { return summary; } ... })

In that way we avoid creating a new message and then add its fields into the summary object, since we didn't decorate the message in this case.

edmundoa · 2018-04-19T12:04:13Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldConverter.java

+                    final Message message = new Message(ImmutableMap.copyOf(summary.message()));
+                    if (summary.message().containsKey(linkField)) {
+                        final String href = (String) summary.message().get(linkField);
+                        final LinkedHashMap object = new LinkedHashMap<String, String>();


Here I see a couple of improvements:

Use a regular HashMap, as we don't really care to maintain order in this case, and it's going to be a bit faster

Use generics in the local variable you are storing the Map, avoiding unchecked calls to put() afterwards

The name object is too generic for my taste, I would go with decoratedField or something similar

In summary, I would change this line into:

final Map<String, String> decoratedField = new HashMap<>();

edmundoa · 2018-04-19T12:11:58Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

+      const link = this.props.fieldValue.href;
+      return React.createElement('a', {href: link}, link);
+    } else {
+      return this.props.renderForDisplay(this.props.fieldName)


Here there's a missing semicolon at the end of the line.

edmundoa · 2018-04-19T12:13:14Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

@@ -50,6 +50,15 @@ class MessageFieldDescription extends React.Component {
    SearchStore.addSearchTerm(this.props.fieldName, this.props.fieldValue);
  };

+  _getFieldValue() {


Please use an arrow function here, to ensure the method is bound to this in any case.

edmundoa · 2018-04-19T12:17:07Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

+  _getFieldValue() {
+    if (this.props.isDecorated && ('type' in this.props.fieldValue) && (this.props.fieldValue.type === 'a')) {
+      const link = this.props.fieldValue.href;
+      return React.createElement('a', {href: link}, link);


Please add spaces between the object braces: https://eslint.org/docs/rules/object-curly-spacing

edmundoa · 2018-04-19T12:18:04Z

graylog2-web-interface/src/components/search/MessageFieldDescription.jsx

+    if (this.props.isDecorated && ('type' in this.props.fieldValue) && (this.props.fieldValue.type === 'a')) {
+      const link = this.props.fieldValue.href;
+      return React.createElement('a', {href: link}, link);
+    } else {


The else case is not really needed (since we return in the if branch) and can be deleted.

Fixed all the requested changes.

edmundoa · 2018-04-19T14:08:06Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldDecorator.java

+
+    public interface Factory extends SearchResponseDecorator.Factory {
+        @Override
+        LinkFieldConverter create(Decorator decorator);


There are a bunch of the old LinkFieldConverter class name that must be renamed here and in DecoratorBindings.

edmundoa · 2018-04-19T14:08:49Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldDecorator.java

+import org.graylog2.rest.resources.search.responses.SearchResponse;
+
+import javax.inject.Inject;
+import java.util.LinkedHashMap;


This import is not needed any longer.

edmundoa · 2018-04-19T14:27:41Z

graylog2-server/src/main/java/org/graylog2/decorators/LinkFieldDecorator.java

@@ -30,9 +30,8 @@
 import org.graylog2.rest.resources.search.responses.SearchResponse;

 import javax.inject.Inject;
-import java.util.LinkedHashMap;
-import java.util.List;


List is still used in apply(), so this import should not be removed.

pbr0ck3r · 2018-04-19T15:11:07Z

@edmundoa Thanks for making the time to work through this with me! Much appreciated!

edmundoa · 2018-04-19T15:11:56Z

Thank you very much for your contribution @pbr0ck3r! 🎉

joschi reviewed Mar 19, 2018

View reviewed changes

dennisoelkers changed the title ~~adding hyerlinked fields in search result details~~ Adding hyperlinked fields in search result details Mar 22, 2018

joschi approved these changes Mar 23, 2018

View reviewed changes

joschi suggested changes Mar 23, 2018

View reviewed changes

pbr0ck3r changed the base branch from 2.4 to master April 6, 2018 17:55

added LinkFieldConverter decorator

645b8aa

edmundoa suggested changes Apr 19, 2018

View reviewed changes

refactored code

42bae36

edmundoa suggested changes Apr 19, 2018

View reviewed changes

removed unused imports and renamed fields

3e030f4

edmundoa reviewed Apr 19, 2018

View reviewed changes

re-add missing dependency

172a4a3

edmundoa self-assigned this Apr 19, 2018

edmundoa added this to the 3.0.0 milestone Apr 19, 2018

edmundoa approved these changes Apr 19, 2018

View reviewed changes

joschi approved these changes Apr 19, 2018

View reviewed changes

joschi merged commit 8a1850b into Graylog2:master Apr 19, 2018

edmundoa mentioned this pull request Apr 19, 2018

Link decorator fixes #4742

Merged

tom-zagar mentioned this pull request Dec 16, 2022

Hyperlink a decorated field, or pipeline field #10040

Open


		import static java.util.Objects.requireNonNull;

		public class LinkFieldConverter implements SearchResponseDecorator {

Adding hyperlinked fields in search result details #4669

Adding hyperlinked fields in search result details #4669

Conversation

pbr0ck3r commented Mar 19, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundoa commented Apr 2, 2018

pbr0ck3r commented Apr 2, 2018

pbr0ck3r commented Apr 2, 2018

edmundoa commented Apr 2, 2018

pbr0ck3r commented Apr 2, 2018 • edited Loading

pbr0ck3r commented Apr 2, 2018

pbr0ck3r commented Apr 2, 2018 • edited Loading

edmundoa commented Apr 3, 2018

pbr0ck3r commented Apr 3, 2018

edmundoa commented Apr 5, 2018

pbr0ck3r commented Apr 6, 2018

edmundoa commented Apr 18, 2018

edmundoa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbr0ck3r commented Apr 19, 2018

edmundoa commented Apr 19, 2018

pbr0ck3r commented Mar 19, 2018 •

edited

Loading

pbr0ck3r commented Apr 2, 2018 •

edited

Loading

pbr0ck3r commented Apr 2, 2018 •

edited

Loading

edmundoa left a comment •

edited

Loading