From dd7babdb452250a09e526ccf8ff4229917fb6a07 Mon Sep 17 00:00:00 2001 From: Shivansh Arora Date: Thu, 6 Jun 2024 03:09:11 +0530 Subject: [PATCH] Address PR comments Signed-off-by: Shivansh Arora --- .../cluster/block/ClusterBlock.java | 8 ++- .../cluster/block/ClusterBlocks.java | 14 +++- .../cluster/block/ClusterBlockTests.java | 46 ++++--------- .../cluster/block/ClusterBlocksTests.java | 66 +++++++++---------- 4 files changed, 60 insertions(+), 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 index c958d7752cdbb..3794df2657dc8 100644 --- 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,7 +234,7 @@ 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)) { @@ -240,7 +242,7 @@ public static ClusterBlock fromXContent(XContentParser parser, int id) throws IO 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 index e783ac1578d8f..0483a070fa87b 100644 --- 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 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 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> 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 index 4ff20a20a9e40..f23f4e6da6dd7 100644 --- 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 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 index c1d4401760be3..00f9c21faa49b 100644 --- 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()); } }