From c2f395d94c8edf082ad6947566fe42125ffba992 Mon Sep 17 00:00:00 2001 From: Yingjian Wu Date: Wed, 6 Mar 2024 11:24:56 -0500 Subject: [PATCH] remove unused code + use insert if not exist for values_string instead of loading from memory + test --- .../server/usermetadata/LookupService.java | 18 ++-- .../netflix/metacat/MetacatSmokeSpec.groovy | 79 ++++++++++++++++ .../metadata/mysql/MySqlLookupService.java | 94 ++++--------------- .../metadata/mysql/MySqlTagService.java | 6 +- .../mysql/MySqlLookupServiceSpec.groovy | 79 ++++++++++++++++ 5 files changed, 187 insertions(+), 89 deletions(-) create mode 100644 metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/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 d1e31ac36..fcc79792a 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 @@ -31,12 +31,14 @@ public interface LookupService { /** * Returns the lookup for the given name. + * 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) { + default Lookup get(final String name, final boolean includeValues) { return null; } @@ -75,22 +77,20 @@ 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 setValues(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 * @return updated lookup */ @Nullable - default Lookup addValues(final String name, final Set values) { + default Lookup addValues(final String name, final Set values, boolean includeValues) { return null; } @@ -99,11 +99,7 @@ default Lookup addValues(final String name, final Set values) { * * @param name lookup name * @param value lookup value + * @param includeValues whether to return lookup value in the Lookup Object * @return updated lookup */ - @Nullable - default Lookup setValue(final String name, final String value) { - return null; - } - } diff --git a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeSpec.groovy b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeSpec.groovy index 7f961e58e..efd19489f 100644 --- a/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeSpec.groovy +++ b/metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeSpec.groovy @@ -1771,4 +1771,83 @@ class MetacatSmokeSpec extends Specification { 'embedded-hive-metastore/invalid/dm/vm', 'embedded-hive-metastore/invalid/dm/vm=1'] } + + def "Test Set tags for all scenarios"() { + setup: + // The setup already contains the following 7 tags + // [data-category:non-customer, test, test_tag, unused, do_not_drop, iceberg_migration_do_not_modify, do_not_rename] + def catalogName = "hive-metastore" + def databaseName = "default" + def catalog = QualifiedName.ofCatalog(catalogName) + def database = QualifiedName.ofDatabase(catalogName, databaseName) + + def table1Name = "table1" + def table1Tags = ['table1', 'mock'] as Set + + def table2Name = "table2" + def table2Tags = ['table2', 'mock'] as Set + + def table3Name = "table3" + def table3Tags = ['table3', 'bdow'] as Set + when: + // add 2 new tags on table1 + createTable(catalogName, databaseName, table1Name) + tagApi.setTableTags(catalogName, databaseName, table1Name, table1Tags) + then: + tagApi.getTags().size() == 9 + tagApi.list(['table1', 'mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 1 + tagApi.search('mock', null, null, null).size() == 1 + tagApi.search('table1', null, null, null).size() == 1 + metacatJson.convertValue(api.getTable(catalogName, databaseName, table1Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table1Tags + when: + // add an existing tag mock but a new tag on table 2 + createTable(catalogName, databaseName, table2Name) + tagApi.setTableTags(catalogName, databaseName, table2Name, table2Tags) + then: + tagApi.getTags().size() == 10 + tagApi.list(['table2'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 1 + tagApi.list(['mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.list(['table1', 'mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.search('mock', null, null, null).size() == 2 + tagApi.search('table1', null, null, null).size() == 1 + tagApi.search('table2', null, null, null).size() == 1 + metacatJson.convertValue(api.getTable(catalogName, databaseName, table2Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table2Tags + when: + // delete table1, for now even if tag table1 is only applied to table1, we will not remove the tag from our db + api.deleteTable(catalogName, databaseName, "table1") + then: + tagApi.getTags().size() == 10 + tagApi.list(['table1'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 1 + tagApi.list(['table2', 'mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.list(['mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.search('mock', null, null, null).size() == 2 + tagApi.search('table1', null, null, null).size() == 1 + tagApi.search('table2', null, null, null).size() == 1 + when: + // add tags on catalog database and table level through setTags api + // create 1 new catalogTag on catalog + // create 1 new databaseTag on database + // create 2 new tableTags on table3 + tagApi.setTags(TagCreateRequestDto.builder().name(catalog).tags(['mock', 'catalogTag'] as List).build()) + tagApi.setTags(TagCreateRequestDto.builder().name(database).tags(['mock', 'databaseTag'] as List).build()) + createTable(catalogName, databaseName, "table3") + def table3 = QualifiedName.ofTable(catalogName, databaseName, table3Name) + tagApi.setTags(TagCreateRequestDto.builder().name(table3).tags(table3Tags as List).build()) + then: + tagApi.getTags().size() == 14 + tagApi.list(['table2', 'mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.list(['table3', 'bdow'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 1 + tagApi.list(['mock'] as Set, null, null, null, null, QualifiedName.Type.TABLE).size() == 2 + tagApi.list(['mock'] as Set, null, null, null, null, QualifiedName.Type.DATABASE).size() == 1 + tagApi.list(['mock'] as Set, null, null, null, null, QualifiedName.Type.CATALOG).size() == 1 + tagApi.list(['mock'] as Set, null, null, null, null, null).size() == 4 + tagApi.search('mock', null, null, null).size() == 4 + tagApi.search('table3', null, null, null).size() == 1 + tagApi.search('bdow', null, null, null).size() == 1 + tagApi.search('table2', null, null, null).size() == 1 + metacatJson.convertValue(api.getTable(catalogName, databaseName, table3Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table3Tags + cleanup: + api.deleteTable(catalogName, databaseName, "table2") + api.deleteTable(catalogName, databaseName, "table3") + } } 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..3b0217f7c 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,7 +13,6 @@ 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; @@ -23,7 +22,6 @@ 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; @@ -32,6 +30,7 @@ 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; @@ -50,8 +49,10 @@ 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_DELETE_LOOKUP_VALUES = - "delete from lookup_values where lookup_id=? and values_string in (%s)"; + private static final String SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST = + "INSERT INTO lookup_values(lookup_id, values_string) VALUES (?,?) " + + "ON DUPLICATE KEY UPDATE lookup_id=lookup_id, values_string=values_string"; + 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 +80,7 @@ public MySqlLookupService(final Config config, final JdbcTemplate jdbcTemplate) */ @Override @Transactional(readOnly = true) - public Lookup get(final String name) { + public Lookup get(final String name, final boolean includeValues) { try { return jdbcTemplate.queryForObject( SQL_GET_LOOKUP, @@ -93,7 +94,7 @@ public Lookup get(final String name) { lookup.setLastUpdated(rs.getDate("lastUpdated")); lookup.setLastUpdatedBy(rs.getString("lastUpdatedBy")); lookup.setDateCreated(rs.getDate("dateCreated")); - lookup.setValues(getValues(rs.getLong("id"))); + lookup.setValues(includeValues ? getValues(rs.getLong("id")) : Collections.EMPTY_SET); return lookup; }); } catch (EmptyResultDataAccessException e) { @@ -162,61 +163,22 @@ public Set getValues(final String name) { } } - /** - * 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}) + 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}) .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) throws SQLException { - Lookup lookup = get(name); + private Lookup findOrCreateLookupByName(final String name, final boolean includeValues) throws SQLException { + Lookup lookup = get(name, includeValues); if (lookup == null) { final KeyHolder holder = new GeneratedKeyHolder(); jdbcTemplate.update(connection -> { @@ -244,20 +206,14 @@ private Lookup findOrCreateLookupByName(final String name) throws SQLException { * @return returns the lookup with the given name. */ @Override - public Lookup addValues(final String name, final Set values) { + public Lookup addValues(final String name, final Set values, final boolean includeValues) { try { - 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); + final Lookup lookup = findOrCreateLookupByName(name, includeValues); + if (!values.isEmpty()) { + insertLookupValuesIfNotExist(lookup.getId(), values); } - if (!inserts.isEmpty()) { - insertLookupValues(lookup.getId(), inserts); + if (includeValues) { + lookup.getValues().addAll(values); } return lookup; } catch (Exception e) { @@ -266,16 +222,4 @@ public Lookup addValues(final String name, final Set values) { 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 8310d11ec..06576daa4 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) { + private Lookup addTags(final Set tags, final boolean includeValues) { try { - return lookupService.addValues(LOOKUP_NAME_TAG, tags); + return lookupService.addValues(LOOKUP_NAME_TAG, tags, includeValues); } 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); + addTags(tags, false); 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 new file mode 100644 index 000000000..644948913 --- /dev/null +++ b/metacat-metadata-mysql/src/test/groovy/com/netflix/metacat/metadata/mysql/MySqlLookupServiceSpec.groovy @@ -0,0 +1,79 @@ +/* + * Copyright 2017 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.metadata.mysql + +import com.google.common.collect.Lists +import com.netflix.metacat.common.QualifiedName +import com.netflix.metacat.common.server.model.Lookup +import com.netflix.metacat.common.server.properties.Config +import com.netflix.metacat.testdata.provider.DataDtoProvider +import org.springframework.jdbc.core.JdbcTemplate +import spock.lang.Ignore +import spock.lang.Specification +import spock.lang.Unroll + +import java.time.Instant + +/** + * Tests for MysqlUserMetadataService. + * TODO: Need to move this to integration-test + * @author amajumdar + */ + +class MySqlLookupServiceSpec extends Specification { + MySqlLookupService mySqlLookupService + + Config config + JdbcTemplate jdbcTemplate + + def setup() { + config = Mock(Config) + jdbcTemplate = Mock(JdbcTemplate) + + mySqlLookupService = new MySqlLookupService(config, jdbcTemplate) + } + + @Unroll + def "test includeValues"() { + setup: + def lookupWithValues = new Lookup( + id: 1L, + name: 'Test1', + values: new HashSet(['tag1', 'tag2']), + dateCreated: new Date(), + lastUpdated: new Date() + ) + def lookupWithoutValues = new Lookup( + id: 2L, + name: 'Test2', + values: new HashSet([]), + dateCreated: new Date(), + lastUpdated: new Date() + ) + when: + jdbcTemplate.queryForObject(_, _, _, _) >> (includeValues ? lookupWithValues : lookupWithoutValues) + def lookUpResult = mySqlLookupService.addValues("tag", tagsToAdd, includeValues) + + then: + lookUpResult.values == expectedValues + where: + tagsToAdd | includeValues | expectedValues + ['tag1', 'tag2'] as Set |true | ['tag1', 'tag2'] as Set + ['tag1', 'tag2'] as Set |false | [] as Set + ['tag1', 'tag3'] as Set |true | ['tag1', 'tag2', 'tag3'] as Set + ['tag1', 'tag3'] as Set |false | [] as Set + ['tag3', 'tag4'] as Set |true | ['tag1', 'tag2', 'tag3', 'tag4'] as Set + ['tag3', 'tag4'] as Set |false | [] as Set + } +}