This is an automated email from the ASF dual-hosted git repository.
leventov pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git
The following commit(s) were added to refs/heads/master by this push:
new 72496d3 #7858 Throwing UnsupportedOperationException from
ImmutableDrui… (#7933)
72496d3 is described below
commit 72496d371290850cbc2acee8be82cce17fa3480e
Author: Sashidhar Thallam <[email protected]>
AuthorDate: Fri Jul 19 01:05:19 2019 +0530
#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
---
.../druid/client/ImmutableDruidDataSource.java | 48 ++++++++---
.../druid/client/ImmutableDruidDataSourceTest.java | 84 ++++++++++++++-----
.../druid/server/http/DataSourcesResourceTest.java | 15 ++--
.../utils/ImmutableDruidDataSourceTestUtils.java | 94 ++++++++++++++++++++++
4 files changed, 204 insertions(+), 37 deletions(-)
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 7ce3fd6..d92f90f 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<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.
- // 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<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);
- }
}
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 4514113..bf3d47f 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 cb463fd..e200e70 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<ImmutableDruidDataSource> result = (Set<ImmutableDruidDataSource>)
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<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);
}
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 0000000..80036de
--- /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<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;
+ }
+
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
