Clean up XPackInfoResponse class and related tests (#35547)

Response classes in Elasticsearch (and xpack) only need to implement ToXContent, which is needed to print their output put in the REST layer and return the response in json (or others) format. On the other hand, response classes that are added to the high-level REST client, need to do the opposite: parse xcontent and create a new object based on that.

This commit removes the parsing code from the XPackInfoResponse server variant, and the toXContent portion from the corresponding client variant. It also removes a client specific test class that looks redundant now that we have a single test class for both classes.

client/rest-high-level/src/main/java/org/elasticsearch/client/xpack/XPackInfoResponse.java

@@ -18,20 +18,15 @@
  */
 package org.elasticsearch.client.xpack;
 
+import org.elasticsearch.client.license.LicenseStatus;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.client.license.LicenseStatus;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collections;
-import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -39,12 +34,11 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
 
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
-public class XPackInfoResponse implements ToXContentObject {
+public class XPackInfoResponse {
     /**
      * Value of the license's expiration time if it should never expire.
      */
@@ -102,7 +96,11 @@ public class XPackInfoResponse implements ToXContentObject {
 
     @Override
     public String toString() {
-        return Strings.toString(this, true, false);
+        return "XPackInfoResponse{" +
+            "buildInfo=" + buildInfo +
+            ", licenseInfo=" + licenseInfo +
+            ", featureSetsInfo=" + featureSetsInfo +
+            '}';
     }
 
     private static final ConstructingObjectParser<XPackInfoResponse, Void> PARSER = new ConstructingObjectParser<>(
@@ -131,41 +129,12 @@ public class XPackInfoResponse implements ToXContentObject {
                 (p, c, name) -> FeatureSetsInfo.FeatureSet.PARSER.parse(p, name),
                 new ParseField("features"));
     }
+
     public static XPackInfoResponse fromXContent(XContentParser parser) throws IOException {
         return PARSER.parse(parser, null);
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-
-        if (buildInfo != null) {
-            builder.field("build", buildInfo, params);
-        }
-
-        EnumSet<XPackInfoRequest.Category> categories = XPackInfoRequest.Category
-                .toSet(Strings.splitStringByCommaToArray(params.param("categories", "_all")));
-        if (licenseInfo != null) {
-            builder.field("license", licenseInfo, params);
-        } else if (categories.contains(XPackInfoRequest.Category.LICENSE)) {
-            // if the user requested the license info, and there is no license, we should send
-            // back an explicit null value (indicating there is no license). This is different
-            // than not adding the license info at all
-            builder.nullField("license");
-        }
-
-        if (featureSetsInfo != null) {
-            builder.field("features", featureSetsInfo, params);
-        }
-
-        if (params.paramAsBoolean("human", true)) {
-            builder.field("tagline", "You know, for X");
-        }
-
-        return builder.endObject();
-    }
-
-    public static class LicenseInfo implements ToXContentObject {
+    public static class LicenseInfo {
         private final String uid;
         private final String type;
         private final String mode;
@@ -217,6 +186,17 @@ public class XPackInfoResponse implements ToXContentObject {
             return Objects.hash(uid, type, mode, status, expiryDate);
         }
 
+        @Override
+        public String toString() {
+            return "LicenseInfo{" +
+                "uid='" + uid + '\'' +
+                ", type='" + type + '\'' +
+                ", mode='" + mode + '\'' +
+                ", status=" + status +
+                ", expiryDate=" + expiryDate +
+                '}';
+        }
+
         private static final ConstructingObjectParser<LicenseInfo, Void> PARSER = new ConstructingObjectParser<>(
                 "license_info", true, (a, v) -> {
                     String uid = (String) a[0];
@@ -234,22 +214,9 @@ public class XPackInfoResponse implements ToXContentObject {
             PARSER.declareString(constructorArg(), new ParseField("status"));
             PARSER.declareLong(optionalConstructorArg(), new ParseField("expiry_date_in_millis"));
         }
-
-        @Override
-        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-            builder.startObject()
-                .field("uid", uid)
-                .field("type", type)
-                .field("mode", mode)
-                .field("status", status.label());
-            if (expiryDate != BASIC_SELF_GENERATED_LICENSE_EXPIRATION_MILLIS) {
-                builder.timeField("expiry_date_in_millis", "expiry_date", expiryDate);
-            }
-            return builder.endObject();
-        }
     }
 
-    public static class BuildInfo implements ToXContentObject {
+    public static class BuildInfo {
         private final String hash;
         private final String timestamp;
 
@@ -280,23 +247,23 @@ public class XPackInfoResponse implements ToXContentObject {
             return Objects.hash(hash, timestamp);
         }
 
+        @Override
+        public String toString() {
+            return "BuildInfo{" +
+                "hash='" + hash + '\'' +
+                ", timestamp='" + timestamp + '\'' +
+                '}';
+        }
+
         private static final ConstructingObjectParser<BuildInfo, Void> PARSER = new ConstructingObjectParser<>(
                 "build_info", true, (a, v) -> new BuildInfo((String) a[0], (String) a[1]));
         static {
             PARSER.declareString(constructorArg(), new ParseField("hash"));
             PARSER.declareString(constructorArg(), new ParseField("date"));
         }
-
-        @Override
-        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-            return builder.startObject()
-                    .field("hash", hash)
-                    .field("date", timestamp)
-                    .endObject();
-        }
     }
 
-    public static class FeatureSetsInfo implements ToXContentObject {
+    public static class FeatureSetsInfo {
         private final Map<String, FeatureSet> featureSets;
 
         public FeatureSetsInfo(Set<FeatureSet> featureSets) {
@@ -325,16 +292,13 @@ public class XPackInfoResponse implements ToXContentObject {
         }
 
         @Override
-        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-            builder.startObject();
-            List<String> names = new ArrayList<>(this.featureSets.keySet()).stream().sorted().collect(Collectors.toList());
-            for (String name : names) {
-                builder.field(name, featureSets.get(name), params);
-            }
-            return builder.endObject();
+        public String toString() {
+            return "FeatureSetsInfo{" +
+                "featureSets=" + featureSets +
+                '}';
         }
 
-        public static class FeatureSet implements ToXContentObject {
+        public static class FeatureSet {
             private final String name;
             @Nullable private final String description;
             private final boolean available;
@@ -389,6 +353,17 @@ public class XPackInfoResponse implements ToXContentObject {
                 return Objects.hash(name, description, available, enabled, nativeCodeInfo);
             }
 
+            @Override
+            public String toString() {
+                return "FeatureSet{" +
+                    "name='" + name + '\'' +
+                    ", description='" + description + '\'' +
+                    ", available=" + available +
+                    ", enabled=" + enabled +
+                    ", nativeCodeInfo=" + nativeCodeInfo +
+                    '}';
+            }
+
             private static final ConstructingObjectParser<FeatureSet, String> PARSER = new ConstructingObjectParser<>(
                     "feature_set", true, (a, name) -> {
                         String description = (String) a[0];
@@ -404,20 +379,6 @@ public class XPackInfoResponse implements ToXContentObject {
                 PARSER.declareBoolean(constructorArg(), new ParseField("enabled"));
                 PARSER.declareObject(optionalConstructorArg(), (p, name) -> p.map(), new ParseField("native_code_info"));
             }
-
-            @Override
-            public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-                builder.startObject();
-                if (description != null) {
-                    builder.field("description", description);
-                }
-                builder.field("available", available);
-                builder.field("enabled", enabled);
-                if (nativeCodeInfo != null) {
-                    builder.field("native_code_info", nativeCodeInfo);
-                }
-                return builder.endObject();
-            }
         }
     }
 }

client/rest-high-level/src/test/java/org/elasticsearch/client/xpack/XPackInfoResponseTests.java

@@ -1,116 +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.client.xpack;
-
-import org.elasticsearch.client.license.LicenseStatus;
-import org.elasticsearch.client.xpack.XPackInfoResponse.BuildInfo;
-import org.elasticsearch.client.xpack.XPackInfoResponse.FeatureSetsInfo;
-import org.elasticsearch.client.xpack.XPackInfoResponse.FeatureSetsInfo.FeatureSet;
-import org.elasticsearch.client.xpack.XPackInfoResponse.LicenseInfo;
-import org.elasticsearch.common.xcontent.ToXContent;
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.test.AbstractXContentTestCase;
-
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
-import java.util.function.Predicate;
-
-public class XPackInfoResponseTests extends AbstractXContentTestCase<XPackInfoResponse> {
-
-    @Override
-    protected boolean supportsUnknownFields() {
-        return true;
-    }
-
-    protected XPackInfoResponse doParseInstance(XContentParser parser) throws IOException {
-        return XPackInfoResponse.fromXContent(parser);
-    }
-
-    protected Predicate<String> getRandomFieldsExcludeFilter() {
-        return path -> path.equals("features")
-                || (path.startsWith("features") && path.endsWith("native_code_info"));
-    }
-
-    protected ToXContent.Params getToXContentParams() {
-        Map<String, String> params = new HashMap<>();
-        if (randomBoolean()) {
-            params.put("human", randomBoolean() ? "true" : "false");
-        }
-        if (randomBoolean()) {
-            params.put("categories", "_none");
-        }
-        return new ToXContent.MapParams(params);
-    }
-
-    protected XPackInfoResponse createTestInstance() {
-        return new XPackInfoResponse(
-            randomBoolean() ? null : randomBuildInfo(),
-            randomBoolean() ? null : randomLicenseInfo(),
-            randomBoolean() ? null : randomFeatureSetsInfo());
-    }
-
-    private BuildInfo randomBuildInfo() {
-        return new BuildInfo(
-            randomAlphaOfLength(10),
-            randomAlphaOfLength(15));
-    }
-
-    private LicenseInfo randomLicenseInfo() {
-        return new LicenseInfo(
-            randomAlphaOfLength(10),
-            randomAlphaOfLength(4),
-            randomAlphaOfLength(5),
-            randomFrom(LicenseStatus.values()),
-            randomLong());
-    }
-
-    private FeatureSetsInfo randomFeatureSetsInfo() {
-        int size = between(0, 10);
-        Set<FeatureSet> featureSets = new HashSet<>(size);
-        while (featureSets.size() < size) {
-            featureSets.add(randomFeatureSet());
-        }
-        return new FeatureSetsInfo(featureSets);
-    }
-
-    private FeatureSet randomFeatureSet() {
-        return new FeatureSet(
-            randomAlphaOfLength(5),
-            randomBoolean() ? null : randomAlphaOfLength(20),
-            randomBoolean(),
-            randomBoolean(),
-            randomNativeCodeInfo());
-    }
-
-    private Map<String, Object> randomNativeCodeInfo() {
-        if (randomBoolean()) {
-            return null;
-        }
-        int size = between(0, 10);
-        Map<String, Object> nativeCodeInfo = new HashMap<>(size);
-        while (nativeCodeInfo.size() < size) {
-            nativeCodeInfo.put(randomAlphaOfLength(5), randomAlphaOfLength(5));
-        }
-        return nativeCodeInfo;
-    }
-}

x-pack/plugin/core/src/main/java/org/elasticsearch/protocol/xpack/XPackInfoResponse.java

@@ -13,10 +13,8 @@ import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
-import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.protocol.xpack.license.LicenseStatus;
 
 import java.io.IOException;
@@ -24,7 +22,6 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -33,7 +30,6 @@ import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
-import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
 public class XPackInfoResponse extends ActionResponse implements ToXContentObject {
     /**
@@ -111,36 +107,6 @@ public class XPackInfoResponse extends ActionResponse implements ToXContentObjec
         return Strings.toString(this, true, false);
     }
 
-    private static final ConstructingObjectParser<XPackInfoResponse, Void> PARSER = new ConstructingObjectParser<>(
-            "xpack_info_response", true, (a, v) -> {
-                BuildInfo buildInfo = (BuildInfo) a[0];
-                LicenseInfo licenseInfo = (LicenseInfo) a[1];
-                @SuppressWarnings("unchecked") // This is how constructing object parser works
-                List<FeatureSetsInfo.FeatureSet> featureSets = (List<FeatureSetsInfo.FeatureSet>) a[2];
-                FeatureSetsInfo featureSetsInfo = featureSets == null ? null : new FeatureSetsInfo(new HashSet<>(featureSets));
-                return new XPackInfoResponse(buildInfo, licenseInfo, featureSetsInfo);
-            });
-    static {
-        PARSER.declareObject(optionalConstructorArg(), BuildInfo.PARSER, new ParseField("build"));
-        /*
-         * licenseInfo is sort of "double optional" because it is
-         * optional but it can also be send as `null`.
-         */
-        PARSER.declareField(optionalConstructorArg(), (p, v) -> {
-                    if (p.currentToken() == XContentParser.Token.VALUE_NULL) {
-                        return null;
-                    }
-                    return LicenseInfo.PARSER.parse(p, v);
-                },
-                new ParseField("license"), ValueType.OBJECT_OR_NULL);
-        PARSER.declareNamedObjects(optionalConstructorArg(),
-                (p, c, name) -> FeatureSetsInfo.FeatureSet.PARSER.parse(p, name),
-                new ParseField("features"));
-    }
-    public static XPackInfoResponse fromXContent(XContentParser parser) throws IOException {
-        return PARSER.parse(parser, null);
-    }
-
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
@@ -236,24 +202,6 @@ public class XPackInfoResponse extends ActionResponse implements ToXContentObjec
             return Objects.hash(uid, type, mode, status, expiryDate);
         }
 
-        private static final ConstructingObjectParser<LicenseInfo, Void> PARSER = new ConstructingObjectParser<>(
-                "license_info", true, (a, v) -> {
-                    String uid = (String) a[0];
-                    String type = (String) a[1];
-                    String mode = (String) a[2];
-                    LicenseStatus status = LicenseStatus.fromString((String) a[3]);
-                    Long expiryDate = (Long) a[4];
-                    long primitiveExpiryDate = expiryDate == null ? BASIC_SELF_GENERATED_LICENSE_EXPIRATION_MILLIS : expiryDate;
-                    return new LicenseInfo(uid, type, mode, status, primitiveExpiryDate);
-                });
-        static {
-            PARSER.declareString(constructorArg(), new ParseField("uid"));
-            PARSER.declareString(constructorArg(), new ParseField("type"));
-            PARSER.declareString(constructorArg(), new ParseField("mode"));
-            PARSER.declareString(constructorArg(), new ParseField("status"));
-            PARSER.declareLong(optionalConstructorArg(), new ParseField("expiry_date_in_millis"));
-        }
-
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             builder.startObject()
@@ -449,22 +397,6 @@ public class XPackInfoResponse extends ActionResponse implements ToXContentObjec
                 return Objects.hash(name, description, available, enabled, nativeCodeInfo);
             }
 
-            private static final ConstructingObjectParser<FeatureSet, String> PARSER = new ConstructingObjectParser<>(
-                    "feature_set", true, (a, name) -> {
-                        String description = (String) a[0];
-                        boolean available = (Boolean) a[1];
-                        boolean enabled = (Boolean) a[2];
-                        @SuppressWarnings("unchecked") // Matches up with declaration below
-                        Map<String, Object> nativeCodeInfo = (Map<String, Object>) a[3];
-                        return new FeatureSet(name, description, available, enabled, nativeCodeInfo);
-                    });
-            static {
-                PARSER.declareString(optionalConstructorArg(), new ParseField("description"));
-                PARSER.declareBoolean(constructorArg(), new ParseField("available"));
-                PARSER.declareBoolean(constructorArg(), new ParseField("enabled"));
-                PARSER.declareObject(optionalConstructorArg(), (p, name) -> p.map(), new ParseField("native_code_info"));
-            }
-
             @Override
             public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
                 builder.startObject();

x-pack/plugin/core/src/test/java/org/elasticsearch/protocol/AbstractHlrcStreamableXContentTestCase.java

@@ -44,7 +44,7 @@ public abstract class AbstractHlrcStreamableXContentTestCase<T extends ToXConten
     public abstract T convertHlrcToInternal(H instance);
 
     //TODO this would be final ideally: why do both responses need to parse from xcontent, only one (H) should? I think that T#fromXContent
-    //are only there for testing and could go away?
+    //are only there for testing and could go away? Then the additional testHlrcFromXContent is also no longer needed.
     @Override
     protected T doParseInstance(XContentParser parser) throws IOException {
         return convertHlrcToInternal(doHlrcParseInstance(parser));

x-pack/plugin/core/src/test/java/org/elasticsearch/protocol/xpack/XPackInfoResponseTests.java

@@ -26,11 +26,6 @@ import java.util.stream.Collectors;
 public class XPackInfoResponseTests extends
         AbstractHlrcStreamableXContentTestCase<XPackInfoResponse, org.elasticsearch.client.xpack.XPackInfoResponse> {
 
-    @Override
-    protected XPackInfoResponse doParseInstance(XContentParser parser) throws IOException {
-        return XPackInfoResponse.fromXContent(parser);
-    }
-
     @Override
     protected XPackInfoResponse createBlankInstance() {
         return new XPackInfoResponse();