Skip to content

Commit

Permalink
Issue searchbox-io#146 More changes + sample test
Browse files Browse the repository at this point in the history
  • Loading branch information
Clayton F Stout authored and Clayton F Stout committed Feb 18, 2015
1 parent feeb1da commit 798c5dd
Show file tree
Hide file tree
Showing 10 changed files with 135 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ public <T extends Facet> List<T> getFacets(Class<T> type) {
Constructor<T> c;
try {
JsonObject facetsMap = (JsonObject) jsonObject.get("facets");
if (facetsMap == null) return facets;
if (facetsMap == null)
return facets;
for (Map.Entry<String, JsonElement> facetEntry : facetsMap.entrySet()) {
JsonObject facet = facetEntry.getValue().getAsJsonObject();
if (facet.get("_type").getAsString().equalsIgnoreCase(type.getField("TYPE").get(null).toString())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public List<T> getAggregations(Map<String, Class<T>> nameToTypeMap) {
for (String nameCandidate : nameToTypeMap.keySet()) {
if (jsonRoot.has(nameCandidate)) {
try {
Class<T> type = nameToTypeMap.get(name);
Class<T> type = nameToTypeMap.get(nameCandidate);
Constructor<T> c = type.getConstructor(String.class, JsonObject.class);
aggregations.add(c.newInstance(nameCandidate, jsonRoot.get(nameCandidate).getAsJsonObject()));
} catch (Exception e) {
Expand All @@ -45,7 +45,7 @@ public Aggregation getAggregation(String aggName, Class<T> aggType) {
if(jsonRoot.has(aggName)) {
try {
Constructor<T> c = aggType.getConstructor(String.class, JsonObject.class);
c.newInstance(aggName, jsonRoot.get(aggName).getAsJsonObject());
return c.newInstance(aggName, jsonRoot.get(aggName).getAsJsonObject());
} catch(Exception e) {
throw new RuntimeException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@
/**
* @author cfstout
*/
public class AvgAggregation extends Aggregation {
public class AvgAggregation extends SingleValueAggregation {

public static final String TYPE = "avg";

private Double avg;

public <T extends Aggregation> AvgAggregation(String name, JsonObject avgAggregation) {
super(name, avgAggregation);
avg = avgAggregation.get(String.valueOf(VALUE)).getAsDouble();
}

public Double getAvg() {
return avg;
return getValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@
/**
* @author cfstout
*/
public class MaxAggregation extends Aggregation {
public class MaxAggregation extends SingleValueAggregation {

public static final String TYPE = "max";

private Double max;

public <T extends Aggregation> MaxAggregation(String name, JsonObject maxAggregation) {
super(name, maxAggregation);
max = maxAggregation.get("value").getAsDouble();
}

public Double getMax() {
return max;
return getValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@
/**
* @author cfstout
*/
public class MinAggregation extends Aggregation {
public class MinAggregation extends SingleValueAggregation {

public static final String TYPE = "min";

private Double min;

public <T extends Aggregation> MinAggregation(String name, JsonObject minAggregation) {
super(name, minAggregation);
min = minAggregation.get(String.valueOf(VALUE)).getAsDouble();
}

public Double getMin() {
return min;
return getValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,11 @@
/**
* @author cfstout
*/
public class ScriptedMetricAggregation extends Aggregation {
public class ScriptedMetricAggregation extends SingleValueAggregation {

public static final String TYPE = "scripted_metric";

public Double value;

public <T extends Aggregation> ScriptedMetricAggregation(String name, JsonObject scriptedMetricAggregation) {
super(name, scriptedMetricAggregation);
value = scriptedMetricAggregation.get(String.valueOf(VALUE)).getAsDouble();
}

public Double getValue() {
return value;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package io.searchbox.core.search.aggregation;

import com.google.gson.JsonObject;

import static io.searchbox.core.search.aggregation.AggregationField.VALUE;

/**
* @author cfstout
*/
public abstract class SingleValueAggregation extends Aggregation {

private Double value;

protected SingleValueAggregation(String name, JsonObject singleValueAggregation) {
super(name, singleValueAggregation);
value = singleValueAggregation.get(String.valueOf(VALUE)).getAsDouble();
}

protected Double getValue() {
return value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@
/**
* @author cfstout
*/
public class SumAggregation extends Aggregation {
public class SumAggregation extends SingleValueAggregation {

public static final String TYPE = "sum";

private Double sum;

public <T extends Aggregation> SumAggregation(String name, JsonObject sumAggregation) {
super(name, sumAggregation);
sum = sumAggregation.get(String.valueOf(VALUE)).getAsDouble();
}

public Double getSum() {
return sum;
return getValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,14 @@ public class ValueCountAggregation extends Aggregation {

public static final String TYPE = "value_count";

private ValueCount valueCount;
private Long valueCount;

public <T extends Aggregation> ValueCountAggregation(String name, JsonObject valueCountAggregation) {
super(name, valueCountAggregation);
valueCount = new ValueCount(valueCountAggregation.get(String.valueOf(VALUE)).getAsLong());
valueCount = valueCountAggregation.get(String.valueOf(VALUE)).getAsLong();
}

public ValueCount getValueCount() {
public Long getValueCount() {
return valueCount;
}

public class ValueCount {

private Long value;

ValueCount(Long value) {
this.value = value;
}

public Long getValue() {
return value;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package io.searchbox.core.search.aggregation;

import io.searchbox.common.AbstractIntegrationTest;
import io.searchbox.core.Index;
import io.searchbox.core.Search;
import io.searchbox.core.SearchResult;
import io.searchbox.params.Parameters;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.junit.Before;
import org.junit.Test;

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

/**
* @author cfstout
*/
@ElasticsearchIntegrationTest.ClusterScope (scope = ElasticsearchIntegrationTest.Scope.SUITE, numDataNodes = 1)
public class AvgAggregationIntegrationTest extends AbstractIntegrationTest {

private SearchResult _result;

@Before

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

Create 3 separate test cases instead please. Readability and isolation is more important than code duplication in tests.

This comment has been minimized.

Copy link
@cfstout

cfstout Feb 19, 2015

Owner

There are actually 3 different test cases here. As the intention of the tests it to tell how we are handling the return of the data, not indexing, it seems to me that the initial test setup makes sense to occur only once as this can be done independent of how we handle results. I can duplicate this code, but just seemed that was a lot of extra work (in terms of lines of code and initial test setup) than is actually necessary. Let me know your thoughts.

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

I see your point that the result parsing is the actual section we want to test but since this is an integration test each case should run in their own isolated "integrated session" (where the session consists of given,when,then stages). This isolation is especially important with elasticsearch because of the minor changes they tend to include between versions (just search for testfix in the commit history 😆 ). You can still share the data (objects) but I'd strongly suggest doing any actual work like indexing inside the case even if they are shared.

This comment has been minimized.

Copy link
@cfstout

cfstout Feb 20, 2015

Owner

Yes, I understand that that is normally preferable, but in this case since we only care about parsing these search results in the different test cases, they're basically just unit tests inside the big integration test. I simply split them up for better clarity, but they are each just testing their parsing of the results object.

I have written a few more that tests for bad aggregation names and other off cases. For these where I am constructing a whole new query I will create the indices from scratch. If it still looks bad in my next draft I can change it also.

public void getResult()
throws IOException {
createIndex("avg_aggregation");

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

String index = "avg_aggregation";
createIndex(index);
client().admin().indices().putMapping(new PutMappingRequest("avg_aggregation")

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

PutMappingResponse putMappingResponse = client()...

.type("document")
.source("{\"document\":{\"properties\":{\"delivery\":{\"store\":true,\"type\":\"date\"}}}}")
).actionGet();

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

assertTrue(putMappingResponse.isAcknowledged());
ensureSearchable(index);

String query = "{\n" +
" \"query\" : {\n" +
" \"match_all\" : {}\n" +
" },\n" +
" \"aggs\" : {\n" +
" \"avg1\" : {\n" +
" \"avg\" : {\n" +
" \"field\" : \"num\"\n" +
" }\n" +
" }\n" +
" }\n" +
"}";

Index index = new Index.Builder("{\"num\":2}")
.index("avg_aggregation")
.type("document")
.setParameter(Parameters.REFRESH, true)
.build();
client.execute(index);

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

no use of keeping the action in a variable here.

client.execute(new Index.Builder...)

Actually index ops should not go through Jest here, only the tested op should be via Jest; so please use the index() method of ElasticsearchIntegrationTest.

This comment has been minimized.

Copy link
@cfstout

cfstout Feb 19, 2015

Owner

Ok will do. I was basically just duplicating the flow of the facet tests here, but will update with your suggestions.


index = new Index.Builder("{\"num\":3}")
.index("avg_aggregation")
.type("document")
.setParameter(Parameters.REFRESH, true)
.build();
client.execute(index);

Search search = new Search.Builder(query)
.addIndex("avg_aggregation")
.addType("document")
.build();
_result = client.execute(search);

This comment has been minimized.

Copy link
@kramer

kramer Feb 19, 2015

assertTrue(_result.getErrorMessage(), _result.isSucceeded());

}

@Test
public void testGetAvgAggregationByName() {
AvgAggregation average = _result.getAggregations().getAvgAggregation("avg1");
assertEquals("avg1", average.getName());
assertEquals(new Double(2.5) , average.getAvg());
}

@Test
public void testGetAvgAggregationByType() {
Aggregation aggregation = _result.getAggregations().getAggregation("avg1", AvgAggregation.class);
assertTrue(aggregation instanceof AvgAggregation);
AvgAggregation average = (AvgAggregation) aggregation;
assertEquals("avg1", average.getName());
assertEquals(new Double(2.5), average.getAvg());
}

@Test
public void testGetAvgAggregationWithMap() {
Map<String, Class> nameToTypeMap = new HashMap<String, Class>();
nameToTypeMap.put("avg1", AvgAggregation.class);
List<Aggregation> aggregations = _result.getAggregations().getAggregations(nameToTypeMap);
assertEquals(1, aggregations.size());
assertTrue(aggregations.get(0) instanceof AvgAggregation);
AvgAggregation average = (AvgAggregation) aggregations.get(0);
assertEquals("avg1", average.getName());
assertEquals(new Double(2.5), average.getAvg());
}

}

0 comments on commit 798c5dd

Please sign in to comment.