From 0845991fe5e4f6b53a428b275db2b1073453cdb8 Mon Sep 17 00:00:00 2001 From: stevie9868 <151791653+stevie9868@users.noreply.github.com> Date: Mon, 11 Mar 2024 09:45:02 -0400 Subject: [PATCH] Add back SetValues() as it is used internally (#579) * addSetValues * fix test * fix test --------- Co-authored-by: Yingjian Wu --- .../server/usermetadata/LookupService.java | 23 +-- metacat-functional-tests/build.gradle | 2 +- .../metacat-test-cluster/docker-compose.yml | 2 +- .../metacat/MySqlLookupServiceSpec.groovy | 152 ++++++++++++++++++ .../metadata/mysql/MySqlLookupService.java | 56 ++++++- 5 files changed, 223 insertions(+), 12 deletions(-) create mode 100644 metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy diff --git a/metacat-common-server/src/main/java/com/netflix/metacat/common/server/usermetadata/LookupService.java b/metacat-common-server/src/main/java/com/netflix/metacat/common/server/usermetadata/LookupService.java index fcc79792a..8ac8192ec 100644 --- a/metacat-common-server/src/main/java/com/netflix/metacat/common/server/usermetadata/LookupService.java +++ b/metacat-common-server/src/main/java/com/netflix/metacat/common/server/usermetadata/LookupService.java @@ -77,29 +77,34 @@ default Set getValues(final Long lookupId) { * * @param name lookup name * @param values multiple values - * @param includeValues whether to populate the values field in the Lookup Object + * @param includeValues whether to include values in the final Lookup Object * @return updated lookup */ + @Nullable + default Lookup addValues(final String name, final Set values, boolean includeValues) { + return null; + } /** * Saves the lookup value. * - * @param name lookup name - * @param values multiple values - * @param includeValues whether to include values in the final Lookup Object + * @param name lookup name + * @param values lookup value * @return updated lookup */ @Nullable - default Lookup addValues(final String name, final Set values, boolean includeValues) { + default Lookup setValues(final String name, final Set values) { return null; } /** - * Saves the lookup value. - * + * Save single lookup Value. * @param name lookup name * @param value lookup value - * @param includeValues whether to return lookup value in the Lookup Object - * @return updated lookup + * @return returns the lookup with the given name. */ + @Nullable + default Lookup setValue(final String name, final String value) { + return null; + } } diff --git a/metacat-functional-tests/build.gradle b/metacat-functional-tests/build.gradle index 3d64cc971..ab9f9693c 100644 --- a/metacat-functional-tests/build.gradle +++ b/metacat-functional-tests/build.gradle @@ -42,6 +42,7 @@ configurations { dependencies { + testImplementation project(path: ':metacat-metadata-mysql') warproject(project(path: ':metacat-war', configuration: 'archives')) /******************************* @@ -64,7 +65,6 @@ dependencies { testImplementation(project(":metacat-common-server")) testImplementation(project(":metacat-connector-hive")) testImplementation(project(":metacat-testdata-provider")) - functionalTestImplementation(project(":metacat-client")) functionalTestImplementation("org.apache.hadoop:hadoop-core") functionalTestImplementation("org.apache.hive:hive-exec:${hive_version}") { diff --git a/metacat-functional-tests/metacat-test-cluster/docker-compose.yml b/metacat-functional-tests/metacat-test-cluster/docker-compose.yml index 7579553eb..4e422c91f 100644 --- a/metacat-functional-tests/metacat-test-cluster/docker-compose.yml +++ b/metacat-functional-tests/metacat-test-cluster/docker-compose.yml @@ -89,7 +89,7 @@ services: - MYSQL_PASSWORD=metacat_user_password - MYSQL_DATABASE=metacat ports: - - '3306' + - '3306:3306' labels: - "com.netflix.metacat.oss.test" postgresql: diff --git a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy new file mode 100644 index 000000000..e456cea10 --- /dev/null +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -0,0 +1,152 @@ +/* + * Copyright 2016 Netflix, Inc. + * + * Licensed 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 com.netflix.metacat + +import com.netflix.metacat.common.server.model.Lookup +import com.netflix.metacat.common.server.properties.DefaultConfigImpl +import com.netflix.metacat.common.server.properties.MetacatProperties +import com.netflix.metacat.metadata.mysql.MySqlLookupService +import org.springframework.jdbc.core.JdbcTemplate +import org.springframework.jdbc.datasource.DriverManagerDataSource +import spock.lang.Shared +import spock.lang.Specification + +class MySqlLookupServiceSpec extends Specification{ + private MySqlLookupService mySqlLookupService; + private JdbcTemplate jdbcTemplate; + + @Shared + MySqlLookupService mySqlLookupService + + @Shared + JdbcTemplate jdbcTemplate + + def setupSpec() { + String jdbcUrl = "jdbc:mysql://localhost:3306/metacat" + String username = "metacat_user" + String password = "metacat_user_password" + + DriverManagerDataSource dataSource = new DriverManagerDataSource() + dataSource.setDriverClassName("com.mysql.cj.jdbc.Driver") + dataSource.setUrl(jdbcUrl) + dataSource.setUsername(username) + dataSource.setPassword(password) + + jdbcTemplate = new JdbcTemplate(dataSource) + mySqlLookupService = new MySqlLookupService(new DefaultConfigImpl(new MetacatProperties()), jdbcTemplate) + } + + boolean areLookupsEqual(Lookup l1, Lookup l2) { + l1.id == l2.id && + l1.name == l2.name && + l1.type == l2.type && + l1.values == l2.values && + l1.createdBy == l2.createdBy && + l1.lastUpdatedBy == l2.lastUpdatedBy + } + + def "test setValues with getValue/getValues iterative"() { + setup: + def values = valuesList as Set + def lookup = mySqlLookupService.setValues("mock", values) + + expect: + lookup.values.size() == expectedSize + lookup.values == mySqlLookupService.getValues("mock") + lookup.values == mySqlLookupService.getValues(lookup.id) + lookup.values.contains(mySqlLookupService.getValue("mock")) + areLookupsEqual(lookup, mySqlLookupService.get("mock", true)) + + where: + valuesList | expectedSize + ["1", "2", "3"] | 3 + ["1", "2", "3", "4"] | 4 + ["1", "2", "3", "3", "4"] | 4 + ["3", "4"] | 2 + ["6"] | 1 + ["1", "6"] | 2 + } + + def "test setValues for different id"(){ + when: + def mock1LookUp = mySqlLookupService.setValues("mock1", ["1", "2", "3"] as Set) + def mock2LookUp = mySqlLookupService.setValues("mock2", ["4", "5", "6"] as Set) + then: + mock1LookUp.values == ["1", "2", "3"] as Set + mock1LookUp.values == mySqlLookupService.getValues("mock1") + areLookupsEqual(mock1LookUp, mySqlLookupService.get("mock1", true)) + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("mock2") + areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock2", true)) + } + + def "test addValues iterative"() { + setup: + def values = valuesList as Set + def lookup = mySqlLookupService.addValues("mockAdd", values, includeValues) + + expect: + if (includeValues) { + lookup.values == mySqlLookupService.getValues(lookup.id) + lookup.values.contains(mySqlLookupService.getValue("mockAdd")) + lookup.values == mySqlLookupService.getValues("mockAdd") + } + lookup.values.size() == expectedSize + areLookupsEqual(lookup, mySqlLookupService.get("mockAdd", includeValues)) + where: + valuesList | expectedSize | includeValues + ["1", "2", "3"] | 3 | true + ["1", "2", "3", "4"] | 4 | true + ["1", "2", "3", "3", "4"] | 4 | true + ["3", "4"] | 4 | true + ["6"] | 5 | true + ["1", "6"] | 5 | true + ["1", "2", "3"] | 0 | false + ["1", "2", "3", "4"] | 0 | false + ["1", "2", "3", "3", "4"] | 0 | false + ["3", "4"] | 0 | false + ["6"] | 0 | false + ["1", "6"] | 0 | false + } + + def "test addValues for different id"() { + setup: + def mock1LookUp = mySqlLookupService.addValues("addValues_mock1", ["1", "2", "3"] as Set, false) + def mock2LookUp = mySqlLookupService.addValues("addValues_mock2", ["4", "5", "6"] as Set, true) + + expect: + mock1LookUp.values.isEmpty() + areLookupsEqual(mock1LookUp, mySqlLookupService.get("addValues_mock1", false)) + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("addValues_mock2") + areLookupsEqual(mock2LookUp, mySqlLookupService.get("addValues_mock2", true)) + } + + def "test setValue for different id"(){ + when: + def mock1LookUp = mySqlLookupService.setValues("mock1", ["1", "2", "3"] as Set) + def mock2LookUp = mySqlLookupService.setValues("mock2", ["4", "5", "6"] as Set) + then: + mock1LookUp.values == ["1", "2", "3"] as Set + mock1LookUp.values == mySqlLookupService.getValues("mock1") + areLookupsEqual(mock1LookUp, mySqlLookupService.get("mock1", true)) + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("mock2") + areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock2", true)) + } +} + diff --git a/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlLookupService.java b/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlLookupService.java index 0f32974aa..d0ad67f48 100644 --- a/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlLookupService.java +++ b/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlLookupService.java @@ -13,6 +13,7 @@ package com.netflix.metacat.metadata.mysql; +import com.google.common.base.Joiner; import com.google.common.collect.Sets; import com.netflix.metacat.common.server.model.Lookup; import com.netflix.metacat.common.server.properties.Config; @@ -22,6 +23,7 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.jdbc.core.SqlParameterValue; import org.springframework.jdbc.support.GeneratedKeyHolder; import org.springframework.jdbc.support.KeyHolder; import org.springframework.transaction.annotation.Transactional; @@ -52,6 +54,9 @@ public class MySqlLookupService implements LookupService { private static final String SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST = "INSERT IGNORE INTO lookup_values (lookup_id, values_string) VALUES (?, ?)"; + private static final String SQL_DELETE_LOOKUP_VALUES_IF_NOT_IN = + "delete from lookup_values where lookup_id=? and values_string not in (%s)"; + private static final String SQL_GET_LOOKUP_VALUES = "select values_string value from lookup_values where lookup_id=?"; private static final String SQL_GET_LOOKUP_VALUES_BY_NAME = @@ -168,6 +173,13 @@ private void insertLookupValuesIfNotExist(final Long id, final Set inser .collect(Collectors.toList()), new int[]{Types.BIGINT, Types.VARCHAR}); } + private void deleteLookupValuesIfNotIn(final Long id, final Set values) { + jdbcTemplate.update( + String.format(SQL_DELETE_LOOKUP_VALUES_IF_NOT_IN, + "'" + Joiner.on("','").skipNulls().join(values) + "'"), + new SqlParameterValue(Types.BIGINT, id)); + } + /** * findOrCreateLookupByName. * @@ -193,6 +205,7 @@ private Lookup findOrCreateLookupByName(final String name, final boolean include lookup = new Lookup(); lookup.setName(name); lookup.setId(lookupId); + lookup.setValues(Collections.emptySet()); } return lookup; } @@ -212,8 +225,38 @@ public Lookup addValues(final String name, final Set values, final boole insertLookupValuesIfNotExist(lookup.getId(), values); } if (includeValues) { - lookup.getValues().addAll(values); + if (lookup.getValues() == null || lookup.getValues().isEmpty()) { + lookup.setValues(values); + } else { + lookup.getValues().addAll(values); + } + } + return lookup; + } catch (Exception e) { + final String message = String.format("Failed to set the lookup values for name %s", name); + log.error(message, e); + throw new UserMetadataServiceException(message, e); + } + } + + /** + * Saves the lookup value. + * + * @param name lookup name + * @param values multiple values + * @return returns the lookup with the given name. + */ + @Override + public Lookup setValues(final String name, final Set values) { + try { + // For set values, no need to include values from querying rds + // since the values will be the same as the input values + final Lookup lookup = findOrCreateLookupByName(name, false); + if (!values.isEmpty()) { + insertLookupValuesIfNotExist(lookup.getId(), values); + deleteLookupValuesIfNotIn(lookup.getId(), values); } + lookup.setValues(values); return lookup; } catch (Exception e) { final String message = String.format("Failed to set the lookup values for name %s", name); @@ -221,4 +264,15 @@ public Lookup addValues(final String name, final Set values, final boole throw new UserMetadataServiceException(message, e); } } + /** + * Saves the lookup value. + * + * @param name lookup name + * @param value lookup value + * @return returns the lookup with the given name. + */ + @Override + public Lookup setValue(final String name, final String value) { + return setValues(name, Sets.newHashSet(value)); + } }