[Remove] Type from TermsLookUp (#2459)

* [Remove] Type from TermsLookUp

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Fix unit test failure

Signed-off-by: Suraj Singh <surajrider@gmail.com>

rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml

@@ -39,7 +39,7 @@
       search:
         rest_total_hits_as_int: true
         index: "search_index"
-        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
+        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } }
   - do:
       indices.create:
           index: lookup_index
@@ -64,7 +64,7 @@
       search:
         rest_total_hits_as_int: true
         index: "search_index"
-        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
+        body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } }
 
   - match: { _shards.total: 5 }
   - match: { _shards.successful: 5 }

server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java

@@ -1195,75 +1195,63 @@ public class SearchQueryIT extends OpenSearchIntegTestCase {
         );
 
         SearchResponse searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms")))
+            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms")))
             .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "1", "3");
 
         // same as above, just on the _id...
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "1", "terms"))).get();
-            .setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "type", "1", "terms")))
-            .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "1", "3");
 
         // another search with same parameters...
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms"))).get();
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms")))
-            .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "1", "3");
 
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "2", "terms"))).get();
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "2", "terms")))
-            .get();
         assertHitCount(searchResponse, 1L);
         assertFirstHit(searchResponse, hasId("2"));
 
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "3", "terms"))).get();
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "3", "terms")))
-            .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "2", "4");
 
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "4", "terms"))).get();
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "4", "terms")))
-            .get();
         assertHitCount(searchResponse, 0L);
 
         searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "1", "arr.term")))
+            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "1", "arr.term")))
             .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "1", "3");
 
         searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "2", "arr.term")))
+            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "2", "arr.term")))
             .get();
         assertHitCount(searchResponse, 1L);
         assertFirstHit(searchResponse, hasId("2"));
 
         searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "3", "arr.term")))
+            .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "3", "arr.term")))
             .get();
         assertHitCount(searchResponse, 2L);
         assertSearchHits(searchResponse, "2", "4");
 
         searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "type", "3", "arr.term")))
+            .setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "3", "arr.term")))
             .get();
         assertHitCount(searchResponse, 0L);
 
         // index "lookup" type "type" id "missing" document does not exist: ignore the lookup terms
         searchResponse = client().prepareSearch("test")
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "missing", "terms")))
+            .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "missing", "terms")))
             .get();
         assertHitCount(searchResponse, 0L);
 
         // index "lookup3" type "type" has the source disabled: ignore the lookup terms
-        searchResponse = client().prepareSearch("test")
+        searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "1", "terms"))).get();
-            .setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "type", "1", "terms")))
-            .get();
         assertHitCount(searchResponse, 0L);
     }
 

server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java

@@ -491,7 +491,7 @@ public class SimpleValidateQueryIT extends OpenSearchIntegTestCase {
         client().prepareIndex("twitter").setId("1").setSource("followers", new int[] { 1, 2, 3 }).get();
         refresh();
 
-        TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "_doc", "1", "followers"));
+        TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "1", "followers"));
         ValidateQueryResponse response = client().admin()
             .indices()
             .prepareValidateQuery("twitter")

server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java

@@ -225,10 +225,6 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
         return this.termsLookup;
     }
 
-    public boolean isTypeless() {
-        return termsLookup == null || termsLookup.type() == null;
-    }
-
     private static final Set<Class<? extends Number>> INTEGER_TYPES = new HashSet<>(
         Arrays.asList(Byte.class, Short.class, Integer.class, Long.class)
     );

server/src/main/java/org/opensearch/indices/TermsLookup.java

@@ -32,8 +32,7 @@
 
 package org.opensearch.indices;
 
-import org.opensearch.LegacyESVersion;
+import org.opensearch.Version;
-import org.opensearch.common.Nullable;
 import org.opensearch.common.ParseField;
 import org.opensearch.common.io.stream.StreamInput;
 import org.opensearch.common.io.stream.StreamOutput;
@@ -42,13 +41,13 @@ import org.opensearch.common.xcontent.ConstructingObjectParser;
 import org.opensearch.common.xcontent.ToXContentFragment;
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.XContentParser;
+import org.opensearch.index.mapper.MapperService;
 import org.opensearch.index.query.TermsQueryBuilder;
 
 import java.io.IOException;
 import java.util.Objects;
 
 import static org.opensearch.common.xcontent.ConstructingObjectParser.constructorArg;
