From 505c4be5de51820e3ff85d26b88162c7b45b97f4 Mon Sep 17 00:00:00 2001
From: Jedr Blaszyk <jedrazb@gmail.com>
Date: Thu, 7 Dec 2023 14:33:34 +0100
Subject: [PATCH] [Connector API] QA adapt handling null values in
 parser/toXContent (#103107)

(cherry picked from commit 6313275a8bcbd23344f057d09e322711092e0408)
---
 .../application/connector/Connector.java      | 136 ++++++------------
 .../connector/action/PostConnectorAction.java |  12 +-
 .../connector/action/PutConnectorAction.java  |   9 +-
 .../connector/ConnectorTestUtils.java         |   6 +-
 .../application/connector/ConnectorTests.java |  58 ++++++++
 5 files changed, 109 insertions(+), 112 deletions(-)

diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java
index bcb182774e758..11c5a44d45977 100644
--- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java
+++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java
@@ -39,7 +39,7 @@
  * and synchronizing external data sources with Elasticsearch. Each Connector instance encapsulates
  * various settings and state information, including:
  * <ul>
- *     <li>A unique identifier for distinguishing different connectors.</li>
+ *     <li>A doc _id of the connector document.</li>
  *     <li>API key for authenticating with Elasticsearch, ensuring secure access.</li>
  *     <li>A configuration mapping which holds specific settings and parameters for the connector's operation.</li>
  *     <li>A {@link ConnectorCustomSchedule} object that defines custom scheduling.</li>
@@ -65,12 +65,11 @@ public class Connector implements NamedWriteable, ToXContentObject {
 
     public static final String NAME = Connector.class.getName().toUpperCase(Locale.ROOT);
 
+    @Nullable
     private final String connectorId;
     @Nullable
     private final String apiKeyId;
-    @Nullable
     private final Map<String, ConnectorConfiguration> configuration;
-    @Nullable
     private final Map<String, ConnectorCustomSchedule> customScheduling;
     @Nullable
     private final String description;
@@ -78,11 +77,9 @@ public class Connector implements NamedWriteable, ToXContentObject {
     private final String error;
     @Nullable
     private final ConnectorFeatures features;
-    @Nullable
     private final List<ConnectorFiltering> filtering;
     @Nullable
     private final String indexName;
-
     private final boolean isNative;
     @Nullable
     private final String language;
@@ -94,7 +91,6 @@ public class Connector implements NamedWriteable, ToXContentObject {
     private final String name;
     @Nullable
     private final ConnectorIngestPipeline pipeline;
-    @Nullable
     private final ConnectorScheduling scheduling;
     @Nullable
     private final String serviceType;
@@ -151,22 +147,22 @@ private Connector(
     ) {
         this.connectorId = connectorId;
         this.apiKeyId = apiKeyId;
-        this.configuration = configuration;
-        this.customScheduling = customScheduling;
+        this.configuration = Objects.requireNonNull(configuration, "[configuration] cannot be null");
+        this.customScheduling = Objects.requireNonNull(customScheduling, "[custom_scheduling] cannot be null");
         this.description = description;
         this.error = error;
         this.features = features;
-        this.filtering = filtering;
-        this.indexName = indexName;
+        this.filtering = Objects.requireNonNull(filtering, "[filtering] cannot be null");
+        this.indexName = Objects.requireNonNull(indexName, "[index_name] cannot be null");
         this.isNative = isNative;
         this.language = language;
         this.lastSeen = lastSeen;
         this.syncInfo = syncInfo;
         this.name = name;
         this.pipeline = pipeline;
-        this.scheduling = scheduling;
+        this.scheduling = Objects.requireNonNull(scheduling, "[scheduling] cannot be null");
         this.serviceType = serviceType;
-        this.status = Objects.requireNonNull(status, "connector status cannot be null");
+        this.status = Objects.requireNonNull(status, "[status] cannot be null");
         this.syncCursor = syncCursor;
         this.syncNow = syncNow;
     }
@@ -257,31 +253,24 @@ public Connector(StreamInput in) throws IOException {
     );
 
     static {
-        PARSER.declareString(optionalConstructorArg(), API_KEY_ID_FIELD);
-        PARSER.declareField(
+        PARSER.declareStringOrNull(optionalConstructorArg(), API_KEY_ID_FIELD);
+        PARSER.declareObject(
             optionalConstructorArg(),
             (p, c) -> p.map(HashMap::new, ConnectorConfiguration::fromXContent),
-            CONFIGURATION_FIELD,
-            ObjectParser.ValueType.OBJECT
+            CONFIGURATION_FIELD
         );
-        PARSER.declareField(
+        PARSER.declareObject(
             optionalConstructorArg(),
             (p, c) -> p.map(HashMap::new, ConnectorCustomSchedule::fromXContent),
-            CUSTOM_SCHEDULING_FIELD,
-            ObjectParser.ValueType.OBJECT
-        );
-        PARSER.declareString(optionalConstructorArg(), DESCRIPTION_FIELD);
-        PARSER.declareString(optionalConstructorArg(), ERROR_FIELD);
-        PARSER.declareField(
-            optionalConstructorArg(),
-            (p, c) -> ConnectorFeatures.fromXContent(p),
-            FEATURES_FIELD,
-            ObjectParser.ValueType.OBJECT
+            CUSTOM_SCHEDULING_FIELD
         );
+        PARSER.declareStringOrNull(optionalConstructorArg(), DESCRIPTION_FIELD);
+        PARSER.declareStringOrNull(optionalConstructorArg(), ERROR_FIELD);
+        PARSER.declareObjectOrNull(optionalConstructorArg(), (p, c) -> ConnectorFeatures.fromXContent(p), null, FEATURES_FIELD);
         PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> ConnectorFiltering.fromXContent(p), FILTERING_FIELD);
-        PARSER.declareString(optionalConstructorArg(), INDEX_NAME_FIELD);
+        PARSER.declareStringOrNull(optionalConstructorArg(), INDEX_NAME_FIELD);
         PARSER.declareBoolean(optionalConstructorArg(), IS_NATIVE_FIELD);
-        PARSER.declareString(optionalConstructorArg(), LANGUAGE_FIELD);
+        PARSER.declareStringOrNull(optionalConstructorArg(), LANGUAGE_FIELD);
         PARSER.declareField(
             optionalConstructorArg(),
             (p, c) -> p.currentToken() == XContentParser.Token.VALUE_NULL ? null : Instant.parse(p.text()),
@@ -330,32 +319,17 @@ public Connector(StreamInput in) throws IOException {
             ObjectParser.ValueType.STRING_OR_NULL
         );
 
-        PARSER.declareString(optionalConstructorArg(), NAME_FIELD);
-        PARSER.declareField(
-            optionalConstructorArg(),
-            (p, c) -> ConnectorIngestPipeline.fromXContent(p),
-            PIPELINE_FIELD,
-            ObjectParser.ValueType.OBJECT
-        );
-        PARSER.declareField(
-            optionalConstructorArg(),
-            (p, c) -> ConnectorScheduling.fromXContent(p),
-            SCHEDULING_FIELD,
-            ObjectParser.ValueType.OBJECT
-        );
-        PARSER.declareString(optionalConstructorArg(), SERVICE_TYPE_FIELD);
+        PARSER.declareStringOrNull(optionalConstructorArg(), NAME_FIELD);
+        PARSER.declareObjectOrNull(optionalConstructorArg(), (p, c) -> ConnectorIngestPipeline.fromXContent(p), null, PIPELINE_FIELD);
+        PARSER.declareObject(optionalConstructorArg(), (p, c) -> ConnectorScheduling.fromXContent(p), SCHEDULING_FIELD);
+        PARSER.declareStringOrNull(optionalConstructorArg(), SERVICE_TYPE_FIELD);
         PARSER.declareField(
             optionalConstructorArg(),
             (p, c) -> ConnectorStatus.connectorStatus(p.text()),
             STATUS_FIELD,
             ObjectParser.ValueType.STRING
         );
-        PARSER.declareField(
-            optionalConstructorArg(),
-            (parser, context) -> parser.map(),
-            SYNC_CURSOR_FIELD,
-            ObjectParser.ValueType.OBJECT_OR_NULL
-        );
+        PARSER.declareObjectOrNull(optionalConstructorArg(), (p, c) -> p.map(), null, SYNC_CURSOR_FIELD);
         PARSER.declareBoolean(optionalConstructorArg(), SYNC_NOW_FIELD);
     }
 
@@ -375,59 +349,30 @@ public static Connector fromXContent(XContentParser parser, String docId) throws
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
         {
+            // The "id": connectorId is included in GET and LIST responses to provide the connector's docID.
+            // Note: This ID is not written to the Elasticsearch index; it's only for API response purposes.
             if (connectorId != null) {
                 builder.field(ID_FIELD.getPreferredName(), connectorId);
             }
-            if (apiKeyId != null) {
-                builder.field(API_KEY_ID_FIELD.getPreferredName(), apiKeyId);
-            }
-            if (configuration != null) {
-                builder.xContentValuesMap(CONFIGURATION_FIELD.getPreferredName(), configuration);
-            }
-            if (customScheduling != null) {
-                builder.xContentValuesMap(CUSTOM_SCHEDULING_FIELD.getPreferredName(), customScheduling);
-            }
-            if (description != null) {
-                builder.field(DESCRIPTION_FIELD.getPreferredName(), description);
-            }
-            if (error != null) {
-                builder.field(ERROR_FIELD.getPreferredName(), error);
-            }
-            if (features != null) {
-                builder.field(FEATURES_FIELD.getPreferredName(), features);
-            }
-            if (filtering != null) {
-                builder.xContentList(FILTERING_FIELD.getPreferredName(), filtering);
-            }
-            if (indexName != null) {
-                builder.field(INDEX_NAME_FIELD.getPreferredName(), indexName);
-            }
+            builder.field(API_KEY_ID_FIELD.getPreferredName(), apiKeyId);
+            builder.xContentValuesMap(CONFIGURATION_FIELD.getPreferredName(), configuration);
+            builder.xContentValuesMap(CUSTOM_SCHEDULING_FIELD.getPreferredName(), customScheduling);
+            builder.field(DESCRIPTION_FIELD.getPreferredName(), description);
+            builder.field(ERROR_FIELD.getPreferredName(), error);
+            builder.field(FEATURES_FIELD.getPreferredName(), features);
+            builder.xContentList(FILTERING_FIELD.getPreferredName(), filtering);
+            builder.field(INDEX_NAME_FIELD.getPreferredName(), indexName);
             builder.field(IS_NATIVE_FIELD.getPreferredName(), isNative);
-            if (language != null) {
-                builder.field(LANGUAGE_FIELD.getPreferredName(), language);
-            }
+            builder.field(LANGUAGE_FIELD.getPreferredName(), language);
             builder.field(LAST_SEEN_FIELD.getPreferredName(), lastSeen);
-            if (syncInfo != null) {
-                syncInfo.toXContent(builder, params);
-            }
-            if (name != null) {
-                builder.field(NAME_FIELD.getPreferredName(), name);
-            }
-            if (pipeline != null) {
-                builder.field(PIPELINE_FIELD.getPreferredName(), pipeline);
-            }
-            if (scheduling != null) {
-                builder.field(SCHEDULING_FIELD.getPreferredName(), scheduling);
-            }
-            if (serviceType != null) {
-                builder.field(SERVICE_TYPE_FIELD.getPreferredName(), serviceType);
-            }
-            if (syncCursor != null) {
-                builder.field(SYNC_CURSOR_FIELD.getPreferredName(), syncCursor);
-            }
+            syncInfo.toXContent(builder, params);
+            builder.field(NAME_FIELD.getPreferredName(), name);
+            builder.field(PIPELINE_FIELD.getPreferredName(), pipeline);
+            builder.field(SCHEDULING_FIELD.getPreferredName(), scheduling);
+            builder.field(SERVICE_TYPE_FIELD.getPreferredName(), serviceType);
+            builder.field(SYNC_CURSOR_FIELD.getPreferredName(), syncCursor);
             builder.field(STATUS_FIELD.getPreferredName(), status.toString());
             builder.field(SYNC_NOW_FIELD.getPreferredName(), syncNow);
-
         }
         builder.endObject();
         return builder;
@@ -608,7 +553,6 @@ public static class Builder {
         private String indexName;
         private boolean isNative = false;
         private String language;
-
         private Instant lastSeen;
         private ConnectorSyncInfo syncInfo = new ConnectorSyncInfo.Builder().build();
         private String name;
diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PostConnectorAction.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PostConnectorAction.java
index 6570b111d8a0e..947c2f63d4950 100644
--- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PostConnectorAction.java
+++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PostConnectorAction.java
@@ -29,6 +29,7 @@
 import java.io.IOException;
 import java.util.Objects;
 
+import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
 import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
 public class PostConnectorAction extends ActionType<PostConnectorAction.Response> {
@@ -44,7 +45,6 @@ public static class Request extends ActionRequest implements ToXContentObject {
 
         @Nullable
         private final String description;
-        @Nullable
         private final String indexName;
         @Nullable
         private final Boolean isNative;
@@ -67,7 +67,7 @@ public Request(String description, String indexName, Boolean isNative, String la
         public Request(StreamInput in) throws IOException {
             super(in);
             this.description = in.readOptionalString();
-            this.indexName = in.readOptionalString();
+            this.indexName = in.readString();
             this.isNative = in.readOptionalBoolean();
             this.language = in.readOptionalString();
             this.name = in.readOptionalString();
@@ -89,7 +89,7 @@ public Request(StreamInput in) throws IOException {
 
         static {
             PARSER.declareString(optionalConstructorArg(), new ParseField("description"));
-            PARSER.declareString(optionalConstructorArg(), new ParseField("index_name"));
+            PARSER.declareString(constructorArg(), new ParseField("index_name"));
             PARSER.declareBoolean(optionalConstructorArg(), new ParseField("is_native"));
             PARSER.declareString(optionalConstructorArg(), new ParseField("language"));
             PARSER.declareString(optionalConstructorArg(), new ParseField("name"));
@@ -115,9 +115,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                 if (description != null) {
                     builder.field("description", description);
                 }
-                if (indexName != null) {
-                    builder.field("index_name", indexName);
-                }
+                builder.field("index_name", indexName);
                 if (isNative != null) {
                     builder.field("is_native", isNative);
                 }
@@ -144,7 +142,7 @@ public ActionRequestValidationException validate() {
         public void writeTo(StreamOutput out) throws IOException {
             super.writeTo(out);
             out.writeOptionalString(description);
-            out.writeOptionalString(indexName);
+            out.writeString(indexName);
             out.writeOptionalBoolean(isNative);
             out.writeOptionalString(language);
             out.writeOptionalString(name);
diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PutConnectorAction.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PutConnectorAction.java
index 6abb5ef548be5..592be3a6b37ab 100644
--- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PutConnectorAction.java
+++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/action/PutConnectorAction.java
@@ -49,7 +49,6 @@ public static class Request extends ActionRequest implements ToXContentObject {
 
         @Nullable
         private final String description;
-        @Nullable
         private final String indexName;
         @Nullable
         private final Boolean isNative;
@@ -82,7 +81,7 @@ public Request(StreamInput in) throws IOException {
             super(in);
             this.connectorId = in.readString();
             this.description = in.readOptionalString();
-            this.indexName = in.readOptionalString();
+            this.indexName = in.readString();
             this.isNative = in.readOptionalBoolean();
             this.language = in.readOptionalString();
             this.name = in.readOptionalString();
@@ -131,9 +130,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                 if (description != null) {
                     builder.field("description", description);
                 }
-                if (indexName != null) {
-                    builder.field("index_name", indexName);
-                }
+                builder.field("index_name", indexName);
                 if (isNative != null) {
                     builder.field("is_native", isNative);
                 }
@@ -168,7 +165,7 @@ public void writeTo(StreamOutput out) throws IOException {
             super.writeTo(out);
             out.writeString(connectorId);
             out.writeOptionalString(description);
-            out.writeOptionalString(indexName);
+            out.writeString(indexName);
             out.writeOptionalBoolean(isNative);
             out.writeOptionalString(language);
             out.writeOptionalString(name);
diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTestUtils.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTestUtils.java
index 200b14109059b..6a16e6f183383 100644
--- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTestUtils.java
+++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTestUtils.java
@@ -245,15 +245,15 @@ public static Connector getRandomConnector() {
             .setDescription(randomFrom(new String[] { null, randomAlphaOfLength(10) }))
             .setError(randomFrom(new String[] { null, randomAlphaOfLength(10) }))
             .setFeatures(randomBoolean() ? getRandomConnectorFeatures() : null)
-            .setFiltering(randomBoolean() ? List.of(getRandomConnectorFiltering()) : null)
-            .setIndexName(randomFrom(new String[] { null, randomAlphaOfLength(10) }))
+            .setFiltering(List.of(getRandomConnectorFiltering()))
+            .setIndexName(randomAlphaOfLength(10))
             .setIsNative(randomBoolean())
             .setLanguage(randomFrom(new String[] { null, randomAlphaOfLength(10) }))
             .setLastSeen(randomFrom(new Instant[] { null, Instant.ofEpochMilli(randomLong()) }))
             .setSyncInfo(getRandomConnectorSyncInfo())
             .setName(randomFrom(new String[] { null, randomAlphaOfLength(10) }))
             .setPipeline(randomBoolean() ? getRandomConnectorIngestPipeline() : null)
-            .setScheduling(randomBoolean() ? getRandomConnectorScheduling() : null)
+            .setScheduling(getRandomConnectorScheduling())
             .setStatus(getRandomConnectorStatus())
             .setSyncCursor(randomBoolean() ? Map.of(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLengthBetween(5, 10)) : null)
             .setSyncNow(randomBoolean())
diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTests.java
index c08cd37218aeb..9401a2a58403e 100644
--- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTests.java
+++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/ConnectorTests.java
@@ -216,6 +216,64 @@ public void testToXContent() throws IOException {
         assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON);
     }
 
+    public void testToContent_WithNullValues() throws IOException {
+        String connectorId = "test-connector";
+        String content = XContentHelper.stripWhitespace("""
+            {
+               "api_key_id": null,
+               "custom_scheduling":{},
+               "configuration":{},
+               "description": null,
+               "features": null,
+               "filtering":[],
+               "index_name": "search-test",
+               "is_native": false,
+               "language": null,
+               "last_access_control_sync_error": null,
+               "last_access_control_sync_scheduled_at": null,
+               "last_access_control_sync_status": null,
+               "last_incremental_sync_scheduled_at": null,
+               "last_seen": null,
+               "last_sync_error": null,
+               "last_sync_scheduled_at": null,
+               "last_sync_status": null,
+               "last_synced": null,
+               "name": null,
+               "pipeline":{
+                  "extract_binary_content":true,
+                  "name":"ent-search-generic-ingestion",
+                  "reduce_whitespace":true,
+                  "run_ml_inference":false
+               },
+               "scheduling":{
+                  "access_control":{
+                     "enabled":false,
+                     "interval":"0 0 0 * * ?"
+                  },
+                  "full":{
+                     "enabled":false,
+                     "interval":"0 0 0 * * ?"
+                  },
+                  "incremental":{
+                     "enabled":false,
+                     "interval":"0 0 0 * * ?"
+                  }
+               },
+               "service_type": null,
+               "status": "needs_configuration",
+               "sync_now":false
+            }""");
+
+        Connector connector = Connector.fromXContentBytes(new BytesArray(content), connectorId, XContentType.JSON);
+        boolean humanReadable = true;
+        BytesReference originalBytes = toShuffledXContent(connector, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable);
+        Connector parsed;
+        try (XContentParser parser = createParser(XContentType.JSON.xContent(), originalBytes)) {
+            parsed = Connector.fromXContent(parser, connectorId);
+        }
+        assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON);
+    }
+
     private void assertTransportSerialization(Connector testInstance) throws IOException {
         Connector deserializedInstance = copyInstance(testInstance);
         assertNotSame(testInstance, deserializedInstance);