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..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; @@ -34,11 +33,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,20 +75,22 @@ 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 * @return updated lookup */ + @Nullable + default Lookup addValues(final String name, final Set values) { + 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; } @@ -99,7 +99,10 @@ default Lookup addValues(final String name, final Set values, boolean in * * @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. */ + + 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..4b2646283 --- /dev/null +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MySqlLookupServiceSpec.groovy @@ -0,0 +1,146 @@ +/* + * 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")) + + 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 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")) + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("mock2") + areLookupsEqual(mock2LookUp, mySqlLookupService.get("mock2")) + } + + def "test addValues iterative"() { + setup: + def values = valuesList as Set + 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")) + + 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"] | 5 + } + + def "test addValues for different id"() { + setup: + 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")) + mock2LookUp.values == ["4", "5", "6"] as Set + mock2LookUp.values == mySqlLookupService.getValues("addValues_mock2") + 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 0f32974aa..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 @@ -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; @@ -30,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; @@ -49,9 +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 = + "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 = @@ -79,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, @@ -93,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) { @@ -162,22 +162,61 @@ 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 deleteLookupValues(final Long id, final Set deletes) { + jdbcTemplate.update( + String.format(SQL_DELETE_LOOKUP_VALUES, "'" + Joiner.on("','").skipNulls().join(deletes) + "'"), + new SqlParameterValue(Types.BIGINT, id)); + } + /** * 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 -> { @@ -205,13 +244,24 @@ 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; + } else { + inserts = Sets.difference(values, lookupValues); } - if (includeValues) { + if (!inserts.isEmpty()) { + insertLookupValues(lookup.getId(), inserts); + } + + if (lookupValues == null || lookupValues.isEmpty()) { + lookup.setValues(values); + } else { lookup.getValues().addAll(values); } return lookup; @@ -221,4 +271,16 @@ 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)); + } } 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; 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