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
+ }
+}