Skip to content

Forbid the single argument Environment constructor in production code#27235

Merged
droberts195 merged 4 commits intoelastic:masterfrom
droberts195:forbid_single_arg_env_constructor
Nov 4, 2017
Merged

Forbid the single argument Environment constructor in production code#27235
droberts195 merged 4 commits intoelastic:masterfrom
droberts195:forbid_single_arg_env_constructor

Conversation

@droberts195
Copy link
Copy Markdown

@droberts195 droberts195 commented Nov 2, 2017

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
dakrone approved these changes Nov 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming CI is happy :)

@jasontedor
Copy link
Copy Markdown
Member

jasontedor commented Nov 2, 2017 •
edited
Loading

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
jasontedor reviewed Nov 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a suggestion for exploration.

@droberts195
Forbid the single argument Environment constructor in production code
290e4c3 
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.
@droberts195
Revert "Forbid the single argument Environment constructor in product…
3d0163c 
…ion code"

This reverts commit d8fdce8f0c908fc43f2a58c849f32f0cc8120aba.
@droberts195
Replace single arg Environment constructor with test framework factory
4f401a1 
This ensures that only the two argument constructor can ever be used in
production code.
@droberts195 droberts195 force-pushed the forbid_single_arg_env_constructor branch from 25becd9 to 4f401a1 Compare November 3, 2017 11:30
@droberts195 droberts195 removed the :Delivery/Build Build or test infrastructure label Nov 3, 2017
@jasontedor
Copy link
Copy Markdown
Member

jasontedor commented Nov 3, 2017

Thank you @droberts195!

jasontedor
jasontedor approved these changes Nov 3, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@droberts195 droberts195 merged commit 749c3ec into elastic:master Nov 4, 2017
@droberts195 droberts195 deleted the forbid_single_arg_env_constructor branch November 4, 2017 13:25
droberts195 added a commit that referenced this pull request Nov 4, 2017
@droberts195
Remove the single argument Environment constructor (#27235)
c0c3cad 
Only tests should use the single argument Environment constructor.  To
enforce this the single arg Environment constructor has been replaced with
a test framework factory method.

Production code (beyond initial Bootstrap) should always use the same
Environment object that Node.getEnvironment() returns.  This Environment
is also available via dependency injection.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Nov 5, 2017
@jasontedor
Merge branch 'master' into deprecation-header-encoding
fbdb73e 
* master:
  Backport the size-based index rollver to v6.1.0
  Add size-based condition to the index rollover API (elastic#27160)
  Remove the single argument Environment constructor (elastic#27235)
martijnvg added a commit that referenced this pull request Nov 6, 2017
@martijnvg
Merge branch '6.x' into ccr-6.x
5f0bd8c 
* 6.x:
  test: Break apart the multi type aspect of rolling upgrade tests,
  Upgrade to Jackson 2.8.10 (#27230)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535)
  Fix inconsistencies in the rest api specs for `tasks` (#27163)
  Adjust RestHighLevelClient method modifiers (#27238)
  Add more information on `_failed_to_convert_` exception (#27034)
  Remove ElasticsearchQueryCachingPolicy (#27190)
  Backport the size-based index rollver to v6.1.0
  Add size-based condition to the index rollover API (#27160)
  Remove the single argument Environment constructor (#27235)
  Fix RestGetAction name typo
@synhershko
Copy link
Copy Markdown
Contributor

synhershko commented Mar 1, 2018

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
Copy link
Copy Markdown
Author

droberts195 commented Mar 1, 2018

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
Copy link
Copy Markdown
Contributor

synhershko commented Mar 1, 2018

That indeed seems to work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dakrone dakrone dakrone approved these changes

@jasontedor jasontedor jasontedor approved these changes

Assignees

No one assigned

Labels

>non-issue v6.1.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@droberts195 @jasontedor @synhershko @dakrone @colings86