added ANTLR visitor for parsing Logstash configuration

[asifsmohammed]

Description

• Added Logstash visitor which populates model POJO's
• Added Exception classes for converter
• Updated checkstyle file to suppress checks for ANTLR generated files

Issues Resolved

Resolve #465

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[sshivanii]

I'd like the javadoc to be more descriptive about what is Visitor? You can update this to LogstashVisitor and in that mention what the visitor class does.

[sshivanii]

can we rename this to LogstashParser.ConfigContext configContext?

[asifsmohammed]

Yeah, I can rename the parameters.

[sshivanii]

Sidenote: The first commit for this PR e56cf3ee9fd2beb3e1234731d0b4b4749941f1d5 is a Merge commit. Based on the coding guidance we discussed, we're opting to Rebase instead of Merge to avoid any unnecessary Merge commits.

[cmanning09]

nit: there is an extra *

[cmanning09]

This logic belongs in LogstashPluginType enum as it is related to the creation of an enum. We can also eliminate the need for a case statement by using a map.

I would recommend updating the enum like so:

public enum LogstashPluginType {
    INPUT("input"),
    FILTER("filter"),
    OUTPUT("output");

    private final String value;

    private static final Map<Stirng, LogstashPlugingType> VALUES_MAP = Arrays.stream(LogstashPlugingType.values())
            .collect(Collectors.toMap(LogstashPlugingType::toString, Function.identity()));

    LogstashPlugingType(final String value) {
        this.value = value;
    }

    public String toString() {
        return value;
    }

    public static LogstashPlugingType getByValue(final String value) {
        return VALUES_MAP.get(name.toLowerCase());
    }
}

Then you can simplify this to:

final LogstashPluginType logstashPluginType = LogstashPlugingType.getByValue(pluginSectionContext.plugin_type().getText());

[cmanning09]

Let's use camel-case for all our grammar definitions (for example: pluginSection) This would result in method signatures with a single casing:

public Object visitPluginSsection(LogstashParser.PluginSectionContext pluginSectionContext) {

[dlvenable]

I think this raises a question as to what convention we should use in the Grammar - follow the convention from Logstash, which would make it easier to map back to it; or use conventions which make the Java code nicer. This code should be used exclusively by the Logstash configuration framework, so I'm fine with using the Logstash conventions.

[cmanning09]

Ahh okay. given this maps directly and is a manual process I am onboard with keep this as is to assist in the manual mapping process. Thanks for clarifying.

[cmanning09]

These lists and maps don't need to be declared globally. We can use the methods as intended and return the constructed lists over mutating and reassigning the objects. This will simplify the code:

    @Override
    public Object visitConfig(final LogstashParser.ConfigContext configContext) {

        final Map<LogstashPluginType, List<LogstashPlugin>> pluginSections = new LinkedHashMap<>();

        configContext.plugin_section().forEach(pluginSection -> {
            final LogstashPluginType logstashPluginType = LogstashPluginType.getByValue(pluginSectionContext.plugin_type().getText();
            final List<LogstashPlugin> logstashPluginList = (List<LogstashPlugin>) visitPlugin_section(pluginSection);
            pluginSections.put(logstashPluginType, logstashPluginList);
                }
        );
        return LogstashConfiguration.builder()
                .pluginSections(pluginSections)
                .build();
    }

    @Override
    public Object visitPlugin_section(final LogstashParser.Plugin_sectionContext pluginSectionContext) {

        return pluginSectionContext.branch_or_plugin().stream()
                .map(this::visitBranch_or_plugin)
                .collect(Collectors.toList());
        });
    }

(see the comment on visitHashEntries() below)

[cmanning09]

This is strange. I am not sure if there is a way around it but attributes is an AttributeContext while attribute is a list of AttributeContexts...

Is this an issue with our grammar?

[asifsmohammed]

So the attributes will have list of attribute in grammar.

attributes

attribute 1
attribute 2

We are storing each attribute from configuration in a list of type LogstashAttribute.

[cmanning09]

hashEntry

[cmanning09]

Extending off what I said earlier to eliminate the use of the shared class value:

    @Override
    public Object visitHashentries(LogstashParser.HashentriesContext hashentriesContext) {

        final Map<String, Object> hashEntries = new LinkedHashMap<>();

        hashentriesContext.hashentry().forEach(hashentryContext -> {
            final String key = hashentryContext.hashname().getText();
            final Object value = visitHashentry(hashentryContext);
            hashEntries.put(key, value);
        });
        return hashEntries;
    }

    @Override
    public Object visitHashentry(LogstashParser.HashentryContext hashentryContext) {

        if (hashentryContext.value().getChild(0) instanceof LogstashParser.ArrayContext)
            return visitArray(hashentryContext.value().array());

        return hashentryContext.value().getText();
    }

[cmanning09]

It appears you are mocking the same objects over and over again in each test. I would encourage you to use @Mock annotation and create a class variable to reuse. I would also encourage you to create a LogstashVisitor class variable to assign in a @BeforeEach function as well. This will help reduce the duplicate code you have.

[cmanning09]

This name is not very descriptive. What are we testing for here?

[cmanning09]

Can you add the following to the documentation? I am not familiar with ANTLR and how this whole process works. It took me a little while to understand where the LogstashBaseVisitor is coming from. I didn't realize I had to build my code to generate the class.

• What is a LogstashBaseVisitor?
• How is a LogstashBaseVisitor generated?
• Where do all the methods come from?

[dlvenable]

While this is confusing in a PR, this is all part of the build process and I think detailed documentation here is not quite appropriate. If anything, I'd keep it as simple as "... using ANTLR library classes and generated code."

[cmanning09]

That works. Anything to point the reader in the right direction.

[dlvenable]

Please remove extra lines between Javadocs and their classes/methods. I found this one, but there may be others.

[dlvenable]

This should be package private so that it is not used outside of this work.

[dlvenable]

This class should also be package private.

[dlvenable]

I think this raises a question as to what convention we should use in the Grammar - follow the convention from Logstash, which would make it easier to map back to it; or use conventions which make the Java code nicer. This code should be used exclusively by the Logstash configuration framework, so I'm fine with using the Logstash conventions.

[dlvenable]

I'm a big fan of {@link}, but I suggest removing it here. The LogstashVisitor is an internal implementation detail. Also, exceptions should generally not declare which classes use them. It goes the other way: classes specify which exceptions they throw.

I suggest rewording this somewhat.

[cmanning09]

We can mark every parameter for all methods implemented in this class as final

[cmanning09]

final

[cmanning09]

This exception is never used. Will it be used in the future?

[asifsmohammed]

Yes. The intention was to use it in the future.