Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST high-level client: add get ingest pipeline API #30847

Merged
merged 7 commits into from
Jun 1, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void putPipelineAsync(PutPipelineRequest request, ActionListener<PutPipel
* Get an existing pipeline
* <p>
* See
* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/put-pipeline-api.html"> Get Pipeline API on elastic.co</a>
* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/get-pipeline-api.html"> Get Pipeline API on elastic.co</a>
*/
public GetPipelineResponse getPipeline(GetPipelineRequest request, Header... headers) throws IOException {
return restHighLevelClient.performRequestAndParseEntity( request, RequestConverters::getPipeline,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public void testPutPipeline() throws IOException {
}

public void testGetPipeline() throws IOException {
String id = "get_pipeline_id";
String id = "some_pipeline_id";
XContentBuilder pipelineBuilder = buildRandomXContentPipeline();
{
PutPipelineRequest request = new PutPipelineRequest(
Expand All @@ -143,9 +143,9 @@ public void testGetPipeline() throws IOException {
GetPipelineResponse response =
execute(request, highLevelClient().cluster()::getPipeline, highLevelClient().cluster()::getPipelineAsync);
assertTrue(response.isFound());
assertEquals(response.getPipelineConfigs().get(0).getId(), id);
assertEquals(response.pipelines().get(0).getId(), id);
PipelineConfiguration expectedConfig =
new PipelineConfiguration(id, BytesReference.bytes(pipelineBuilder), pipelineBuilder.contentType());
assertEquals(expectedConfig.getConfigAsMap(), response.getPipelineConfigs().get(0).getConfigAsMap());
assertEquals(expectedConfig.getConfigAsMap(), response.pipelines().get(0).getConfigAsMap());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

Expand Down Expand Up @@ -284,7 +285,7 @@ public void testGetPipeline() throws IOException {

// tag::get-pipeline-response
boolean successful = response.isFound(); // <1>
List<PipelineConfiguration> pipelines = response.getPipelineConfigs(); // <2>
List<PipelineConfiguration> pipelines = response.pipelines(); // <2>
for(PipelineConfiguration pipeline: pipelines) {
Map<String, Object> config = pipeline.getConfigAsMap(); // <3>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.HashMap;

import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

Expand All @@ -48,8 +50,13 @@ public GetPipelineResponse(List<PipelineConfiguration> pipelines) {
this.pipelines = pipelines;
Copy link
Contributor Author

@sohaibiftikhar sohaibiftikhar May 29, 2018

Choose a reason for hiding this comment

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

@javanna Should I change this constructor to Set<PipelineConfiguration> instead of List<PipelineConfiguration> to keep it more in line with the actual JSON response? Then we don't need to do tricky stuff in the equals and hashcode. I did not do this before because PipelineConfiguration did not have the ideal equals and hashcode.

Copy link
Member

Choose a reason for hiding this comment

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

I would be ok with that, but let's first see whether tests are fine with this change, my assumptions may be wrong :)

}

/**
* Get the list of pipelines that were a part of this response.
* The pipeline id can be obtained using getId on the PipelineConfiguration object.
* @return A list of {@link PipelineConfiguration} objects.
*/
public List<PipelineConfiguration> pipelines() {
return pipelines;
return Collections.unmodifiableList(pipelines);
}

@Override
Expand Down Expand Up @@ -80,15 +87,6 @@ public RestStatus status() {
return isFound() ? RestStatus.OK : RestStatus.NOT_FOUND;
}

/**
* Get the list of pipelines that were a part of this response
* The pipeline id can be obtained using
* @return A list of PipelineConfiguration objects.
*/
public List<PipelineConfiguration> getPipelineConfigs() {
return Collections.unmodifiableList(pipelines);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
Expand All @@ -103,7 +101,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
*
* @param parser the parser for the XContent that contains the serialized GetPipelineResponse.
* @return an instance of GetPipelineResponse read from the parser
* @throws IOException
* @throws IOException If the parsing fails
*/
public static GetPipelineResponse fromXContent(XContentParser parser) throws IOException {
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
Expand All @@ -122,4 +120,43 @@ public static GetPipelineResponse fromXContent(XContentParser parser) throws IOE
ensureExpectedToken(XContentParser.Token.END_OBJECT, parser.currentToken(), parser::getTokenLocation);
return new GetPipelineResponse(pipelines);
}

@Override
public boolean equals(Object other) {
if (other == null) {
return false;
} else if (other instanceof GetPipelineResponse){
GetPipelineResponse otherResponse = (GetPipelineResponse)other;
if (pipelines == null) {
return otherResponse.pipelines == null;
} else {
// We need a map here because order does not matter for equality
Map<String, PipelineConfiguration> otherPipelineMap = new HashMap<>();
for (PipelineConfiguration pipeline: otherResponse.pipelines) {
otherPipelineMap.put(pipeline.getId(), pipeline);
}
for (PipelineConfiguration pipeline: pipelines) {
PipelineConfiguration otherPipeline = otherPipelineMap.get(pipeline.getId());
if (otherPipeline == null ||
!pipeline.getConfigAsMap().equals(otherPipeline.getConfigAsMap())) {
return false;
}
}
return true;
}
} else {
return false;
}
}

@Override
public int hashCode() {
int result = 1;
for (PipelineConfiguration pipeline: pipelines) {
// We only take the sum here to ensure that the order does not matter.
result += (pipeline == null ? 0 : pipeline.getConfigAsMap().hashCode());
Copy link
Member

Choose a reason for hiding this comment

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

shall we add equals and hashcode to PipelineConfiguration ?

Copy link
Contributor Author

@sohaibiftikhar sohaibiftikhar May 29, 2018

Choose a reason for hiding this comment

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

It is already there. Unfortunately, it uses the BytesReference to do this. This depends on the XContentType and hence is not useful for comparison. Do you want me to change the implementation there?

Copy link
Member

Choose a reason for hiding this comment

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

I think we could, I am assuming that we need these only for tests and making equals/hashcode work with the map representation would not hurt. You can try and run IngestMetadaTests, PipelineConfigurationTests, PipelineExecutionServiceTests and PipelineStoreTests to check whether they remain green after the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool thanks! Will do the change.

}
return result;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,42 +26,45 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.ingest.PipelineConfiguration;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.AbstractStreamableXContentTestCase;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class GetPipelineResponseTests extends ESTestCase {
public class GetPipelineResponseTests extends AbstractStreamableXContentTestCase<GetPipelineResponse> {

private XContentBuilder getRandomXContentBuilder() throws IOException {
XContentType xContentType = randomFrom(XContentType.values());
return XContentBuilder.builder(xContentType.xContent());
}

private PipelineConfiguration createRandomPipeline(String pipelineId) throws IOException {
String field = "field_" + randomInt();
String value = "value_" + randomInt();
XContentBuilder builder = getRandomXContentBuilder();
builder.startObject();
// We only use a single SetProcessor here in each pipeline to test.
// Since the contents are returned as a configMap anyway this does not matter for fromXContent
builder.startObject("set");
builder.field("field", field);
builder.field("value", value);
builder.endObject();
builder.endObject();
return
new PipelineConfiguration(
pipelineId, BytesReference.bytes(builder), builder.contentType()
);
}

private Map<String, PipelineConfiguration> createPipelineConfigMap() throws IOException {
int numPipelines = randomInt(5);
Map<String, PipelineConfiguration> pipelinesMap = new HashMap<>();
for (int i=0; i<numPipelines; i++) {
// We only use a single SetProcessor here in each pipeline to test.
// Since the contents are returned as a configMap anyway this does not matter for fromXContent
String pipelineId = "pipeline_" + i;
String field = "field_" + i;
String value = "value_" + i;
XContentBuilder builder = getRandomXContentBuilder();
builder.startObject();
builder.startObject("set");
builder.field("field", field);
builder.field("value", value);
builder.endObject();
builder.endObject();
PipelineConfiguration pipeline =
new PipelineConfiguration(
pipelineId, BytesReference.bytes(builder), builder.contentType()
);
pipelinesMap.put(pipelineId, pipeline);
pipelinesMap.put(pipelineId, createRandomPipeline(pipelineId));
}
return pipelinesMap;
}
Expand Down Expand Up @@ -89,4 +92,41 @@ public void testXContentDeserialization() throws IOException {
assertEquals(pipelinesMap.get(pipeline.getId()).getConfigAsMap(), pipeline.getConfigAsMap());
}
}

@Override
protected GetPipelineResponse doParseInstance(XContentParser parser) throws IOException {
return GetPipelineResponse.fromXContent(parser);
}

@Override
protected GetPipelineResponse createBlankInstance() {
return new GetPipelineResponse();
}

@Override
protected GetPipelineResponse createTestInstance() {
try {
return new GetPipelineResponse(new ArrayList<>(createPipelineConfigMap().values()));
} catch (IOException e) {
// If we fail to construct an instance we return `null` which would make the user of this method
// fail the test.
return null;
Copy link
Member

Choose a reason for hiding this comment

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

I would rather throw UncheckedIOException here

}
}

@Override
protected boolean supportsUnknownFields() {
return false;
}

@Override
protected GetPipelineResponse mutateInstance(GetPipelineResponse response) {
try {
ArrayList<PipelineConfiguration> clonePipelines = new ArrayList<>(response.pipelines());
Copy link
Member

@javanna javanna May 29, 2018

Choose a reason for hiding this comment

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

nit: can you declare it as a list on the left side?

Copy link
Contributor Author

@sohaibiftikhar sohaibiftikhar May 29, 2018

Choose a reason for hiding this comment

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

You mean left side?

Copy link
Member

Choose a reason for hiding this comment

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

yes :) I have edited my comment right after posting it :)

clonePipelines.add(createRandomPipeline("pipeline_" + clonePipelines.size() + 1));
return new GetPipelineResponse(clonePipelines);
} catch (IOException e) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

rather throw UncheckedIOException ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. I didn't know these existed.

}
}
}