From 02e698bc43d164add42ef9fb6012cb9d351bf5c6 Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Wed, 10 Feb 2016 11:26:30 +0100 Subject: [PATCH] Third round of comments --- .../elasticsearch/common/unit/Fuzziness.java | 1 + .../indices/query/IndicesQueriesRegistry.java | 9 - .../search/sort/FieldSortBuilder.java | 62 +++---- .../search/sort/GeoDistanceSortBuilder.java | 5 +- .../search/sort/SortBuilderTemp.java | 8 +- .../elasticsearch/search/sort/SortOrder.java | 2 +- .../AbstractSearchSourceItemTestCase.java | 163 ------------------ .../search/sort/AbstractSortTestCase.java | 6 +- .../search/sort/FieldSortBuilderTests.java | 7 +- 9 files changed, 31 insertions(+), 232 deletions(-) delete mode 100644 core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java diff --git a/core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java b/core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java index 831fbe505bb..24a727691cc 100644 --- a/core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java +++ b/core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java @@ -67,6 +67,7 @@ public final class Fuzziness implements ToXContent, Writeable { /** * Creates a {@link Fuzziness} instance from an edit distance. The value must be one of [0, 1, 2] + * * Note: Using this method only makes sense if the field you are applying Fuzziness to is some sort of string. */ public static Fuzziness fromEdits(int edits) { diff --git a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java index c72810212eb..a9e90884a68 100644 --- a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java +++ b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java @@ -24,11 +24,9 @@ import java.util.Map; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.query.QueryParser; -import org.elasticsearch.search.sort.SortBuilderTemp; public class IndicesQueriesRegistry extends AbstractComponent { private Map> queryParsers; - private Map> sortParsers; public IndicesQueriesRegistry(Settings settings, Map> queryParsers) { super(settings); @@ -41,11 +39,4 @@ public class IndicesQueriesRegistry extends AbstractComponent { public Map> queryParsers() { return queryParsers; } - - /** - * Returns all registered sort parsers - */ - public Map> sortParsers() { - return sortParsers; - } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 5440e996b50..a11eefc1f66 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -286,10 +286,9 @@ public class FieldSortBuilder extends SortBuilder implements SortBuilderTemp getBuilderPrototype() { - return PROTOTYPE; - } - } diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index b5a10e238b7..8cc138bf62c 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -23,11 +23,9 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; -import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.DistanceUnit; -import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryBuilder; @@ -44,8 +42,7 @@ import java.util.Objects; /** * A geo distance based sorting on a geo point like field. */ -public class GeoDistanceSortBuilder extends SortBuilder - implements ToXContent, NamedWriteable, SortElementParserTemp { +public class GeoDistanceSortBuilder extends SortBuilder implements SortBuilderTemp { public static final String NAME = "_geo_distance"; public static final boolean DEFAULT_COERCE = false; public static final boolean DEFAULT_IGNORE_MALFORMED = false; diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java index 15a12ec90f2..e3e571c4410 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilderTemp.java @@ -25,7 +25,7 @@ import org.elasticsearch.index.query.QueryParseContext; import java.io.IOException; -public interface SortBuilderTemp extends NamedWriteable, ToXContent { +public interface SortBuilderTemp extends NamedWriteable, ToXContent { /** * Creates a new item from the json held by the {@link SortBuilderTemp} * in {@link org.elasticsearch.common.xcontent.XContent} format @@ -36,9 +36,5 @@ public interface SortBuilderTemp extends NamedWriteable fromXContent(QueryParseContext context, String elementName) throws IOException; - - String getName(); - - SortBuilderTemp getBuilderPrototype(); + SortBuilderTemp fromXContent(QueryParseContext context, String elementName) throws IOException; } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java b/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java index 9c351880293..73e5ac55247 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortOrder.java @@ -51,7 +51,7 @@ public enum SortOrder implements Writeable { } }; - private static final SortOrder PROTOTYPE = DESC; + private static final SortOrder PROTOTYPE = ASC; @Override public SortOrder readFrom(StreamInput in) throws IOException { diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java deleted file mode 100644 index 3e86a227986..00000000000 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSearchSourceItemTestCase.java +++ /dev/null @@ -1,163 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.search.sort; - -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.NamedWriteable; -import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; -import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentHelper; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.index.query.QueryParseContext; -import org.elasticsearch.indices.query.IndicesQueriesRegistry; -import org.elasticsearch.search.SearchModule; -import org.elasticsearch.test.ESTestCase; -import org.junit.AfterClass; -import org.junit.BeforeClass; - -import java.io.IOException; - -import static org.hamcrest.Matchers.*; - -public abstract class AbstractSearchSourceItemTestCase> extends ESTestCase { - - protected static NamedWriteableRegistry namedWriteableRegistry; - - private static final int NUMBER_OF_TESTBUILDERS = 20; - static IndicesQueriesRegistry indicesQueriesRegistry; - - @BeforeClass - public static void init() { - namedWriteableRegistry = new NamedWriteableRegistry(); - namedWriteableRegistry.registerPrototype(FieldSortBuilder.class, FieldSortBuilder.PROTOTYPE); - indicesQueriesRegistry = new SearchModule(Settings.EMPTY, namedWriteableRegistry).buildQueryParserRegistry(); - } - - @AfterClass - public static void afterClass() throws Exception { - namedWriteableRegistry = null; - } - - /** Returns random sort that is put under test */ - protected abstract T createTestItem(); - - /** Returns mutated version of original so the returned sort is different in terms of equals/hashcode */ - protected abstract T mutate(T original) throws IOException; - - /** - * Test that creates new sort from a random test sort and checks both for equality - */ - public void testFromXContent() throws IOException { - for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { - T testItem = createTestItem(); - - XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values())); - if (randomBoolean()) { - builder.prettyPrint(); - } - builder.startObject(); - testItem.toXContent(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - - XContentParser itemParser = XContentHelper.createParser(builder.bytes()); - itemParser.nextToken(); - - /* - * filter out name of sort, or field name to sort on for element fieldSort - */ - itemParser.nextToken(); - String elementName = itemParser.currentName(); - itemParser.nextToken(); - - QueryParseContext context = new QueryParseContext(indicesQueriesRegistry); - context.reset(itemParser); - NamedWriteable parsedItem = testItem.fromXContent(context, elementName); - assertNotSame(testItem, parsedItem); - assertEquals(testItem, parsedItem); - assertEquals(testItem.hashCode(), parsedItem.hashCode()); - } - } - - /** - * Test serialization and deserialization of the test sort. - */ - public void testSerialization() throws IOException { - for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { - T testsort = createTestItem(); - T deserializedsort = copyItem(testsort); - assertEquals(testsort, deserializedsort); - assertEquals(testsort.hashCode(), deserializedsort.hashCode()); - assertNotSame(testsort, deserializedsort); - } - } - - /** - * Test equality and hashCode properties - */ - public void testEqualsAndHashcode() throws IOException { - for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { - T firstsort = createTestItem(); - assertFalse("sort is equal to null", firstsort.equals(null)); - assertFalse("sort is equal to incompatible type", firstsort.equals("")); - assertTrue("sort is not equal to self", firstsort.equals(firstsort)); - assertThat("same sort's hashcode returns different values if called multiple times", firstsort.hashCode(), - equalTo(firstsort.hashCode())); - assertThat("different sorts should not be equal", mutate(firstsort), not(equalTo(firstsort))); - assertThat("different sorts should have different hashcode", mutate(firstsort).hashCode(), not(equalTo(firstsort.hashCode()))); - - T secondsort = copyItem(firstsort); - assertTrue("sort is not equal to self", secondsort.equals(secondsort)); - assertTrue("sort is not equal to its copy", firstsort.equals(secondsort)); - assertTrue("equals is not symmetric", secondsort.equals(firstsort)); - assertThat("sort copy's hashcode is different from original hashcode", secondsort.hashCode(), equalTo(firstsort.hashCode())); - - T thirdsort = copyItem(secondsort); - assertTrue("sort is not equal to self", thirdsort.equals(thirdsort)); - assertTrue("sort is not equal to its copy", secondsort.equals(thirdsort)); - assertThat("sort copy's hashcode is different from original hashcode", secondsort.hashCode(), equalTo(thirdsort.hashCode())); - assertTrue("equals is not transitive", firstsort.equals(thirdsort)); - assertThat("sort copy's hashcode is different from original hashcode", firstsort.hashCode(), equalTo(thirdsort.hashCode())); - assertTrue("equals is not symmetric", thirdsort.equals(secondsort)); - assertTrue("equals is not symmetric", thirdsort.equals(firstsort)); - } - } - - protected T copyItem(SortBuilderTemp sort) throws IOException { - try (BytesStreamOutput output = new BytesStreamOutput()) { - sort.writeTo(output); - try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { - SortBuilderTemp prototype = sortParser(sort.getName()).getBuilderPrototype(); - @SuppressWarnings("unchecked") - T secondQuery = (T) prototype.readFrom(in); - return secondQuery; - } - } - } - - private SortBuilderTemp sortParser(String queryId) { - return indicesQueriesRegistry.sortParsers().get(queryId); - } -} diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java index dc61f0ef34c..8cadfb24408 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -20,7 +20,6 @@ package org.elasticsearch.search.sort; import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.StreamInput; @@ -43,7 +42,7 @@ import java.io.IOException; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; -public abstract class AbstractSortTestCase & SortElementParserTemp> extends ESTestCase { +public abstract class AbstractSortTestCase> extends ESTestCase { protected static NamedWriteableRegistry namedWriteableRegistry; @@ -154,7 +153,8 @@ public abstract class AbstractSortTestCase { - - @SuppressWarnings("unchecked") - public Class getPrototype() { - return (Class) FieldSortBuilder.PROTOTYPE.getClass(); - } +public class FieldSortBuilderTests extends AbstractSortTestCase { @Override protected FieldSortBuilder createTestItem() {