Whitelist node stats indices level parameter #21024
Conversation
When indices stats are requested via the node stats API, there is a level parameter to request stats at the index, node, or shards level. This parameter was not whitelisted when URL parsing was made strict. This commit whitelists this parameter. Additionally, there was some leniency in the parsing of this parameter that has been removed.
jasontedor
added
>bug
review
:Core/Infra/REST API
| protected Set<String> responseParams() { | ||
| return RESPONSE_PARAMS; | ||
| } | ||
|
|
There was a problem hiding this comment.
it would be good to have a test that verifies that the param is whitelisted?
There was a problem hiding this comment.
We have general tests in BaseRestHandlerTests for verifying whitelisted parameters works. Do you still think a more specific test is needed here?
There was a problem hiding this comment.
I think all we need is a yaml test that uses the parameter or an example of using the parameter in the docs, right? Then we test that this specific parameter is whitelisted.
There was a problem hiding this comment.
I see. I was wondering if there was a way to catch that we forgot about it. We test that whitelisted settings work I think, so now that it is whitelisted it works for sure, but what if we un-whitelist it by mistake? :) do you see what I mean? That said I am not sure, maybe we could add a small rest test to the existing ones that leverages this param?
There was a problem hiding this comment.
I'll add a small REST test. 😄
| if (!isLevelValid) { | ||
| return builder; | ||
| throw new IllegalArgumentException("level parameter must be one of [indices] or [node] or [shards] but was [" + level + "]"); |
There was a problem hiding this comment.
should we have a test for this check? It is kinda breaking but if it goes in 5.0 it should be ok I guess.
This commit adds a basic REST test for the level parameter on the indices metric for the nodes stats API.
This commit adds a simple test that NodeIndicesStats#toXContent throws an IllegalArgumentException if the level parameter is invalid.
|
Thanks for reviewing @javanna; I've pushed commits in response to your comments. |
This commit fixes a line length violation in NodeIndicesStatsTests.java.
When indices stats are requested via the node stats API, there is a level parameter to request stats at the index, node, or shards level. This parameter was not whitelisted when URL parsing was made strict. This commit whitelists this parameter. Additionally, there was some leniency in the parsing of this parameter that has been removed. Relates #21024
When indices stats are requested via the node stats API, there is a level parameter to request stats at the index, node, or shards level. This parameter was not whitelisted when URL parsing was made strict. This commit whitelists this parameter. Additionally, there was some leniency in the parsing of this parameter that has been removed. Relates #21024
|
Thanks @javanna. |
When indices stats are requested via the node stats API, there is a
level parameter to request stats at the index, node, or shards
level. This parameter was not whitelisted when URL parsing was made
strict. This commit whitelists this parameter.
Additionally, there was some leniency in the parsing of this parameter
that has been removed.
Relates #20722