From 3515061b78652ddc122bf078619a7ec2842ce3ba Mon Sep 17 00:00:00 2001 From: John Plaisted Date: Thu, 3 Dec 2020 14:48:20 -0800 Subject: [PATCH] fix: turn on werror for elasticsearch-dao (#59) --- build.gradle | 2 +- .../metadata/dao/browse/ESBrowseDAO.java | 17 ++++++---- ...CompleteQueryForHighCardinalityFields.java | 8 ++--- ...oCompleteQueryForLowCardinalityFields.java | 4 +-- .../metadata/dao/search/ESSearchDAO.java | 18 +++++++--- .../metadata/dao/utils/SearchUtils.java | 5 ++- .../metadata/dao/browse/BrowseDAOTest.java | 33 ++++++++++++++----- .../metadata/dao/search/ESSearchDAOTest.java | 2 +- .../metadata/dao/utils/SearchUtilsTest.java | 2 +- 9 files changed, 60 insertions(+), 31 deletions(-) diff --git a/build.gradle b/build.gradle index 87e777fa0..26a4a83f4 100644 --- a/build.gradle +++ b/build.gradle @@ -72,7 +72,7 @@ def wErrorProjects = [ project(':core-models'), project(':dao-api'), project(':dao-impl:ebean-dao'), - // project(':dao-impl:elasticsearch-dao'), + project(':dao-impl:elasticsearch-dao'), // project(':dao-impl:neo4j-dao'), project(':restli-resources'), project(':testing'), diff --git a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/browse/ESBrowseDAO.java b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/browse/ESBrowseDAO.java index aea92ad63..6a60a3ef2 100644 --- a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/browse/ESBrowseDAO.java +++ b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/browse/ESBrowseDAO.java @@ -45,9 +45,9 @@ @Slf4j public class ESBrowseDAO extends BaseBrowseDAO { private final RestHighLevelClient _client; - private final BaseBrowseConfig _config; + private final BaseBrowseConfig _config; - public ESBrowseDAO(@Nonnull RestHighLevelClient esClient, @Nonnull BaseBrowseConfig config) { + public ESBrowseDAO(@Nonnull RestHighLevelClient esClient, @Nonnull BaseBrowseConfig config) { this._client = esClient; this._config = config; } @@ -233,7 +233,7 @@ List extractEntitiesResponse(@Nonnull SearchResponse entitie final List entityMetadataArray = new ArrayList<>(); Arrays.stream(entitiesResponse.getHits().getHits()).forEach(hit -> { try { - final List allPaths = (List) hit.getSourceAsMap().get(_config.getBrowsePathFieldName()); + final List allPaths = (List) hit.getSourceAsMap().get(_config.getBrowsePathFieldName()); final String nextLevelPath = getNextLevelPath(allPaths, currentPath); if (nextLevelPath != null) { entityMetadataArray.add(new BrowseResultEntity().setName(getSimpleName(nextLevelPath)) @@ -261,10 +261,12 @@ private String getSimpleName(@Nonnull String path) { @VisibleForTesting @Nullable - static String getNextLevelPath(@Nonnull List paths, @Nonnull String currentPath) { + static String getNextLevelPath(@Nonnull List paths, @Nonnull String currentPath) { final String normalizedCurrentPath = currentPath.toLowerCase(); final int pathDepth = getPathDepth(currentPath); return paths.stream() + .filter(x -> x instanceof String) + .map(x -> (String) x) .filter(x -> x.toLowerCase().startsWith(normalizedCurrentPath) && getPathDepth(x) == (pathDepth + 1)) .findFirst() .orElse(null); @@ -296,10 +298,13 @@ public List getBrowsePaths(@Nonnull Urn urn) { if (searchHits.length == 0) { return Collections.emptyList(); } - final Map sourceMap = searchHits[0].getSourceAsMap(); + final Map sourceMap = searchHits[0].getSourceAsMap(); if (!sourceMap.containsKey(_config.getBrowsePathFieldName())) { return Collections.emptyList(); } - return (List) sourceMap.get(_config.getBrowsePathFieldName()); + + @SuppressWarnings("unchecked") + final List paths = (List) sourceMap.get(_config.getBrowsePathFieldName()); + return paths; } } diff --git a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForHighCardinalityFields.java b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForHighCardinalityFields.java index 15c4003a6..e178670c9 100644 --- a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForHighCardinalityFields.java +++ b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForHighCardinalityFields.java @@ -22,9 +22,9 @@ @Slf4j public class ESAutoCompleteQueryForHighCardinalityFields extends BaseESAutoCompleteQuery { private static final Integer DEFAULT_AUTOCOMPLETE_QUERY_SIZE = 100; - private BaseSearchConfig _config; + private final BaseSearchConfig _config; - ESAutoCompleteQueryForHighCardinalityFields(BaseSearchConfig config) { + ESAutoCompleteQueryForHighCardinalityFields(BaseSearchConfig config) { this._config = config; } @@ -63,7 +63,7 @@ StringArray getSuggestionList(@Nonnull SearchResponse searchResponse, @Nonnull S @Nonnull String input, int limit) { Set autoCompletionList = new LinkedHashSet<>(); SearchHit[] hits = searchResponse.getHits().getHits(); - Integer count = 0; + int count = 0; for (SearchHit hit : hits) { Map source = hit.getSource(); if (count >= limit) { @@ -97,7 +97,7 @@ static List decoupleArrayToGetSubstringMatch(@Nonnull Object fieldVal, @ if (!(fieldVal instanceof List)) { return Collections.singletonList(fieldVal.toString()); } - List stringVals = (List) fieldVal; + final List stringVals = (List) fieldVal; return stringVals.stream() .map(Object::toString) .filter(x -> x.toLowerCase().contains(input.toLowerCase())) diff --git a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForLowCardinalityFields.java b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForLowCardinalityFields.java index 917b6012f..f9c90bae1 100644 --- a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForLowCardinalityFields.java +++ b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESAutoCompleteQueryForLowCardinalityFields.java @@ -23,9 +23,9 @@ public class ESAutoCompleteQueryForLowCardinalityFields extends BaseESAutoCompleteQuery { private static final String DEFAULT_QUERY_ANALYZER = "lowercase_keyword"; - private BaseSearchConfig _config; + private final BaseSearchConfig _config; - ESAutoCompleteQueryForLowCardinalityFields(BaseSearchConfig config) { + ESAutoCompleteQueryForLowCardinalityFields(BaseSearchConfig config) { this._config = config; } diff --git a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESSearchDAO.java b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESSearchDAO.java index 899533fcf..bd04a49bb 100644 --- a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESSearchDAO.java +++ b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/search/ESSearchDAO.java @@ -19,7 +19,6 @@ import com.linkedin.metadata.query.SearchResultMetadata; import com.linkedin.metadata.query.SortCriterion; import java.net.URISyntaxException; -import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -201,6 +200,7 @@ SearchRequest getFilteredSearchQuery(@Nullable Filter filters, @Nullable SortCri * @return a valid search request * @deprecated please use {@link #constructSearchQuery(String, Filter, SortCriterion, String, int, int)} instead */ + @Deprecated @Nonnull public SearchRequest constructSearchQuery(@Nonnull String input, @Nullable Filter filter, @Nullable SortCriterion sortCriterion, int from, int size) { @@ -315,11 +315,21 @@ List getDocuments(@Nonnull SearchResponse searchResponse) { */ @Nonnull DataMap buildDocumentsDataMap(@Nonnull Map objectMap) { - final DataMap dataMap = new DataMap(); for (Map.Entry entry : objectMap.entrySet()) { - if (entry.getValue() instanceof ArrayList) { - dataMap.put(entry.getKey(), new DataList((ArrayList) entry.getValue())); + if (entry.getValue() instanceof List) { + final List values = (List) entry.getValue(); + final DataList dataList = new DataList(); + + for (Object value : values) { + if (value instanceof String) { + dataList.add(value); + } else { + log.error("Expected all values to be Strings, but found `{}`", value.getClass().getSimpleName()); + } + } + + dataMap.put(entry.getKey(), dataList); } else if (entry.getValue() != null) { dataMap.put(entry.getKey(), entry.getValue()); } diff --git a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/utils/SearchUtils.java b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/utils/SearchUtils.java index 3c8ab563e..e146fe6db 100644 --- a/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/utils/SearchUtils.java +++ b/dao-impl/elasticsearch-dao/src/main/java/com/linkedin/metadata/dao/utils/SearchUtils.java @@ -94,7 +94,7 @@ public static QueryBuilder getQueryBuilderFromCriterion(@Nonnull Criterion crite } @Nonnull - public static String toEntityType(@Nonnull Class c) { + public static String toEntityType(@Nonnull Class c) { String result = c.getSimpleName().toLowerCase(); if (result.endsWith("entity")) { result = result.substring(0, result.length() - 6); @@ -103,11 +103,10 @@ public static String toEntityType(@Nonnull Class c) { } @Nonnull - public static String readResourceFile(@Nonnull Class clazz, @Nonnull String filePath) { + public static String readResourceFile(@Nonnull Class clazz, @Nonnull String filePath) { try (InputStream inputStream = clazz.getClassLoader().getResourceAsStream(filePath)) { return IOUtils.toString(inputStream); } catch (IOException e) { - log.error("Can't read file: " + filePath); throw new RuntimeException("Can't read file: " + filePath); } } diff --git a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/browse/BrowseDAOTest.java b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/browse/BrowseDAOTest.java index 387c94d50..5d13b5b62 100644 --- a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/browse/BrowseDAOTest.java +++ b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/browse/BrowseDAOTest.java @@ -4,6 +4,7 @@ import com.linkedin.testing.TestUtils; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import org.elasticsearch.action.search.SearchResponse; @@ -18,7 +19,7 @@ public class BrowseDAOTest { - private BaseBrowseConfig _browseConfig; + private BaseBrowseConfig _browseConfig; private RestHighLevelClient _mockClient; private ESBrowseDAO _browseDAO; @@ -68,31 +69,45 @@ public void testMatchingPaths() { } @Test - public void testGetBrowsePath() throws Exception { + public void testEmptyGetBrowsePaths() throws Exception { SearchResponse mockSearchResponse = mock(SearchResponse.class); SearchHits mockSearchHits = mock(SearchHits.class); - SearchHit mockSearchHit = mock(SearchHit.class); Urn dummyUrn = TestUtils.makeUrn(0); - Map mockSourceMap = mock(Map.class); // Test when there is no search hit for getBrowsePaths when(mockSearchHits.getHits()).thenReturn(new SearchHit[0]); when(mockSearchResponse.getHits()).thenReturn(mockSearchHits); when(_mockClient.search(any())).thenReturn(mockSearchResponse); assertEquals(_browseDAO.getBrowsePaths(dummyUrn).size(), 0); + } + + @Test + public void testGetBrowsePathMissingField() throws Exception { + SearchResponse mockSearchResponse = mock(SearchResponse.class); + SearchHits mockSearchHits = mock(SearchHits.class); + SearchHit mockSearchHit = mock(SearchHit.class); + Urn dummyUrn = TestUtils.makeUrn(0); + Map sourceMap = new HashMap<>(); // Test the case of single search hit & browsePaths field doesn't exist - when(mockSourceMap.containsKey(_browseConfig.getBrowsePathFieldName())).thenReturn(false); - when(mockSearchHit.getSourceAsMap()).thenReturn(mockSourceMap); + when(mockSearchHit.getSourceAsMap()).thenReturn(sourceMap); when(mockSearchHits.getHits()).thenReturn(new SearchHit[]{mockSearchHit}); when(mockSearchResponse.getHits()).thenReturn(mockSearchHits); when(_mockClient.search(any())).thenReturn(mockSearchResponse); assertEquals(_browseDAO.getBrowsePaths(dummyUrn).size(), 0); + } + + @Test + public void testGetBrowsePathFoundField() throws Exception { + SearchResponse mockSearchResponse = mock(SearchResponse.class); + SearchHits mockSearchHits = mock(SearchHits.class); + SearchHit mockSearchHit = mock(SearchHit.class); + Urn dummyUrn = TestUtils.makeUrn(0); + Map sourceMap = new HashMap<>(); + sourceMap.put(_browseConfig.getBrowsePathFieldName(), Collections.singletonList("foo")); // Test the case of single search hit & browsePaths field exists - when(mockSourceMap.containsKey(_browseConfig.getBrowsePathFieldName())).thenReturn(true); - when(mockSourceMap.get(_browseConfig.getBrowsePathFieldName())).thenReturn(Collections.singletonList("foo")); - when(mockSearchHit.getSourceAsMap()).thenReturn(mockSourceMap); + when(mockSearchHit.getSourceAsMap()).thenReturn(sourceMap); when(mockSearchHits.getHits()).thenReturn(new SearchHit[]{mockSearchHit}); when(mockSearchResponse.getHits()).thenReturn(mockSearchHits); when(_mockClient.search(any())).thenReturn(mockSearchResponse); diff --git a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/search/ESSearchDAOTest.java b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/search/ESSearchDAOTest.java index 312bc42e9..679a2e2da 100644 --- a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/search/ESSearchDAOTest.java +++ b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/search/ESSearchDAOTest.java @@ -53,7 +53,7 @@ private static String loadJsonFromResource(String resourceName) throws IOExcepti @BeforeMethod public void setup() throws Exception { _testSearchConfig = new TestSearchConfig(); - _searchDAO = new ESSearchDAO(null, EntityDocument.class, _testSearchConfig); + _searchDAO = new ESSearchDAO<>(null, EntityDocument.class, _testSearchConfig); _esAutoCompleteQuery = new ESAutoCompleteQueryForHighCardinalityFields(_testSearchConfig); } diff --git a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/utils/SearchUtilsTest.java b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/utils/SearchUtilsTest.java index 79f1788dc..ac265db3b 100644 --- a/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/utils/SearchUtilsTest.java +++ b/dao-impl/elasticsearch-dao/src/test/java/com/linkedin/metadata/dao/utils/SearchUtilsTest.java @@ -21,7 +21,7 @@ public void testGetRequestMap() { assertTrue(actual1.isEmpty()); // Filter with criteria with default condition - final Map requestParams = Collections.unmodifiableMap(new HashMap() { + final Map requestParams = Collections.unmodifiableMap(new HashMap() { { put("key1", "value1"); put("key2", "value2");