Forbid the single argument Environment constructor in production code

[droberts195]

Only tests should use the single argument Environment constructor.
Production code (beyond initial Bootstrap) should always use the same
Environment object that Node.getEnvironment() returns. This Environment
is also available via dependency injection.

[dakrone]

LGTM assuming CI is happy :)

[jasontedor]

I wonder how much work it would be to move all uses of new Environment(Settings) to a convenience method in the test framework that does the same (delegates to the other Environment constructor)? Maybe an IDE can help do this? Then this method does not even sit in production code?

[jasontedor]

I left a suggestion for exploration.

[jasontedor]

Thank you @droberts195!

[jasontedor]

LGTM.

[synhershko]

Hi there!

Following up on this change, how would a plugin class access the Environment variable?

Adding Environment env to the Plugin class constructor results with the following error message (6.1.0):

   > Throwable #1: java.lang.IllegalStateException: no public constructor of correct signature for [MyAnalysisPlugin]; must be [(org.elasticsearch.common.settings.Settings,java.nio.file.Path)], [(org.elasticsearch.common.settings.Settings)], or [()]
   >    at org.elasticsearch.plugins.PluginsService.loadPlugin(PluginsService.java:449)
   >    at org.elasticsearch.plugins.PluginsService.<init>(PluginsService.java:105)
   >    at org.elasticsearch.node.Node.<init>(Node.java:302)
   >    at org.elasticsearch.node.MockNode.<init>(MockNode.java:73)
   >    at org.elasticsearch.node.MockNode.<init>(MockNode.java:69)
   >    at org.elasticsearch.test.InternalTestCluster.buildNode(InternalTestCluster.java:620)
   >    at org.elasticsearch.test.InternalTestCluster.reset(InternalTestCluster.java:1049)
   >    at org.elasticsearch.test.InternalTestCluster.beforeTest(InternalTestCluster.java:997)
   >    at org.elasticsearch.test.ESIntegTestCase.beforeInternal(ESIntegTestCase.java:380)
   >    at org.elasticsearch.test.ESIntegTestCase.setupTestCluster(ESIntegTestCase.java:2050)
   >    at java.lang.Thread.run(Thread.java:748)Throwable #2: java.lang.IllegalStateException: no public constructor of correct signature for [MyAnalysisPlugin]; must be [(org.elasticsearch.common.settings.Settings,java.nio.file.Path)], [(org.elasticsearch.common.settings.Settings)], or [()]
   >    at org.elasticsearch.plugins.PluginsService.loadPlugin(PluginsService.java:449)
   >    at org.elasticsearch.plugins.PluginsService.<init>(PluginsService.java:105)
   >    at org.elasticsearch.node.Node.<init>(Node.java:302)
   >    at org.elasticsearch.node.MockNode.<init>(MockNode.java:73)
   >    at org.elasticsearch.node.MockNode.<init>(MockNode.java:69)
   >    at org.elasticsearch.test.InternalTestCluster.buildNode(InternalTestCluster.java:620)
   >    at org.elasticsearch.test.InternalTestCluster.buildNode(InternalTestCluster.java:569)
   >    at org.elasticsearch.test.InternalTestCluster.getOrBuildRandomNode(InternalTestCluster.java:475)
   >    at org.elasticsearch.test.InternalTestCluster.client(InternalTestCluster.java:665)
   >    at org.elasticsearch.test.ESIntegTestCase.client(ESIntegTestCase.java:636)
   >    at org.elasticsearch.test.ESIntegTestCase.client(ESIntegTestCase.java:629)
   >    at org.elasticsearch.test.ESIntegTestCase.afterInternal(ESIntegTestCase.java:548)
   >    at org.elasticsearch.test.ESIntegTestCase.cleanUpCluster(ESIntegTestCase.java:2064)
   >    at java.lang.Thread.run(Thread.java:748)

My plugin needs access to env.pluginsFile() to load some files relative to it (in the plugins folder). How should I go about it? Is there a still a way to do DI here?

I should note calling Environment env = new Environment(settings, null); in the plugin constructor seems to work.

[droberts195]

You can use the 2 argument constructor for your plugin, that takes configPath as its second argument, and then construct an Environment passing both settings and configPath. An example is here:

elasticsearch/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsPlugin.java

Line 37 in be74f11

this.config = new ExampleCustomSettingsConfig(new Environment(settings, configPath));

The problem with passing null for the second argument is that things may not work as expected if the config path has been customised.

[synhershko]

That indeed seems to work, thanks!