From b326a10d67ba64b979cb9a416a935db68650feb0 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 10:45:53 -0500 Subject: [PATCH 1/9] addSetValues --- .../server/usermetadata/LookupService.java | 16 ++-- metacat-functional-tests/build.gradle | 2 +- .../metacat-test-cluster/docker-compose.yml | 2 +- .../metacat/MySqlLookupServiceSpec.groovy | 82 +++++++++++++++++++ .../metadata/mysql/MySqlLookupService.java | 38 +++++++++ 5 files changed, 127 insertions(+), 13 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..4b771745f 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 @@ -72,15 +72,6 @@ default Set getValues(final Long lookupId) { return Collections.emptySet(); } - /** - * Saves the lookup value. - * - * @param name lookup name - * @param values multiple values - * @param includeValues whether to populate the values field in the Lookup Object - * @return updated lookup - */ - /** * Saves the lookup value. * @@ -98,8 +89,11 @@ default Lookup addValues(final String name, final Set values, boolean in * Saves the lookup value. * * @param name lookup name - * @param value lookup value - * @param includeValues whether to return lookup value in the Lookup Object + * @param values lookup value * @return updated lookup */ + @Nullable + default Lookup setValues(final String name, final Set values) { + 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..5a9d0676e --- /dev/null +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -0,0 +1,82 @@ +/* + * 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.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 MySqlLookupServiceUnitTest 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) + } + + def "test setValues iterative"() { + setup: + def values = valuesList as Set + def lookup = mySqlLookupService.setValues("mock", values) + + expect: + lookup.values.size() == expectedSize + lookup.values == mySqlLookupService.getValues("mock") + + 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") + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("mock2") + } +} + 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..4bfdd96f0 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. * @@ -221,4 +233,30 @@ 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 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); + log.error(message, e); + throw new UserMetadataServiceException(message, e); + } + } } From a540db5a603b88f1ce90990858accf1d7b1f55cc Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 15:02:13 -0500 Subject: [PATCH 2/9] add more test --- .../metacat/MySqlLookupServiceSpec.groovy | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) 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 index 5a9d0676e..b4ef4527d 100644 --- a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -16,6 +16,7 @@ */ 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 @@ -49,7 +50,18 @@ class MySqlLookupServiceUnitTest extends Specification{ mySqlLookupService = new MySqlLookupService(new DefaultConfigImpl(new MetacatProperties()), jdbcTemplate) } - def "test setValues iterative"() { + boolean areLookupsEqual(Lookup l1, Lookup l2) { + l1.id == l2.id && + l1.name == l2.name && + l1.type == l2.type && + l1.values == l2.values && + l1.dateCreated == l2.dateCreated && + l1.lastUpdated == l2.lastUpdated && + 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) @@ -57,6 +69,9 @@ class MySqlLookupServiceUnitTest extends Specification{ 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 @@ -75,8 +90,46 @@ class MySqlLookupServiceUnitTest extends Specification{ 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, false) + + expect: + lookup.values.size() == expectedSize + lookup.values == mySqlLookupService.getValues("mockAdd") + lookup.values == mySqlLookupService.getValues(lookup.id) + lookup.values.contains(mySqlLookupService.getValue("mockAdd")) + areLookupsEqual(lookup, mySqlLookupService.get("mockAdd", false)) + + where: + valuesList | expectedSize + ["1", "2", "3"] | 3 + ["1", "2", "3", "4"] | 4 + ["1", "2", "3", "3", "4"] | 4 + ["3", "4"] | 4 + ["6"] | 5 + ["1", "6"] | 6 + } + + 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 == ["1", "2", "3"] as Set + mock1LookUp.values == mySqlLookupService.getValues("addValues_mock1") + 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)) } } From 3d3bd785c74d60bc69d82e393ce377e5bfa8190f Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 15:13:47 -0500 Subject: [PATCH 3/9] add more test --- .../groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy | 2 -- 1 file changed, 2 deletions(-) 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 index b4ef4527d..119c9091d 100644 --- a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -55,8 +55,6 @@ class MySqlLookupServiceUnitTest extends Specification{ l1.name == l2.name && l1.type == l2.type && l1.values == l2.values && - l1.dateCreated == l2.dateCreated && - l1.lastUpdated == l2.lastUpdated && l1.createdBy == l2.createdBy && l1.lastUpdatedBy == l2.lastUpdatedBy } From 8b9b69ace397bd2ca7260ec9c48aa88954dbdee4 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 15:37:06 -0500 Subject: [PATCH 4/9] previous behavior --- .../server/usermetadata/LookupService.java | 19 +++- .../metacat/MySqlLookupServiceSpec.groovy | 35 ++++-- .../metadata/mysql/MySqlLookupService.java | 103 +++++++++++------- .../metadata/mysql/MySqlTagService.java | 6 +- 4 files changed, 103 insertions(+), 60 deletions(-) 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 4b771745f..dd7fb3637 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 @@ -17,6 +17,7 @@ */ package com.netflix.metacat.common.server.usermetadata; +import com.google.common.collect.Sets; import com.netflix.metacat.common.server.model.Lookup; import javax.annotation.Nullable; @@ -34,11 +35,10 @@ public interface LookupService { * If includeValues = true, we will set the Lookup values with the associated name, otherwise empty set * * @param name lookup name - * @param includeValues whether we should set the values or not in the Lookup Object * @return lookup */ @Nullable - default Lookup get(final String name, final boolean includeValues) { + default Lookup get(final String name) { return null; } @@ -77,11 +77,10 @@ default Set getValues(final Long lookupId) { * * @param name lookup name * @param values multiple values - * @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) { + default Lookup addValues(final String name, final Set values) { return null; } @@ -96,4 +95,16 @@ default Lookup addValues(final String name, final Set values, boolean in default Lookup setValues(final String name, final Set values) { return null; } + + /** + * Saves the lookup value. + * + * @param name lookup name + * @param value lookup value + * @return returns the lookup with the given name. + */ + + default Lookup setValue(final String name, final String value) { + return null; + } } 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 index 119c9091d..2df708d9b 100644 --- a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -25,7 +25,7 @@ import org.springframework.jdbc.datasource.DriverManagerDataSource import spock.lang.Shared import spock.lang.Specification -class MySqlLookupServiceUnitTest extends Specification{ +class MySqlLookupServiceSpec extends Specification{ private MySqlLookupService mySqlLookupService; private JdbcTemplate jdbcTemplate; @@ -69,7 +69,7 @@ class MySqlLookupServiceUnitTest extends Specification{ lookup.values == mySqlLookupService.getValues("mock") lookup.values == mySqlLookupService.getValues(lookup.id) lookup.values.contains(mySqlLookupService.getValue("mock")) - areLookupsEqual(lookup, mySqlLookupService.get("mock", true),) + areLookupsEqual(lookup, mySqlLookupService.get("mock")) where: valuesList | expectedSize @@ -81,30 +81,43 @@ class MySqlLookupServiceUnitTest extends Specification{ ["1", "6"] | 2 } - def "test setValues for different id"(){ + def "test setValue with different id"() { + when: + def mock1LookUp = mySqlLookupService.setValue("mock_setvalue_1", "1") + def mock2LookUp = mySqlLookupService.setValue("mock_setvalue_2", "2") + then: + mock1LookUp.values == ["1"] as Set + mock1LookUp.values == mySqlLookupService.getValues("mock_setvalue_1") + areLookupsEqual(mock1LookUp, mySqlLookupService.get("mock_setvalue_1")) + mock2LookUp.values == ["2"] as Set + mock2LookUp.values == mySqlLookupService.getValues("mock_setvalue_2") + areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock_setvalue_2")) + } + + 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)) + areLookupsEqual(mock1LookUp, mySqlLookupService.get("mock1")) mock2LookUp.values == ["4", "5", "6"] as Set mock2LookUp.values == mySqlLookupService.getValues("mock2") - areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock2", true)) + areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock2")) } def "test addValues iterative"() { setup: def values = valuesList as Set - def lookup = mySqlLookupService.addValues("mockAdd", values, false) + def lookup = mySqlLookupService.addValues("mockAdd", values) expect: lookup.values.size() == expectedSize lookup.values == mySqlLookupService.getValues("mockAdd") lookup.values == mySqlLookupService.getValues(lookup.id) lookup.values.contains(mySqlLookupService.getValue("mockAdd")) - areLookupsEqual(lookup, mySqlLookupService.get("mockAdd", false)) + areLookupsEqual(lookup, mySqlLookupService.get("mockAdd")) where: valuesList | expectedSize @@ -118,16 +131,16 @@ class MySqlLookupServiceUnitTest extends Specification{ 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) + def mock1LookUp = mySqlLookupService.addValues("addValues_mock1", ["1", "2", "3"] as Set) + def mock2LookUp = mySqlLookupService.addValues("addValues_mock2", ["4", "5", "6"] as Set) expect: mock1LookUp.values == ["1", "2", "3"] as Set mock1LookUp.values == mySqlLookupService.getValues("addValues_mock1") - areLookupsEqual(mock1LookUp, mySqlLookupService.get("addValues_mock1", false)) + areLookupsEqual(mock1LookUp, mySqlLookupService.get("addValues_mock1")) mock2LookUp.values == ["4", "5", "6"] as Set mock2LookUp.values == mySqlLookupService.getValues("addValues_mock2") - areLookupsEqual(mock2LookUp, mySqlLookupService.get("addValues_mock2", true)) + areLookupsEqual(mock2LookUp, mySqlLookupService.get("addValues_mock2")) } } 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 4bfdd96f0..ca84d53e3 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 @@ -32,7 +32,6 @@ import java.sql.SQLException; import java.sql.Statement; import java.sql.Types; -import java.util.Collections; import java.util.Set; import java.util.stream.Collectors; @@ -51,12 +50,8 @@ public class MySqlLookupService implements LookupService { + " values (?,0,?,?,?,now(),now())"; private static final String SQL_INSERT_LOOKUP_VALUES = "insert into lookup_values( lookup_id, values_string) values (?,?)"; - 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_DELETE_LOOKUP_VALUES = + "delete from lookup_values where lookup_id=? and values_string 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 = @@ -84,7 +79,7 @@ public MySqlLookupService(final Config config, final JdbcTemplate jdbcTemplate) */ @Override @Transactional(readOnly = true) - public Lookup get(final String name, final boolean includeValues) { + public Lookup get(final String name) { try { return jdbcTemplate.queryForObject( SQL_GET_LOOKUP, @@ -98,7 +93,7 @@ public Lookup get(final String name, final boolean includeValues) { lookup.setLastUpdated(rs.getDate("lastUpdated")); lookup.setLastUpdatedBy(rs.getString("lastUpdatedBy")); lookup.setDateCreated(rs.getDate("dateCreated")); - lookup.setValues(includeValues ? getValues(rs.getLong("id")) : Collections.EMPTY_SET); + lookup.setValues(getValues(rs.getLong("id"))); return lookup; }); } catch (EmptyResultDataAccessException e) { @@ -167,16 +162,49 @@ public Set getValues(final String name) { } } - private void insertLookupValuesIfNotExist(final Long id, final Set inserts) { - jdbcTemplate.batchUpdate(SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST, - inserts.stream().map(insert -> new Object[]{id, insert}) + /** + * 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 { + final Lookup lookup = findOrCreateLookupByName(name); + final Set inserts; + Set deletes = Sets.newHashSet(); + final Set lookupValues = lookup.getValues(); + if (lookupValues == null || lookupValues.isEmpty()) { + inserts = values; + } else { + inserts = Sets.difference(values, lookupValues).immutableCopy(); + deletes = Sets.difference(lookupValues, values).immutableCopy(); + } + lookup.setValues(values); + if (!inserts.isEmpty()) { + insertLookupValues(lookup.getId(), inserts); + } + if (!deletes.isEmpty()) { + deleteLookupValues(lookup.getId(), deletes); + } + 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); + } + } + + private void insertLookupValues(final Long id, final Set inserts) { + jdbcTemplate.batchUpdate(SQL_INSERT_LOOKUP_VALUES, inserts.stream().map(insert -> new Object[]{id, insert}) .collect(Collectors.toList()), new int[]{Types.BIGINT, Types.VARCHAR}); } - private void deleteLookupValuesIfNotIn(final Long id, final Set values) { + private void deleteLookupValues(final Long id, final Set deletes) { jdbcTemplate.update( - String.format(SQL_DELETE_LOOKUP_VALUES_IF_NOT_IN, - "'" + Joiner.on("','").skipNulls().join(values) + "'"), + String.format(SQL_DELETE_LOOKUP_VALUES, "'" + Joiner.on("','").skipNulls().join(deletes) + "'"), new SqlParameterValue(Types.BIGINT, id)); } @@ -184,12 +212,11 @@ private void deleteLookupValuesIfNotIn(final Long id, final Set values) * findOrCreateLookupByName. * * @param name name to find or create - * @param includeValues whether to include the values in the Lookup Object * @return Look up object * @throws SQLException sql exception */ - private Lookup findOrCreateLookupByName(final String name, final boolean includeValues) throws SQLException { - Lookup lookup = get(name, includeValues); + private Lookup findOrCreateLookupByName(final String name) throws SQLException { + Lookup lookup = get(name); if (lookup == null) { final KeyHolder holder = new GeneratedKeyHolder(); jdbcTemplate.update(connection -> { @@ -217,14 +244,20 @@ private Lookup findOrCreateLookupByName(final String name, final boolean include * @return returns the lookup with the given name. */ @Override - public Lookup addValues(final String name, final Set values, final boolean includeValues) { + public Lookup addValues(final String name, final Set values) { try { - final Lookup lookup = findOrCreateLookupByName(name, includeValues); - if (!values.isEmpty()) { - insertLookupValuesIfNotExist(lookup.getId(), values); + final Lookup lookup = findOrCreateLookupByName(name); + + final Set inserts; + final Set lookupValues = lookup.getValues(); + if (lookupValues == null || lookupValues.isEmpty()) { + inserts = values; + lookup.setValues(values); + } else { + inserts = Sets.difference(values, lookupValues); } - if (includeValues) { - lookup.getValues().addAll(values); + if (!inserts.isEmpty()) { + insertLookupValues(lookup.getId(), inserts); } return lookup; } catch (Exception e) { @@ -237,26 +270,12 @@ public Lookup addValues(final String name, final Set values, final boole /** * Saves the lookup value. * - * @param name lookup name - * @param values multiple values + * @param name lookup name + * @param value lookup value * @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); - log.error(message, e); - throw new UserMetadataServiceException(message, e); - } + public Lookup setValue(final String name, final String value) { + return setValues(name, Sets.newHashSet(value)); } } diff --git a/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlTagService.java b/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlTagService.java index 06576daa4..8310d11ec 100644 --- a/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlTagService.java +++ b/metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlTagService.java @@ -124,9 +124,9 @@ public MySqlTagService( this.userMetadataService = Preconditions.checkNotNull(userMetadataService, "userMetadataService is required"); } - private Lookup addTags(final Set tags, final boolean includeValues) { + private Lookup addTags(final Set tags) { try { - return lookupService.addValues(LOOKUP_NAME_TAG, tags, includeValues); + return lookupService.addValues(LOOKUP_NAME_TAG, tags); } catch (Exception e) { final String message = String.format("Failed adding the tags %s", tags); log.error(message, e); @@ -396,7 +396,7 @@ public List search( @Override public Set setTags(final QualifiedName name, final Set tags, final boolean updateUserMetadata) { - addTags(tags, false); + addTags(tags); try { final TagItem tagItem = findOrCreateTagItemByName(name.toString()); final Set inserts; From 88abbbbe5ad9d311b79d149846aadac6ba80606b Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 15:59:07 -0500 Subject: [PATCH 5/9] add more test --- .../com/netflix/metacat/metadata/mysql/MySqlLookupService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ca84d53e3..3842c02fb 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 @@ -252,13 +252,13 @@ public Lookup addValues(final String name, final Set values) { final Set lookupValues = lookup.getValues(); if (lookupValues == null || lookupValues.isEmpty()) { inserts = values; - lookup.setValues(values); } else { inserts = Sets.difference(values, lookupValues); } if (!inserts.isEmpty()) { insertLookupValues(lookup.getId(), inserts); } + lookup.getValues().addAll(values); return lookup; } catch (Exception e) { final String message = String.format("Failed to set the lookup values for name %s", name); From 6b4b637d81fbc9fd74308d6af3c3491ca6c8c623 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 16:20:12 -0500 Subject: [PATCH 6/9] fix test --- .../com/netflix/metacat/MySqlLookupServiceSpec.groovy | 2 +- .../netflix/metacat/metadata/mysql/MySqlLookupService.java | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) 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 index 2df708d9b..4b2646283 100644 --- a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -126,7 +126,7 @@ class MySqlLookupServiceSpec extends Specification{ ["1", "2", "3", "3", "4"] | 4 ["3", "4"] | 4 ["6"] | 5 - ["1", "6"] | 6 + ["1", "6"] | 5 } def "test addValues for different id"() { 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 3842c02fb..65827ca68 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 @@ -258,7 +258,12 @@ public Lookup addValues(final String name, final Set values) { if (!inserts.isEmpty()) { insertLookupValues(lookup.getId(), inserts); } - lookup.getValues().addAll(values); + + if (lookupValues == null || lookupValues.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); From 017be675dc524898dde1b1b188d09a4e9e49e492 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 16:38:17 -0500 Subject: [PATCH 7/9] add more test --- .../metacat/common/server/usermetadata/LookupService.java | 1 - 1 file changed, 1 deletion(-) 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 dd7fb3637..3c764bec7 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 @@ -17,7 +17,6 @@ */ package com.netflix.metacat.common.server.usermetadata; -import com.google.common.collect.Sets; import com.netflix.metacat.common.server.model.Lookup; import javax.annotation.Nullable; From 62c0211d943e8ab839f58b667d887b7450355485 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 16:39:54 -0500 Subject: [PATCH 8/9] fix style --- .../metacat/common/server/usermetadata/LookupService.java | 1 - 1 file changed, 1 deletion(-) 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 3c764bec7..34e227d79 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 @@ -16,7 +16,6 @@ * */ package com.netflix.metacat.common.server.usermetadata; - import com.netflix.metacat.common.server.model.Lookup; import javax.annotation.Nullable; From 6cab683e55318bf6a892044b4691aa70cdb93f40 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Fri, 8 Mar 2024 16:58:03 -0500 Subject: [PATCH 9/9] fix style --- .../metacat/metadata/mysql/MySqlLookupServiceSpec.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/MySqlLookupServiceSpec.groovy b/metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/MySqlLookupServiceSpec.groovy index c82f537fb..6fe34774a 100644 --- a/metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/MySqlLookupServiceSpec.groovy +++ b/metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/MySqlLookupServiceSpec.groovy @@ -30,7 +30,7 @@ import java.time.Instant * TODO: Need to move this to integration-test * @author amajumdar */ - +@Ignore class MySqlLookupServiceSpec extends Specification { MySqlLookupService mySqlLookupService