-import static org.opensearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
 /**
  * Encapsulates the parameters needed to fetch terms.
@@ -56,20 +55,11 @@ import static org.opensearch.common.xcontent.ConstructingObjectParser.optionalCo
 public class TermsLookup implements Writeable, ToXContentFragment {
 
     private final String index;
-    private @Nullable String type;
     private final String id;
     private final String path;
     private String routing;
 
     public TermsLookup(String index, String id, String path) {
-        this(index, null, id, path);
-    }
-
-    /**
-     * @deprecated Types are in the process of being removed, use {@link TermsLookup(String, String, String)} instead.
-     */
-    @Deprecated
-    public TermsLookup(String index, String type, String id, String path) {
         if (id == null) {
             throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the id.");
         }
@@ -80,7 +70,6 @@ public class TermsLookup implements Writeable, ToXContentFragment {
             throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the index.");
         }
         this.index = index;
-        this.type = type;
         this.id = id;
         this.path = path;
     }
@@ -89,11 +78,8 @@ public class TermsLookup implements Writeable, ToXContentFragment {
      * Read from a stream.
      */
     public TermsLookup(StreamInput in) throws IOException {
-        if (in.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) {
+        if (in.getVersion().before(Version.V_2_0_0)) {
-            type = in.readOptionalString();
+            in.readOptionalString();
-        } else {
-            // Before 7.0, the type parameter was always non-null and serialized as a (non-optional) string.
-            type = in.readString();
         }
         id = in.readString();
         path = in.readString();
@@ -103,16 +89,8 @@ public class TermsLookup implements Writeable, ToXContentFragment {
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
-        if (out.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) {
+        if (out.getVersion().before(Version.V_2_0_0)) {
-            out.writeOptionalString(type);
+            out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME);
-        } else {
-            if (type == null) {
-                throw new IllegalArgumentException(
-                    "Typeless [terms] lookup queries are not supported if any " + "node is running a version before 7.0."
-                );
-
-            }
-            out.writeString(type);
         }
         out.writeString(id);
         out.writeString(path);
@@ -124,14 +102,6 @@ public class TermsLookup implements Writeable, ToXContentFragment {
         return index;
     }
 
-    /**
-     * @deprecated Types are in the process of being removed.
-     */
-    @Deprecated
-    public String type() {
-        return type;
-    }
-
     public String id() {
         return id;
     }
@@ -151,14 +121,12 @@ public class TermsLookup implements Writeable, ToXContentFragment {
 
     private static final ConstructingObjectParser<TermsLookup, Void> PARSER = new ConstructingObjectParser<>("terms_lookup", args -> {
         String index = (String) args[0];
-        String type = (String) args[1];
+        String id = (String) args[1];
-        String id = (String) args[2];
+        String path = (String) args[2];
-        String path = (String) args[3];
+        return new TermsLookup(index, id, path);
-        return new TermsLookup(index, type, id, path);
     });
     static {
         PARSER.declareString(constructorArg(), new ParseField("index"));
-        PARSER.declareString(optionalConstructorArg(), new ParseField("type").withAllDeprecated());
         PARSER.declareString(constructorArg(), new ParseField("id"));
         PARSER.declareString(constructorArg(), new ParseField("path"));
         PARSER.declareString(TermsLookup::routing, new ParseField("routing"));
@@ -170,19 +138,12 @@ public class TermsLookup implements Writeable, ToXContentFragment {
 
     @Override
     public String toString() {
-        if (type == null) {
+        return index + "/" + id + "/" + path;
-            return index + "/" + id + "/" + path;
-        } else {
-            return index + "/" + type + "/" + id + "/" + path;
-        }
     }
 
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.field("index", index);
-        if (type != null) {
-            builder.field("type", type);
-        }
         builder.field("id", id);
         builder.field("path", path);
         if (routing != null) {
@@ -193,7 +154,7 @@ public class TermsLookup implements Writeable, ToXContentFragment {
 
     @Override
     public int hashCode() {
-        return Objects.hash(index, type, id, path, routing);
+        return Objects.hash(index, id, path, routing);
     }
 
     @Override
@@ -206,7 +167,6 @@ public class TermsLookup implements Writeable, ToXContentFragment {
         }
         TermsLookup other = (TermsLookup) obj;
         return Objects.equals(index, other.index)
-            && Objects.equals(type, other.type)
             && Objects.equals(id, other.id)
             && Objects.equals(path, other.path)
             && Objects.equals(routing, other.routing);

server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java

@@ -119,9 +119,7 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
 
     private TermsLookup randomTermsLookup() {
         // Randomly choose between a typeless terms lookup and one with an explicit type to make sure we are
-        TermsLookup lookup = maybeIncludeType && randomBoolean()
+        TermsLookup lookup = new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
-            ? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
-            : new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
         // testing both cases.
         lookup.routing(randomBoolean() ? randomAlphaOfLength(10) : null);
         return lookup;
@@ -379,13 +377,6 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
         try {
             QueryBuilder query = super.parseQuery(parser);
             assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));
-
-            TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
-            String deprecationWarning = "Deprecated field [type] used, this field is unused and will be removed entirely";
-            if (termsQuery.isTypeless() == false && !assertedWarnings.contains(deprecationWarning)) {
-                assertWarnings(deprecationWarning);
-                assertedWarnings.add(deprecationWarning);
-            }
             return query;
         } finally {
 

server/src/test/java/org/opensearch/indices/TermsLookupTests.java

@@ -45,42 +45,36 @@ import static org.hamcrest.Matchers.containsString;
 public class TermsLookupTests extends OpenSearchTestCase {
     public void testTermsLookup() {
         String index = randomAlphaOfLengthBetween(1, 10);
-        String type = randomAlphaOfLengthBetween(1, 10);
         String id = randomAlphaOfLengthBetween(1, 10);
         String path = randomAlphaOfLengthBetween(1, 10);
         String routing = randomAlphaOfLengthBetween(1, 10);
-        TermsLookup termsLookup = new TermsLookup(index, type, id, path);
+        TermsLookup termsLookup = new TermsLookup(index, id, path);
         termsLookup.routing(routing);
         assertEquals(index, termsLookup.index());
-        assertEquals(type, termsLookup.type());
         assertEquals(id, termsLookup.id());
         assertEquals(path, termsLookup.path());
         assertEquals(routing, termsLookup.routing());
     }
 
     public void testIllegalArguments() {
-        String type = randomAlphaOfLength(5);
         String id = randomAlphaOfLength(5);
         String path = randomAlphaOfLength(5);
         String index = randomAlphaOfLength(5);
-        switch (randomIntBetween(0, 3)) {
+        switch (randomIntBetween(0, 2)) {
             case 0:
-                type = null;
-                break;
-            case 1:
                 id = null;
                 break;
-            case 2:
+            case 1:
                 path = null;
                 break;
-            case 3:
+            case 2:
                 index = null;
                 break;
             default:
                 fail("unknown case");
         }
         try {
-            new TermsLookup(index, type, id, path);
+            new TermsLookup(index, id, path);
         } catch (IllegalArgumentException e) {
             assertThat(e.getMessage(), containsString("[terms] query lookup element requires specifying"));
         }
@@ -99,35 +93,6 @@ public class TermsLookupTests extends OpenSearchTestCase {
         }
     }
 
-    public void testSerializationWithTypes() throws IOException {
-        TermsLookup termsLookup = randomTermsLookupWithTypes();
-        try (BytesStreamOutput output = new BytesStreamOutput()) {
-            termsLookup.writeTo(output);
-            try (StreamInput in = output.bytes().streamInput()) {
-                TermsLookup deserializedLookup = new TermsLookup(in);
-                assertEquals(deserializedLookup, termsLookup);
-                assertEquals(deserializedLookup.hashCode(), termsLookup.hashCode());
-                assertNotSame(deserializedLookup, termsLookup);
-            }
-        }
-    }
-
-    public void testXContentParsingWithType() throws IOException {
-        XContentParser parser = createParser(
-            JsonXContent.jsonXContent,
-            "{ \"index\" : \"index\", \"id\" : \"id\", \"type\" : \"type\", \"path\" : \"path\", \"routing\" : \"routing\" }"
-        );
-
-        TermsLookup tl = TermsLookup.parseTermsLookup(parser);
-        assertEquals("index", tl.index());
-        assertEquals("type", tl.type());
-        assertEquals("id", tl.id());
-        assertEquals("path", tl.path());
-        assertEquals("routing", tl.routing());
-
-        assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely");
-    }
-
     public void testXContentParsing() throws IOException {
         XContentParser parser = createParser(
             JsonXContent.jsonXContent,
@@ -136,7 +101,6 @@ public class TermsLookupTests extends OpenSearchTestCase {
 
         TermsLookup tl = TermsLookup.parseTermsLookup(parser);
         assertEquals("index", tl.index());
-        assertNull(tl.type());
         assertEquals("id", tl.id());
         assertEquals("path", tl.path());
         assertEquals("routing", tl.routing());
@@ -147,13 +111,4 @@ public class TermsLookupTests extends OpenSearchTestCase {
             randomBoolean() ? randomAlphaOfLength(10) : null
         );
     }
-
-    public static TermsLookup randomTermsLookupWithTypes() {
-        return new TermsLookup(
-            randomAlphaOfLength(10),
-            randomAlphaOfLength(10),
-            randomAlphaOfLength(10),
-            randomAlphaOfLength(10).replace('.', '_')
-        ).routing(randomBoolean() ? randomAlphaOfLength(10) : null);
-    }
 }