#7858 Throwing UnsupportedOperationException from ImmutableDrui… (#7933)

* #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

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<String, ImmutableDruidDataSource> 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.
+    throw new UnsupportedOperationException("ImmutableDruidDataSource shouldn't be used as the key in containers");
-    // See https://github.com/apache/incubator-druid/issues/7858.
+  }
+
+  /**
+   * 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<String, ImmutableDruidDataSource> 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);
-  }
 }

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(
+    final DataSegment segment = getTestSegment();
-        "test",
+    final ImmutableDruidDataSource dataSource = getImmutableDruidDataSource(segment);
-        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 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();
   }
 }

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<ImmutableDruidDataSource> result = (Set<ImmutableDruidDataSource>) response.getEntity();
     Assert.assertEquals(200, response.getStatus());
     Assert.assertEquals(2, result.size());
-    Assert.assertEquals(
+    ImmutableDruidDataSourceTestUtils.assertEquals(
-        listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()),
+        listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toList()),
-        new HashSet<>(result)
+        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<String> result1 = (List<String>) 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);
   }
 

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<ImmutableDruidDataSource> expected, List<ImmutableDruidDataSource> 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<ImmutableDruidDataSource> 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;
+  }
+
+}