lazy load jsonParser and yamlParser FlagFileParser

[jtjeferreira]

I want to be able to use the thin jar without the snakeyaml dependency, since I don't need to parse yaml files, just json files.

If I try that I get the following exception:

Cause: java.lang.ClassNotFoundException: org.yaml.snakeyaml.error.YAMLException
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
at sbt.internal.ManagedClassLoader.findClass(ManagedClassLoader.java:103)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:594)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:527)
at com.launchdarkly.sdk.server.integrations.FileDataSourceParsing$FlagFileParser.<clinit>(FileDataSourceParsing.java:76)
at com.launchdarkly.sdk.server.integrations.FileDataSourceImpl$DataLoader.load(FileDataSourceImpl.java:241)
at com.launchdarkly.sdk.server.integrations.FileDataSourceImpl.reload(FileDataSourceImpl.java:108)
at com.launchdarkly.sdk.server.integrations.FileDataSourceImpl.start(FileDataSourceImpl.java:92)
at com.launchdarkly.sdk.server.LDClient.<init>(LDClient.java:239)

This PR avoids loading the YamlFlagFileParser by using a holder class idiom for lazy initialization of a static field.

Requirements

• I have added test coverage for new or changed functionality
  • Existing tests still pass, but I don't see a way to add unit tests for this functionality. I published this library locally and tested it on my project successfully.
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Related issues

n/a

Describe the solution you've provided

The solution is to lazy load the YamlFlagFileParser (and the JsonFlagFileParser) so they are only initialised if needed

Describe alternatives you've considered

n/a

Additional context

n/a

[tanderson-ld]

Hi @jtjeferreira and thank you for contributing! If we are going to support excluding certain file formats, we should implement a solution that treats JSON and YAML as equals in the design. It should also be impossible at runtime to get to an unsupported parser's code even if another parser is removed from the code (perhaps in a future update).

What do you think of the following approach?

Create list of supported parsers
    If JSON parsing is supported by dependencies, create JSON parser and add to list.
    If YAML parsing is supported by dependencies, create YAML parser and add to list.
for (Parser p : parserList) {
    if (p.isMatch(reader)) {
        return parser
    }

    // error case due to misconfigured project (seems impossible given JSON is required elsewhere in the project).  I'm thinking on if an exception is warranted here.
}

By moving the match logic into the parsers and iterating, all formats are treated as equals and non-supported formats never have their code executed.

We may need to do some optimizing of when the reader is reset / recreated since now this will do a reader pass per format instead of if JSON ? JParser : YParser

[jtjeferreira]

Hi @tanderson-ld

First of all, sorry for the late response.

I think that is an elegant approach, but I think it is a bit over-engineered (it looks like you are proposing a plugin system) for the value of this feature.

It should also be impossible at runtime to get to an unsupported parser's code even if another parser is removed from the code

I think that even with the approach you suggest, we still need to use runtime exceptions for the case when we want to parse YAML, but the yaml parser is not present.

[tanderson-ld]

Hi again @jtjeferreira . We discussed on team briefly and the consensus was the following:

• A whole plugin scheme is probably overkill
• There should be conditional logic to protect against the ClassNotFound exception instead of relying on Java's static initialization behavior.
• We are fine throwing a dedicated exception if an unsupported file is loaded.

Perhaps the Class.forName API could achieve that. Stackoverflow with example

If you would like to iterate with this feedback we can review again once your changes are pushed. Alternatively we can file a ticket for a modification. That would get prioritized according to current work demands and may be some time (likely at least a month).

Let us know your thoughts! Thank you.

[jtjeferreira]

• There should be conditional logic to protect against the ClassNotFound exception instead of relying on Java's static initialization

I don't think we need to worry about the ClassNotFound. The ClassNotFound is just a symptom of the problem that the parsers are instantiated when the FlagFileParser class is loaded

static abstract class FlagFileParser {
    private static final FlagFileParser jsonParser = new JsonFlagFileParser();
    private static final FlagFileParser yamlParser = new YamlFlagFileParser();

and the easiest way that I could think of to make the parsers lazy (but keep them singletons), was to use a "holder class"

[valters]

should be private with a getter so it only gets initialized on access as opposed to having direct reference to variable.