From 72496d371290850cbc2acee8be82cce17fa3480e Mon Sep 17 00:00:00 2001 From: Sashidhar Thallam Date: Fri, 19 Jul 2019 01:05:19 +0530 Subject: [PATCH] =?UTF-8?q?#7858=20Throwing=20UnsupportedOperationExceptio?= =?UTF-8?q?n=20from=20ImmutableDrui=E2=80=A6=20(#7933)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods. * 1. Turning ImmutableDruidDataSource into a data container. 2. Adding a Util method to be used in tests for checking equality of ImmutableDruidDataSource objects. * Removing unused method * Fixing assert equals * Fixing assert equals in TestUtils.java * Adding java doc comments, Using ExpectedException in tests * Fixing test cases * Fixed expected exception message in tests * fixed line width * line width fix * code style fixes * code indentation fixes * fixing method name --- .../client/ImmutableDruidDataSource.java | 48 +++++++--- .../client/ImmutableDruidDataSourceTest.java | 84 +++++++++++++---- .../server/http/DataSourcesResourceTest.java | 15 +-- .../ImmutableDruidDataSourceTestUtils.java | 94 +++++++++++++++++++ 4 files changed, 204 insertions(+), 37 deletions(-) create mode 100644 server/src/test/java/org/apache/druid/test/utils/ImmutableDruidDataSourceTestUtils.java diff --git a/server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java b/server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java index 7ce3fd6dffb..d92f90f29f1 100644 --- a/server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java +++ b/server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java @@ -22,6 +22,7 @@ package org.apache.druid.client; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; @@ -30,10 +31,10 @@ import org.apache.druid.timeline.SegmentId; import java.util.Collection; import java.util.Map; -import java.util.Objects; /** - * An immutable collection of metadata of segments ({@link DataSegment} objects), belonging to a particular data source. + * An immutable collection of metadata of segments ({@link DataSegment} objects), belonging to a particular data + * source. * * @see DruidDataSource - a mutable counterpart of this class */ @@ -120,11 +121,42 @@ public class ImmutableDruidDataSource + "'}"; } + /** + * ImmutableDruidDataSource should be considered a container, not a data class. The idea is the same as behind + * prohibiting/limiting equals() (and therefore usage as HashSet/HashMap keys) of DataSegment: see + * https://github.com/apache/incubator-druid/issues/6358. When somebody wants to deduplicate ImmutableDruidDataSource + * objects, they would need to put them into a Map and resolve conflicts by name + * manually. + * + * See https://github.com/apache/incubator-druid/issues/7858 + */ @Override public boolean equals(Object o) { - // Note: this method is not well-defined. It should instead just throw UnsupportedOperationsException. - // See https://github.com/apache/incubator-druid/issues/7858. + throw new UnsupportedOperationException("ImmutableDruidDataSource shouldn't be used as the key in containers"); + } + + /** + * ImmutableDruidDataSource should be considered a container, not a data class. The idea is the same as behind + * prohibiting/limiting hashCode() (and therefore usage as HashSet/HashMap keys) of DataSegment: see + * https://github.com/apache/incubator-druid/issues/6358. When somebody wants to deduplicate ImmutableDruidDataSource + * objects, they would need to put them into a Map and resolve conflicts by name + * manually. + * + * See https://github.com/apache/incubator-druid/issues/7858 + */ + @Override + public int hashCode() + { + throw new UnsupportedOperationException("ImmutableDruidDataSource shouldn't be used as the key in containers"); + } + + /** + * This method should only be used in tests. + */ + @VisibleForTesting + public boolean equalsForTesting(Object o) + { if (this == o) { return true; } @@ -144,12 +176,4 @@ public class ImmutableDruidDataSource return this.idToSegments.equals(that.idToSegments); } - - @Override - public int hashCode() - { - // Note: this method is not well-defined. It should instead just throw UnsupportedOperationsException. - // See https://github.com/apache/incubator-druid/issues/7858. - return Objects.hash(name, properties); - } } diff --git a/server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java b/server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java index 4514113013a..bf3d47f4ca2 100644 --- a/server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java +++ b/server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java @@ -26,39 +26,85 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.test.utils.ImmutableDruidDataSourceTestUtils; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.DataSegment.PruneLoadSpecHolder; -import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.io.IOException; public class ImmutableDruidDataSourceTest { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Test public void testSerde() throws IOException { - final DataSegment segment = new DataSegment( - "test", - Intervals.of("2017/2018"), - "version", - null, - ImmutableList.of("dim1", "dim2"), - ImmutableList.of("met1", "met2"), - null, - 1, - 100L, - PruneLoadSpecHolder.DEFAULT - ); - final ImmutableDruidDataSource dataSource = new ImmutableDruidDataSource( - "test", - ImmutableMap.of("prop1", "val1", "prop2", "val2"), - ImmutableSortedMap.of(segment.getId(), segment) - ); + final DataSegment segment = getTestSegment(); + final ImmutableDruidDataSource dataSource = getImmutableDruidDataSource(segment); final ObjectMapper objectMapper = new DefaultObjectMapper() .setInjectableValues(new Std().addValue(PruneLoadSpecHolder.class, PruneLoadSpecHolder.DEFAULT)); final String json = objectMapper.writeValueAsString(dataSource); - Assert.assertEquals(dataSource, objectMapper.readValue(json, ImmutableDruidDataSource.class)); + + ImmutableDruidDataSourceTestUtils.assertEquals(dataSource, objectMapper.readValue(json, + ImmutableDruidDataSource.class)); + } + + @Test + public void testEqualsMethodThrowsUnsupportedOperationException() + { + expectedException.expect(UnsupportedOperationException.class); + expectedException.expectMessage("ImmutableDruidDataSource shouldn't be used as the key in containers"); + + final DataSegment segment1 = getTestSegment(); + + final ImmutableDruidDataSource dataSource1 = getImmutableDruidDataSource(segment1); + + final DataSegment segment2 = getTestSegment(); + + final ImmutableDruidDataSource dataSource2 = getImmutableDruidDataSource(segment2); + + dataSource1.equals(dataSource2); + } + + private ImmutableDruidDataSource getImmutableDruidDataSource(DataSegment segment1) + { + return new ImmutableDruidDataSource( + "test", + ImmutableMap.of("prop1", "val1", "prop2", "val2"), + ImmutableSortedMap.of(segment1.getId(), segment1) + ); + } + + private DataSegment getTestSegment() + { + return new DataSegment( + "test", + Intervals.of("2017/2018"), + "version", + null, + ImmutableList.of("dim1", "dim2"), + ImmutableList.of("met1", "met2"), + null, + 1, + 100L, + PruneLoadSpecHolder.DEFAULT + ); + } + + @Test + public void testHashCodeMethodThrowsUnsupportedOperationException() + { + expectedException.expect(UnsupportedOperationException.class); + expectedException.expectMessage("ImmutableDruidDataSource shouldn't be used as the key in containers"); + final DataSegment segment = getTestSegment(); + final ImmutableDruidDataSource dataSource = getImmutableDruidDataSource(segment); + + dataSource.hashCode(); } } diff --git a/server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java b/server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java index cb463fd5175..e200e700e8e 100644 --- a/server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java @@ -50,6 +50,7 @@ import org.apache.druid.server.security.AuthenticationResult; import org.apache.druid.server.security.Authorizer; import org.apache.druid.server.security.AuthorizerMapper; import org.apache.druid.server.security.Resource; +import org.apache.druid.test.utils.ImmutableDruidDataSourceTestUtils; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.SegmentId; import org.apache.druid.timeline.TimelineObjectHolder; @@ -68,7 +69,6 @@ import javax.ws.rs.core.Response; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -179,9 +179,9 @@ public class DataSourcesResourceTest Set result = (Set) response.getEntity(); Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(2, result.size()); - Assert.assertEquals( - listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()), - new HashSet<>(result) + ImmutableDruidDataSourceTestUtils.assertEquals( + listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toList()), + new ArrayList<>(result) ); response = dataSourcesResource.getQueryableDataSources(null, null, request); @@ -254,7 +254,10 @@ public class DataSourcesResourceTest Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(1, result.size()); - Assert.assertEquals(listDataSources.get(0).toImmutableDruidDataSource(), Iterables.getOnlyElement(result)); + ImmutableDruidDataSourceTestUtils.assertEquals( + listDataSources.get(0).toImmutableDruidDataSource(), + Iterables.getOnlyElement(result) + ); response = dataSourcesResource.getQueryableDataSources(null, null, request); List result1 = (List) response.getEntity(); @@ -312,7 +315,7 @@ public class DataSourcesResourceTest Response response = dataSourcesResource.getDataSource("datasource1", "full"); ImmutableDruidDataSource result = (ImmutableDruidDataSource) response.getEntity(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(dataSource1.toImmutableDruidDataSource(), result); + ImmutableDruidDataSourceTestUtils.assertEquals(dataSource1.toImmutableDruidDataSource(), result); EasyMock.verify(inventoryView, server); } diff --git a/server/src/test/java/org/apache/druid/test/utils/ImmutableDruidDataSourceTestUtils.java b/server/src/test/java/org/apache/druid/test/utils/ImmutableDruidDataSourceTestUtils.java new file mode 100644 index 00000000000..80036debd09 --- /dev/null +++ b/server/src/test/java/org/apache/druid/test/utils/ImmutableDruidDataSourceTestUtils.java @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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.apache.druid.test.utils; + +import org.apache.druid.client.ImmutableDruidDataSource; +import org.junit.Assert; + +import javax.annotation.Nullable; +import java.util.List; + +public class ImmutableDruidDataSourceTestUtils +{ + + /** + * This method is to check equality of {@link ImmutableDruidDataSource} objects to be called from test code. + * @param expected expected object + * @param actual actual object + */ + public static void assertEquals(ImmutableDruidDataSource expected, ImmutableDruidDataSource actual) + { + if (checkEquals(expected, actual)) { + return; + } else { + throw new AssertionError("Expected and actual objects are not equal as per ImmutableDruidDataSource's " + + "equalsForTesting() method"); + } + } + + private static boolean checkEquals( + @Nullable ImmutableDruidDataSource expected, + @Nullable ImmutableDruidDataSource actual + ) + { + if (expected == null) { + return actual == null; + } + + return expected.equalsForTesting(actual); + } + + /** + * This method is to check the equality of a list of {@link ImmutableDruidDataSource} objects to be called from + * test code + * @param expected expected list + * @param actual actual list + * @return + */ + public static boolean assertEquals(List expected, List actual) + { + if (expected == null) { + return actual == null; + } + + Assert.assertEquals("expected and actual ImmutableDruidDataSource lists should be of equal size", + expected.size(), actual.size()); + + for (ImmutableDruidDataSource e : expected) { + if (!contains(e, actual)) { + throw new AssertionError("Expected and actual objects are not equal as per " + + "ImmutableDruidDataSource's equalsForTesting()" + " method"); + } + } + return true; + } + + private static boolean contains(ImmutableDruidDataSource expected, List actualList) + { + // Iterate over actual list to see if the element expected is present, if not return false + for (ImmutableDruidDataSource ds : actualList) { + if (ds.equalsForTesting(expected)) { + return true; + } + } + return false; + } + +}