Address PR comments

Signed-off-by: Shivansh Arora <hishiv@amazon.com>
opensearch-project · Jun 5, 2024 · dd7babd · dd7babd
1 parent 251daa9
commit dd7babd
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 74 deletions.
diff --git a/server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java b/server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java
@@ -32,6 +32,8 @@
 
 package org.opensearch.cluster.block;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.common.Nullable;
 import org.opensearch.common.annotation.PublicApi;
@@ -60,7 +62,7 @@
  */
 @PublicApi(since = "1.0.0")
 public class ClusterBlock implements Writeable, ToXContentFragment {
-
+    private static final Logger logger = LogManager.getLogger(ClusterBlock.class);
     static final String KEY_UUID = "uuid";
     static final String KEY_DESCRIPTION = "description";
     static final String KEY_RETRYABLE = "retryable";
@@ -232,15 +234,15 @@ public static ClusterBlock fromXContent(XContentParser parser, int id) throws IO
                         allowReleaseResources = parser.booleanValue();
                         break;
                     default:
-                        throw new IllegalArgumentException("unknown field [" + currentFieldName + "]");
+                        logger.warn("unknown field [{}]", currentFieldName);
                 }
             } else if (token == XContentParser.Token.START_ARRAY) {
                 if (currentFieldName.equals(KEY_LEVELS)) {
                     while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
                         levels.add(ClusterBlockLevel.fromString(parser.text(), Locale.ROOT));
                     }
                 } else {
-                    throw new IllegalArgumentException("unknown field [" + currentFieldName + "]");
+                    logger.warn("unknown field [{}]", currentFieldName);
                 }
             } else {
                 throw new IllegalArgumentException("unexpected token [" + token + "]");

diff --git a/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java
@@ -32,6 +32,8 @@
 
 package org.opensearch.cluster.block;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.cluster.AbstractDiffable;
 import org.opensearch.cluster.Diff;
 import org.opensearch.cluster.metadata.IndexMetadata;
@@ -68,6 +70,7 @@
  */
 @PublicApi(since = "1.0.0")
 public class ClusterBlocks extends AbstractDiffable<ClusterBlocks> implements ToXContentFragment {
+    private static final Logger logger = LogManager.getLogger(ClusterBlocks.class);
     public static final ClusterBlocks EMPTY_CLUSTER_BLOCK = new ClusterBlocks(emptySet(), Map.of());
 
     private final Set<ClusterBlock> global;
@@ -368,6 +371,10 @@ public static Builder builder() {
         return new Builder();
     }
 
+    public static Builder builder(ClusterBlocks clusterBlocks) {
+        return new Builder(clusterBlocks);
+    }
+
     /**
      * Builder for cluster blocks.
      *
@@ -382,6 +389,11 @@ public static class Builder {
 
         public Builder() {}
 
+        public Builder(ClusterBlocks clusterBlocks) {
+            this.global.addAll(clusterBlocks.global());
+            this.indices.putAll(clusterBlocks.indices());
+        }
+
         public Builder blocks(ClusterBlocks blocks) {
             global.addAll(blocks.global());
             for (final Map.Entry<String, Set<ClusterBlock>> entry : blocks.indices().entrySet()) {
@@ -555,7 +567,7 @@ public static ClusterBlocks fromXContent(XContentParser parser) throws IOExcepti
                         }
                         break;
                     default:
-                        throw new IllegalArgumentException("unknown field [" + currentFieldName + "]");
+                        logger.warn("unknown field [{}]", currentFieldName);
                 }
             }
             return builder.build();

diff --git a/server/src/test/java/org/opensearch/cluster/block/ClusterBlockTests.java b/server/src/test/java/org/opensearch/cluster/block/ClusterBlockTests.java
@@ -45,7 +45,6 @@
 import org.opensearch.core.xcontent.ToXContent;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.test.OpenSearchTestCase;
-import org.opensearch.test.XContentTestUtils;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -173,43 +172,16 @@ public void testToXContent_GatewayMode() throws IOException {
     }
 
     public void testFromXContent() throws IOException {
-        doFromXContentTestWithRandomFields(false);
-    }
-
-    public void testFromXContentWithRandomFields() throws IOException {
-        doFromXContentTestWithRandomFields(true);
-    }
-
-    private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
         ClusterBlock clusterBlock = randomClusterBlock();
         boolean humanReadable = randomBoolean();
         final MediaType mediaType = MediaTypeRegistry.JSON;
         BytesReference originalBytes = toShuffledXContent(clusterBlock, mediaType, ToXContent.EMPTY_PARAMS, humanReadable);
-
-        if (addRandomFields) {
-            String unsupportedField = "unsupported_field";
-            BytesReference mutated = BytesReference.bytes(
-                XContentTestUtils.insertIntoXContent(
-                    mediaType.xContent(),
-                    originalBytes,
-                    Collections.singletonList(Integer.toString(clusterBlock.id())),
-                    () -> unsupportedField,
-                    () -> randomAlphaOfLengthBetween(3, 10)
-                )
-            );
-            IllegalArgumentException e = expectThrows(
-                IllegalArgumentException.class,
-                () -> ClusterBlock.fromXContent(createParser(mediaType.xContent(), mutated), clusterBlock.id())
-            );
-            assertEquals(e.getMessage(), "unknown field [" + unsupportedField + "]");
-        } else {
-            ClusterBlock parsedBlock = ClusterBlock.fromXContent(createParser(mediaType.xContent(), originalBytes), clusterBlock.id());
-            assertEquals(clusterBlock, parsedBlock);
-            assertEquals(clusterBlock.description(), parsedBlock.description());
-            assertEquals(clusterBlock.retryable(), parsedBlock.retryable());
-            assertEquals(clusterBlock.disableStatePersistence(), parsedBlock.disableStatePersistence());
-            assertArrayEquals(clusterBlock.levels().toArray(), parsedBlock.levels().toArray());
-        }
+        ClusterBlock parsedBlock = ClusterBlock.fromXContent(createParser(mediaType.xContent(), originalBytes), clusterBlock.id());
+        assertEquals(clusterBlock, parsedBlock);
+        assertEquals(clusterBlock.description(), parsedBlock.description());
+        assertEquals(clusterBlock.retryable(), parsedBlock.retryable());
+        assertEquals(clusterBlock.disableStatePersistence(), parsedBlock.disableStatePersistence());
+        assertArrayEquals(clusterBlock.levels().toArray(), parsedBlock.levels().toArray());
     }
 
     static String getExpectedXContentFragment(ClusterBlock clusterBlock, String indent, boolean gatewayMode) {
@@ -261,10 +233,14 @@ static String getExpectedXContentFragment(ClusterBlock clusterBlock, String inde
     }
 
     static ClusterBlock randomClusterBlock() {
+        return clusterBlockWithId(randomInt());
+    }
+
+    static ClusterBlock clusterBlockWithId(int id) {
         final String uuid = randomBoolean() ? UUIDs.randomBase64UUID() : null;
         final List<ClusterBlockLevel> levels = Arrays.asList(ClusterBlockLevel.values());
         return new ClusterBlock(
-            randomInt(),
+            id,
             uuid,
             "cluster block #" + randomInt(),
             randomBoolean(),

diff --git a/server/src/test/java/org/opensearch/cluster/block/ClusterBlocksTests.java b/server/src/test/java/org/opensearch/cluster/block/ClusterBlocksTests.java
@@ -22,13 +22,16 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static org.opensearch.cluster.block.ClusterBlockTests.clusterBlockWithId;
 import static org.opensearch.cluster.block.ClusterBlockTests.getExpectedXContentFragment;
 import static org.opensearch.cluster.block.ClusterBlockTests.randomClusterBlock;
+import static org.opensearch.core.xcontent.XContentHelper.toXContent;
 
 public class ClusterBlocksTests extends OpenSearchTestCase {
     public void testToXContent() throws IOException {
@@ -86,46 +89,39 @@ public void testToXContent() throws IOException {
     }
 
     public void testFromXContent() throws IOException {
-        doFromXContentTestWithRandomFields(false);
-    }
-
-    public void testFromXContentWithRandomFields() throws IOException {
-        doFromXContentTestWithRandomFields(true);
-    }
-
-    private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
         ClusterBlocks clusterBlocks = randomClusterBlocks();
         boolean humanReadable = randomBoolean();
         final MediaType mediaType = MediaTypeRegistry.JSON;
         BytesReference originalBytes = toShuffledXContent(clusterBlocks, mediaType, ToXContent.EMPTY_PARAMS, humanReadable);
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, originalBytes)) {
+            ClusterBlocks parsedClusterBlocks = ClusterBlocks.fromXContent(parser);
+            assertEquals(clusterBlocks.global().size(), parsedClusterBlocks.global().size());
+            assertEquals(clusterBlocks.indices().size(), parsedClusterBlocks.indices().size());
+            clusterBlocks.global().forEach(clusterBlock -> assertTrue(parsedClusterBlocks.global().contains(clusterBlock)));
+            clusterBlocks.indices().forEach((key, value) -> {
+                assertTrue(parsedClusterBlocks.indices().containsKey(key));
+                value.forEach(clusterBlock -> assertTrue(parsedClusterBlocks.indices().get(key).contains(clusterBlock)));
+            });
+        }
+    }
 
-        if (addRandomFields) {
-            String unsupportedField = "unsupported_field";
-            BytesReference mutated = BytesReference.bytes(
-                XContentTestUtils.insertIntoXContent(
-                    mediaType.xContent(),
-                    originalBytes,
-                    Collections.singletonList("blocks"),
-                    () -> unsupportedField,
-                    () -> randomAlphaOfLengthBetween(3, 10)
-                )
-            );
-            IllegalArgumentException exception = expectThrows(
-                IllegalArgumentException.class,
-                () -> ClusterBlocks.fromXContent(createParser(mediaType.xContent(), mutated))
-            );
-            assertEquals("unknown field [" + unsupportedField + "]", exception.getMessage());
-        } else {
-            try (XContentParser parser = createParser(JsonXContent.jsonXContent, originalBytes)) {
-                ClusterBlocks parsedClusterBlocks = ClusterBlocks.fromXContent(parser);
-                assertEquals(clusterBlocks.global().size(), parsedClusterBlocks.global().size());
-                assertEquals(clusterBlocks.indices().size(), parsedClusterBlocks.indices().size());
-                clusterBlocks.global().forEach(clusterBlock -> assertTrue(parsedClusterBlocks.global().contains(clusterBlock)));
-                clusterBlocks.indices().forEach((key, value) -> {
-                    assertTrue(parsedClusterBlocks.indices().containsKey(key));
-                    value.forEach(clusterBlock -> assertTrue(parsedClusterBlocks.indices().get(key).contains(clusterBlock)));
-                });
-            }
+    public void testFromXContentWithUnknownFields() throws IOException {
+        ClusterBlocks clusterBlocks = ClusterBlocks.builder(randomClusterBlocks())
+            .addGlobalBlock(clusterBlockWithId(111)).build();
+
+        final MediaType mediaType = MediaTypeRegistry.JSON;
+        BytesReference mutated = BytesReference.bytes(
+            XContentTestUtils.insertIntoXContent(
+                mediaType.xContent(),
+                toXContent(clusterBlocks, mediaType, ToXContent.EMPTY_PARAMS, randomBoolean()),
+                List.of("blocks.global.111"),
+                () -> "unknown_field",
+                () -> Collections.singletonMap("a", "b")
+            )
+        );
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, mutated)) {
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ClusterBlocks.fromXContent(parser));
+            assertEquals("unexpected token [START_OBJECT]", e.getMessage());
         }
     }