Fix NPE when using deprecated Azure settings#28769
Merged
dadoonet merged 2 commits intoelastic:6.xfrom Feb 26, 2018
Merged
Conversation
When someone migrates from 5.6 to 6.x with deprecated settings like:
```
cloud:
azure:
storage:
foo:
account: <my_account>
key: <my_key>
```
It produces a NPE anytime someone wants to use a repository which name is not `default`.
This has been introduced by elastic#23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal" `storageSettings` with:
```java
this.storageSettings.get(clientName)
```
But in the context of deprecated settings, this returns `null` which causes the NPE just after.
This commit adds a check and if no settings are found in the "normal" `storageSettings`, we look at settings from `deprecatedStorageSettings`.
The reason I missed it in the 7.0 version (master branch) is because actually `deprecatedStorageSettings` has been removed there already.
Also I modified the `testGetSelectedClientDefault` method which was only doing exactly the same thing as `testGetDefaultClientWithPrimaryAndSecondaries`.
Closes elastic#28299.
Contributor
Author
|
@rjernst I set the target for now to 6.x (6.3.0) but I believe that it should also go in 6.2 and may be 6.1. WDYT? |
rjernst
approved these changes
Feb 22, 2018
| @@ -392,8 +392,9 @@ public void testGetDefaultClientWithPrimaryAndSecondaries() { | |||
| @Deprecated | |||
| public void testGetSelectedClientDefault() { | |||
Member
There was a problem hiding this comment.
Shouldn't this test still be calling getSelectedClient (which in turn calls generateOperationContext)? Otherwise the name should be updated.
Contributor
Author
There was a problem hiding this comment.
No. Because getSelectedClient never calls generateOperationContext where the NPE happened.
I'm going to rename the method indeed to make that more obvious.
Contributor
Author
|
@rjernst Thanks for the fast review! What is your opinion about backporting? I think it's a bad bug for people upgrading from 5.x to 6.x so I believe we should backport to 6.2. |
As we only test the `generateOperationContext` call, that makes a lot of sense to rename it.
Contributor
|
Jenkins, test this please. (my apologies, I had to restart elasticsearch-ci to perform some upgrades, so this job never finished building) |
Member
|
@dadoonet I would backport to 6.x and 6.2. |
dadoonet
added a commit
that referenced
this pull request
Feb 27, 2018
* Fix NPE when using deprecated Azure settings
When someone migrates from 5.6 to 6.x with deprecated settings like:
```
cloud:
azure:
storage:
foo:
account: <my_account>
key: <my_key>
```
It produces a NPE anytime someone wants to use a repository which name is not `default`.
This has been introduced by #23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal" `storageSettings` with:
```java
this.storageSettings.get(clientName)
```
But in the context of deprecated settings, this returns `null` which causes the NPE just after.
This commit adds a check and if no settings are found in the "normal" `storageSettings`, we look at settings from `deprecatedStorageSettings`.
The reason I missed it in the 7.0 version (master branch) is because actually `deprecatedStorageSettings` has been removed there already.
Also I renamed the `testGetSelectedClientDefault` method to `testGenerateOperationContext`
as it was only doing exactly the same thing as `testGetDefaultClientWithPrimaryAndSecondaries`.
Closes #28299.
Backport of #28769 in 6.2 branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When someone migrates from 5.6 to 6.x with deprecated settings like:
It produces a NPE anytime someone wants to use a repository which name is not
default.This has been introduced by #23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal"
storageSettingswith:But in the context of deprecated settings, this returns
nullwhich causes the NPE just after.This commit adds a check and if no settings are found in the "normal"
storageSettings, we look at settings fromdeprecatedStorageSettings.The reason I missed it in the 7.0 version (master branch) is because actually
deprecatedStorageSettingshas been removed there already.Also I modified the
testGetSelectedClientDefaultmethod which was only doing exactly the same thing astestGetDefaultClientWithPrimaryAndSecondaries.Closes #28299.