Add enrich policy PUT API #41383
Add enrich policy PUT API #41383
Conversation
This commit wires up the Rest calls and Transport calls for PUT enrich policy, as well as tests and rest spec additions.
hub-cap
added
>enhancement
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features |
| } | ||
|
|
||
| public static class Request extends MasterNodeRequest<PutEnrichPolicyAction.Request> { | ||
| private final EnrichPolicy policy; |
There was a problem hiding this comment.
add newline before the first field
| private final EnrichPolicy policy; | ||
| private final String name; | ||
|
|
||
|
|
|
|
||
| @Override | ||
| public ActionRequestValidationException validate() { | ||
| return null; |
There was a problem hiding this comment.
add validation here that the name is required?
There was a problem hiding this comment.
im not sure we need this since the constructor for the request has an Objects.requireNotNull. I could add something to the streaminput constructor as well, but that does not look like its typically done, as we should be able to trust that more than a "user input" constructor, so to speak.
There was a problem hiding this comment.
Good point. Maybe instead of using Objects.requireNotNull, use an if statement that checks now null and if so then throw a IllegalArgumentException? Objects.requireNotNull throws a NPE and that will result in a 500 response code, whilst a IllegalArgumentException translates to a 400 response code and that is a better response code in this case.
There was a problem hiding this comment.
Never mind. This good as it is now.
In production, the constructor can't be invoked with a null name, because if the rest handler is invoked then the name had to provided.
| } | ||
|
|
||
| static void putPolicy(String name, EnrichPolicy policy, EnrichStore store, ActionListener<AcknowledgedResponse> listener) { | ||
| if (Strings.isNullOrEmpty(name)) { |
There was a problem hiding this comment.
maybe add this if check to PolicyStore#putStore(...)?
and then replace testGetEnricyPolicyAction_emptyChecks test with a test in EnrichStoreTests
There was a problem hiding this comment.
and I think that this method can be merged with masterOperation(...)
There was a problem hiding this comment.
Also an IllegalArgumentException is properly better here. Specifying an empty name or no name is just not allowed when creating a policy. On the rest layer this is then translated to a 400 response code instead of a 404 response code.
There was a problem hiding this comment.
Given this, I think Im just going to remove the transport test. the REST and other tests exercise the transport layer fully, as it is a wrapper w/o validation, and since there is no license checks, its literally now a 1 line passthru to the EnrichStore.
| @Override | ||
| protected void masterOperation(PutEnrichPolicyAction.Request request, ClusterState state, | ||
| ActionListener<AcknowledgedResponse> listener) throws Exception { | ||
| // TODO license validation |
There was a problem hiding this comment.
This TODO can be removed. Enrich is going to be basic and for basic we don't need to add license validation.
This commit also removes a unit test. This functionlity is tested elsewhere, and the mocking that has to be done to test a thin wrapper is not worth it, since basic PUT calls will fail if the logic is wrong.
| enrich_values: ["a", "b"] | ||
| schedule: "*/120" | ||
|
|
||
| - is_true: acknowledged |
There was a problem hiding this comment.
iirc all yaml test files are required to end with a newline?
|
|
||
| @Override | ||
| public ActionRequestValidationException validate() { | ||
| return null; |
There was a problem hiding this comment.
Good point. Maybe instead of using Objects.requireNotNull, use an if statement that checks now null and if so then throw a IllegalArgumentException? Objects.requireNotNull throws a NPE and that will result in a 500 response code, whilst a IllegalArgumentException translates to a 400 response code and that is a better response code in this case.
| enrich.put_policy: | ||
| name: policy-crud | ||
| body: | ||
| policy: |
There was a problem hiding this comment.
The policy field isn't needed and this is why this test is failing in pr ci.
There was a problem hiding this comment.
oh geez, i removed that {"policy": wrapper I had initially put around the PUT... thank you for the catch.
|
@elasticmachine run elasticsearch-ci/1 |
| @@ -20,7 +20,7 @@ | |||
| /** | |||
| * Helper methods for access and storage of an enrich policy. | |||
| */ | |||
| public final class EnrichStore { | |||
| public class EnrichStore { | |||
There was a problem hiding this comment.
I think we can keep this class final now?
There was a problem hiding this comment.
oh, yes, now that it holds no state, good catch!
This commit wires up the Rest calls and Transport calls for PUT enrich policy, as well as tests and rest spec additions.
This commit wires up the Rest calls and Transport calls for PUT enrich
policy, as well as tests and rest spec additions.