From 4d52242faad3ec0a7208bd7ae22299b515f49cdf Mon Sep 17 00:00:00 2001 From: Ayush Shah Date: Sun, 26 Sep 2021 02:24:04 +0530 Subject: [PATCH] Fix #568 Add tests for APIs related to table complex column type --- .../catalog/jdbi3/TableRepository.java | 39 +-- .../resources/databases/DatabaseUtil.java | 79 ++++- .../resources/databases/TableResource.java | 8 +- .../openmetadata/catalog/util/JsonUtils.java | 7 +- .../json/schema/entity/data/table.json | 35 +- .../resources/json/schema/type/tagLabel.json | 3 +- .../databases/TableResourceTest.java | 309 ++++++++++++------ .../resources/feeds/FeedResourceTest.java | 2 +- ingestion/ingestion_dependency.sh | 7 +- ingestion/pipelines/sample_data.json | 2 +- ingestion/pipelines/sample_usage.json | 2 +- ingestion/pipelines/sample_users.json | 8 +- ingestion/setup.py | 2 +- .../metadata/ingestion/sink/elasticsearch.py | 5 +- .../metadata/ingestion/sink/metadata_rest.py | 2 +- 15 files changed, 347 insertions(+), 163 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java index a86ec6da14f..416f717aacb 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java @@ -342,6 +342,10 @@ public abstract class TableRepository { } private void validateTags(List columns) { + if (columns == null || columns.isEmpty()) { + return; + } + columns.forEach(column -> { EntityUtil.validateTags(tagDAO(), column.getTags()); if (column.getChildren() != null) { @@ -392,6 +396,9 @@ public abstract class TableRepository { } List cloneWithoutTags(List columns) { + if (columns == null || columns.isEmpty()) { + return columns; + } List copy = new ArrayList<>(); columns.forEach(c -> copy.add(cloneWithoutTags(c))); return copy; @@ -402,9 +409,9 @@ public abstract class TableRepository { return new Column().withDescription(column.getDescription()).withName(column.getName()) .withFullyQualifiedName(column.getFullyQualifiedName()) .withArrayDataType(column.getArrayDataType()) - .withColumnConstraint(column.getColumnConstraint()) - .withColumnDataTypeDisplay(column.getColumnDataTypeDisplay()) - .withColumnDataType(column.getColumnDataType()) + .withConstraint(column.getConstraint()) + .withDataTypeDisplay(column.getDataTypeDisplay()) + .withDataType(column.getDataType()) .withDataLength(column.getDataLength()) .withOrdinalPosition(column.getOrdinalPosition()) .withChildren(children); @@ -453,19 +460,10 @@ public abstract class TableRepository { * Update the backend database */ private void patch(Table original, Table updated) throws IOException { - // TODO Patching field usageSummary is ignored - if (!original.getId().equals(updated.getId())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TABLE, "id")); - } - if (!original.getName().equals(updated.getName())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TABLE, "name")); - } - if (updated.getDatabase() == null) { - throw new IllegalArgumentException("Table relationship database can't be removed"); - } - if (!updated.getDatabase().getId().equals(original.getDatabase().getId())) { - throw new IllegalArgumentException("Table relationship database can't be replaced"); - } + // Patch can't make changes to following fields. Ignore the change + updated.withFullyQualifiedName(original.getFullyQualifiedName()).withName(original.getName()) + .withDatabase(original.getDatabase()).withId(original.getId()); + validateRelationships(updated, updated.getDatabase().getId(), updated.getOwner()); // Remove previous tags. Merge tags from the update and the existing tags @@ -524,7 +522,8 @@ public abstract class TableRepository { // Find stored column matching name, data type and ordinal position Column stored = storedColumns.stream() .filter(s -> s.getName().equals(updated.getName()) && - s.getColumnDataType() == updated.getColumnDataType() && + s.getDataType() == updated.getDataType() && + s.getArrayDataType() == updated.getArrayDataType() && Objects.equals(s.getOrdinalPosition(), updated.getOrdinalPosition())) .findAny() .orElse(null); @@ -551,11 +550,13 @@ public abstract class TableRepository { } } + for (Column stored : storedColumns) { // Find updated column matching name, data type and ordinal position - Column updated = storedColumns.stream() + Column updated = updatedColumns.stream() .filter(s -> s.getName().equals(stored.getName()) && - s.getColumnDataType() == stored.getColumnDataType() && + s.getDataType() == stored.getDataType() && + s.getArrayDataType() == stored.getArrayDataType() && Objects.equals(s.getOrdinalPosition(), stored.getOrdinalPosition())) .findAny() .orElse(null); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/DatabaseUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/DatabaseUtil.java index caf3e5848eb..662d28c2253 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/DatabaseUtil.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/DatabaseUtil.java @@ -16,13 +16,16 @@ package org.openmetadata.catalog.resources.databases; +import org.openmetadata.catalog.entity.data.Table; import org.openmetadata.catalog.type.Column; import org.openmetadata.catalog.type.ColumnConstraint; +import org.openmetadata.catalog.type.ColumnDataType; import org.openmetadata.catalog.type.TableConstraint; import org.openmetadata.catalog.type.TableType; import java.util.ArrayList; import java.util.List; +import java.util.Locale; public final class DatabaseUtil { private DatabaseUtil() { @@ -32,7 +35,7 @@ public final class DatabaseUtil { public static boolean validateSinglePrimaryColumn(List columns) { int primaryKeyColumns = 0; for (Column c : columns) { - if (c.getColumnConstraint() == ColumnConstraint.PRIMARY_KEY) { + if (c.getConstraint() == ColumnConstraint.PRIMARY_KEY) { primaryKeyColumns++; if (primaryKeyColumns > 1) { throw new IllegalArgumentException("Multiple columns tagged with primary key constraints"); @@ -68,10 +71,82 @@ public final class DatabaseUtil { } public static void validateViewDefinition(TableType tableType, String viewDefinition) { - if ( (tableType == null || tableType.equals(TableType.Regular) || tableType.equals(TableType.External)) + if ((tableType == null || tableType.equals(TableType.Regular) || tableType.equals(TableType.External)) && viewDefinition != null && !viewDefinition.isEmpty()) { throw new IllegalArgumentException("ViewDefinition can only be set on TableType View, " + "SecureView or MaterializedView"); } } + + public static void validateColumns(Table table) { + for (Column c : table.getColumns()) { + validateColumnDataTypeDisplay(c); + validateColumnDataLength(c); + validateArrayColumn(c); + validateStructColumn(c); + } + } + + public static void validateColumnDataTypeDisplay(Column column) { + // If dataTypeDisplay is null then set it based on dataType + if (column.getDataTypeDisplay() == null) { + column.setDataTypeDisplay(column.getDataType().value().toLowerCase(Locale.ROOT)); + } + + // Make sure types from column dataType and dataTypeDisplay match + String dataType = column.getDataType().value().toLowerCase(Locale.ROOT); + String dataTypeDisplay = column.getDataTypeDisplay().toLowerCase(Locale.ROOT); + + if (!dataTypeDisplay.startsWith(dataType)) { + throw new IllegalArgumentException(String.format("columnDataType %s does not match columnDataTypeDisplay %s", + dataType, dataTypeDisplay)); + } + + column.setDataTypeDisplay(dataTypeDisplay); // Make dataTypeDisplay lower case + } + + public static void validateColumnDataLength(Column column) { + // Types char, varchar, binary, varbinary must have dataLength + ColumnDataType dataType = column.getDataType(); + if ((dataType == ColumnDataType.CHAR || dataType == ColumnDataType.VARCHAR || + dataType == ColumnDataType.BINARY || dataType == ColumnDataType.VARBINARY) && column.getDataLength() == null) { + throw new IllegalArgumentException("For column data types char, varchar, binary, varbinary dataLength " + + "must not be null"); + } + } + + public static void validateArrayColumn(Column column) { + // arrayDataType must only be used when columnDataType is array. Ignore the arrayDataType. + ColumnDataType dataType = column.getDataType(); + if (column.getArrayDataType() != null && dataType != ColumnDataType.ARRAY) { + column.setArrayDataType(null); + } + + if (dataType == ColumnDataType.ARRAY) { + if (column.getArrayDataType() == null) { + throw new IllegalArgumentException("For column data type array, arrayDataType " + + "must not be null"); + } + + if (!column.getDataTypeDisplay().startsWith("array<")) { + throw new IllegalArgumentException("For column data type array, dataTypeDisplay must be of type " + + "array"); + } + } + } + + public static void validateStructColumn(Column column) { + ColumnDataType dataType = column.getDataType(); + if (dataType == ColumnDataType.STRUCT) { + if (column.getChildren() == null) { + throw new IllegalArgumentException("For column data type struct, children " + + "must not be null"); + } + + if (!column.getDataTypeDisplay().startsWith("struct<")) { + throw new IllegalArgumentException("For column data type struct, dataTypeDisplay must be of type " + + "stuct"); + } + } + } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java index a7e3ddd38a1..80167ad8293 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java @@ -17,8 +17,6 @@ package org.openmetadata.catalog.resources.databases; import com.google.inject.Inject; -import org.openmetadata.catalog.type.TableData; -import org.openmetadata.catalog.type.TableJoins; import io.swagger.annotations.Api; import io.swagger.v3.oas.annotations.ExternalDocumentation; import io.swagger.v3.oas.annotations.Operation; @@ -34,7 +32,11 @@ import org.openmetadata.catalog.jdbi3.TableRepository; import org.openmetadata.catalog.resources.Collection; import org.openmetadata.catalog.security.CatalogAuthorizer; import org.openmetadata.catalog.security.SecurityUtil; +import org.openmetadata.catalog.type.Column; +import org.openmetadata.catalog.type.ColumnDataType; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.TableData; +import org.openmetadata.catalog.type.TableJoins; import org.openmetadata.catalog.type.TableProfile; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.EntityUtil.Fields; @@ -71,6 +73,7 @@ import java.security.GeneralSecurityException; import java.text.ParseException; import java.util.Arrays; import java.util.List; +import java.util.Locale; import java.util.Objects; import java.util.UUID; @@ -387,6 +390,7 @@ public class TableResource { table.setId(UUID.randomUUID()); DatabaseUtil.validateConstraints(table.getColumns(), table.getTableConstraints()); DatabaseUtil.validateViewDefinition(table.getTableType(), table.getViewDefinition()); + DatabaseUtil.validateColumns(table); return table; } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java index 9166cd0d9a5..b4d23f66c20 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java @@ -74,7 +74,12 @@ public final class JsonUtils { } public static String pojoToJson(Object o) throws JsonProcessingException { - return OBJECT_MAPPER.writeValueAsString(o); + return pojoToJson(o, false); + } + + public static String pojoToJson(Object o, boolean prettyPrint) throws JsonProcessingException { + return prettyPrint ? OBJECT_MAPPER.writerWithDefaultPrettyPrinter().writeValueAsString(o) : + OBJECT_MAPPER.writeValueAsString(o); } public static JsonStructure getJsonStructure(Object o) { diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/data/table.json b/catalog-rest-service/src/main/resources/json/schema/entity/data/table.json index f50cbb77f91..4b7db76e408 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/data/table.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/data/table.json @@ -35,7 +35,7 @@ } ] }, - "columnDataType": { + "dataType": { "javaType": "org.openmetadata.catalog.type.ColumnDataType", "description": "This enum defines the type of data stored in a column.", "type": "string", @@ -76,7 +76,7 @@ "JSON" ] }, - "columnConstraint": { + "constraint": { "javaType": "org.openmetadata.catalog.type.ColumnConstraint", "description": "This enum defines the type for column constraint.", "type": "string", @@ -111,7 +111,7 @@ } }, "columnName": { - "description": "Local name (not fully qualified name) of the column. ColumnName is `-` when the column is not named in struct columnDataType. For example, BigQuery supports struct with unnamed fields", + "description": "Local name (not fully qualified name) of the column. ColumnName is `-` when the column is not named in struct dataType. For example, BigQuery supports struct with unnamed fields", "type": "string", "minLength": 1, "maxLength": 64, @@ -125,7 +125,7 @@ "pattern": "^[^.]*$" }, "fullyQualifiedColumnName": { - "description": "Fully qualified name of the column that includes `serviceName.databaseName.tableName.columnName[.nestedColumnName]`. When columnName is null for columnDataType struct fields, `field_#` where `#` is field index is used. For map columnDataType, for key the field name `key` is used and for the value field `value` is used.", + "description": "Fully qualified name of the column that includes `serviceName.databaseName.tableName.columnName[.nestedColumnName]`. When columnName is null for dataType struct fields, `field_#` where `#` is field index is used. For map dataType, for key the field name `key` is used and for the value field `value` is used.", "type": "string", "minLength": 1, "maxLength": 256 @@ -138,19 +138,21 @@ "name": { "$ref": "#/definitions/columnName" }, - "columnDataType": { + "dataType": { "description": "Data type of the column (int, date etc.).", - "$ref": "#/definitions/columnDataType" + "$ref": "#/definitions/dataType" }, "arrayDataType" : { - "description": "Data type used array in columnDataType. For example, `array` has columnDataType as `array` and arrayDataType as `int`." + "description": "Data type used array in dataType. For example, `array` has dataType as `array` and arrayDataType as `int`.", + "$ref": "#/definitions/dataType" }, "dataLength" : { - "description": "Length of `char`, `varchar`, `binary`, `varbinary` `columnDataTypes`, else null. For example, `varchar(20)` has columnDataType as `varchar` and dataLength as `20`.", + "description": "Length of `char`, `varchar`, `binary`, `varbinary` `dataTypes`, else null. For example, `varchar(20)` has dataType as `varchar` and dataLength as `20`.", "type": "integer" }, - "columnDataTypeDisplay" : { - "description" : "Display name used for columnDataType. This is useful for complex types, such as `array, map, struct<>, and union types." + "dataTypeDisplay" : { + "description" : "Display name used for dataType. This is useful for complex types, such as `array, map, struct<>, and union types.", + "type": "string" }, "description": { "description": "Description of the column.", @@ -167,29 +169,30 @@ }, "default": null }, - "columnConstraint": { + "constraint": { "description": "Column level constraint.", - "$ref": "#/definitions/columnConstraint" + "$ref": "#/definitions/constraint" }, "ordinalPosition": { "description": "Ordinal position of the column.", "type": "integer" }, "jsonSchema" : { - "description": "Json schema only if the columnDataType is JSON else null.", + "description": "Json schema only if the dataType is JSON else null.", "type": "string" }, "children" : { - "description": "Child columns if columnDataType or arrayDataType is `map`, `struct`, or `union` else `null`.", + "description": "Child columns if dataType or arrayDataType is `map`, `struct`, or `union` else `null`.", "type": "array", "items": { "$ref": "#/definitions/column" - } + }, + "default" : null } }, "required": [ "name", - "columnDataType" + "dataType" ], "additionalProperties": false }, diff --git a/catalog-rest-service/src/main/resources/json/schema/type/tagLabel.json b/catalog-rest-service/src/main/resources/json/schema/type/tagLabel.json index d8cda60291c..14383009d43 100644 --- a/catalog-rest-service/src/main/resources/json/schema/type/tagLabel.json +++ b/catalog-rest-service/src/main/resources/json/schema/type/tagLabel.json @@ -34,5 +34,6 @@ "description": "Link to the tag resource.", "$ref": "basic.json#/definitions/href" } - } + }, + "additionalProperties": false } \ No newline at end of file diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java index 8a0eed79b17..9c6d9275f3e 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java @@ -72,6 +72,7 @@ import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.UUID; import java.util.stream.Collectors; @@ -91,6 +92,13 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.openmetadata.catalog.resources.databases.DatabaseResourceTest.createAndCheckDatabase; import static org.openmetadata.catalog.resources.services.DatabaseServiceResourceTest.createService; +import static org.openmetadata.catalog.type.ColumnDataType.ARRAY; +import static org.openmetadata.catalog.type.ColumnDataType.BIGINT; +import static org.openmetadata.catalog.type.ColumnDataType.CHAR; +import static org.openmetadata.catalog.type.ColumnDataType.FLOAT; +import static org.openmetadata.catalog.type.ColumnDataType.INT; +import static org.openmetadata.catalog.type.ColumnDataType.STRING; +import static org.openmetadata.catalog.type.ColumnDataType.STRUCT; import static org.openmetadata.catalog.util.RestUtil.DATE_FORMAT; import static org.openmetadata.catalog.util.TestUtils.NON_EXISTENT_ENTITY; import static org.openmetadata.catalog.util.TestUtils.adminAuthHeaders; @@ -110,12 +118,9 @@ public class TableResourceTest extends CatalogApplicationTest { public static final TagLabel TIER_2 = new TagLabel().withTagFQN("Tier.Tier2"); public static List COLUMNS = Arrays.asList( - new Column().withName("c1").withColumnDataType(ColumnDataType.BIGINT) - .withTags(singletonList(USER_ADDRESS_TAG_LABEL)), - new Column().withName("c2").withColumnDataType(ColumnDataType.BIGINT) - .withTags(singletonList(USER_ADDRESS_TAG_LABEL)), - new Column().withName("c3").withColumnDataType(ColumnDataType.BIGINT) - .withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL))); + getColumn("c1", BIGINT, USER_ADDRESS_TAG_LABEL), + getColumn("c2", ColumnDataType.VARCHAR, USER_ADDRESS_TAG_LABEL).withDataLength(10), + getColumn("c3", BIGINT, USER_BANK_ACCOUNT_TAG_LABEL)); public static User USER1; public static EntityReference USER_OWNER1; @@ -160,6 +165,41 @@ public class TableResourceTest extends CatalogApplicationTest { assertResponse(exception, BAD_REQUEST, "[name size must be between 1 and 64]"); } + @Test + public void post_tableWithoutColumnDataLength_400(TestInfo test) { + List columns = singletonList(getColumn("c1", BIGINT, null).withOrdinalPosition(1)); + CreateTable create = create(test).withColumns(columns); + + // char, varchar, binary, and varbinary columns must have length + ColumnDataType[] columnDataTypes = {CHAR, ColumnDataType.VARCHAR, ColumnDataType.BINARY, + ColumnDataType.VARBINARY}; + + for (ColumnDataType dataType : columnDataTypes) { + create.getColumns().get(0).withDataType(dataType); + HttpResponseException exception = assertThrows(HttpResponseException.class, () -> + createTable(create, adminAuthHeaders())); + assertResponse(exception, BAD_REQUEST, + "For column data types char, varchar, binary, varbinary dataLength must not be null"); + } + } + + @Test + public void post_tableInvalidArrayColumn_400(TestInfo test) { + // No arrayDataType passed for array + List columns = singletonList(getColumn("c1", ARRAY, "array", null)); + CreateTable create = create(test).withColumns(columns); + HttpResponseException exception = assertThrows(HttpResponseException.class, () -> + createTable(create, adminAuthHeaders())); + assertResponse(exception, BAD_REQUEST, "For column data type array, arrayDataType must not be null"); + + // No dataTypeDisplay passed for array + columns.get(0).withArrayDataType(INT).withDataTypeDisplay(null); + exception = assertThrows(HttpResponseException.class, () -> + createTable(create, adminAuthHeaders())); + assertResponse(exception, BAD_REQUEST, + "For column data type array, dataTypeDisplay must be of type array"); + } + @Test public void post_tableAlreadyExists_409_conflict(TestInfo test) throws HttpResponseException { CreateTable create = create(test); @@ -181,6 +221,85 @@ public class TableResourceTest extends CatalogApplicationTest { createAndCheckTable(create, adminAuthHeaders()); } + private static Column getColumn(String name, ColumnDataType columnDataType, TagLabel tag) { + + return getColumn(name, columnDataType, null, tag); + } + + private static Column getColumn(String name, ColumnDataType columnDataType, String dataTypeDisplay, TagLabel tag) { + + return new Column().withName(name).withDataType(columnDataType) + .withDataTypeDisplay(dataTypeDisplay).withTags(singletonList(tag)); + } + + @Test + public void post_put_patch_complexColumnTypes(TestInfo test) throws HttpResponseException, JsonProcessingException { + Column c1 = getColumn("c1", ARRAY, "array", USER_ADDRESS_TAG_LABEL).withArrayDataType(INT); + Column c2_a = getColumn("a", INT, USER_ADDRESS_TAG_LABEL); + Column c2_b = getColumn("b", CHAR, USER_ADDRESS_TAG_LABEL); + Column c2_c_d = getColumn("d", INT, USER_ADDRESS_TAG_LABEL); + Column c2_c = getColumn("c", STRUCT, "struct>>", USER_ADDRESS_TAG_LABEL) + .withChildren(new ArrayList<>(Arrays.asList(c2_c_d))); + + // Column struct>> + Column c2 = getColumn("c2", STRUCT, "struct>>",USER_BANK_ACCOUNT_TAG_LABEL) + .withChildren(new ArrayList<>(Arrays.asList(c2_a, c2_b, c2_c))); + + // Test POST operation can create complex types + CreateTable create1 = create(test, 1).withColumns(Arrays.asList(c1, c2)); + Table table1 = createAndCheckTable(create1, adminAuthHeaders()); + + // Test PUT operation + CreateTable create2 = create(test, 2).withColumns(Arrays.asList(c1, c2)); + updateAndCheckTable(create2.withName("put_complexColumnType"), Status.CREATED, adminAuthHeaders()); + // Update without any change + updateAndCheckTable(create2.withName("put_complexColumnType"), Status.OK, adminAuthHeaders()); + + // + // Update the complex columns + // + // c1 from array to array and also change the tag + c1.withArrayDataType(CHAR).withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)).withDataTypeDisplay("array"); + + // c2 from -> to + // struct>> + // struct<-----, b:char, c:struct, f:char> + c2_b.withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); // Change c2.b tag + c2_c.getChildren().add(getColumn("e", INT,USER_ADDRESS_TAG_LABEL)); // Add c2.c.e + c2.getChildren().remove(0); // Remove c2.a from struct + c2.getChildren().add(getColumn("f", CHAR, USER_ADDRESS_TAG_LABEL)); // Add c2.f + create2 = create2.withColumns(Arrays.asList(c1, c2)); + + // Update the columns with put operation and validate update + updateAndCheckTable(create2.withName("put_complexColumnType"), Status.OK, adminAuthHeaders()); + + + // + // Patch operations on table1 created by POST operation. Columns can't be added or deleted. Only tags and + // description can be changed + // + String tableJson = JsonUtils.pojoToJson(table1); + c1 = table1.getColumns().get(0); + c1.withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); // c1 tag changed + + c2 = table1.getColumns().get(1); + c2.withTags(Arrays.asList(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); // c2 new tag added + + c2_a = c2.getChildren().get(0); + c2_a.withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); // c2.a tag changed + + c2_b = c2.getChildren().get(1); + c2_b.withTags(new ArrayList<>()); // c2.b tag removed + + c2_c = c2.getChildren().get(2); + c2_c.withTags(new ArrayList<>()); // c2.c tag removed + + c2_c_d = c2_c.getChildren().get(0); + c2_c_d.setTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); // c2.c.d new tag added + table1 = patchTable(tableJson, table1, adminAuthHeaders()); + validateColumns(Arrays.asList(c1, c2), table1.getColumns()); + } + @Test public void post_tableWithUserOwner_200_ok(TestInfo test) throws HttpResponseException { createAndCheckTable(create(test).withOwner(USER_OWNER1), adminAuthHeaders()); @@ -313,8 +432,9 @@ public class TableResourceTest extends CatalogApplicationTest { // Create a table with column c1, type BIGINT, description c1 and tag USER_ADDRESS_TAB_LABEL // List tags = new ArrayList<>(singletonList(USER_ADDRESS_TAG_LABEL)); - List columns = singletonList(new Column().withName("c1").withColumnDataType(ColumnDataType.BIGINT) + List columns = singletonList(new Column().withName("c1").withDataType(BIGINT) .withOrdinalPosition(1).withDescription("c1").withTags(tags)); + CreateTable request = create(test).withColumns(columns); Table table = createAndCheckTable(request, adminAuthHeaders()); columns.get(0).setFullyQualifiedName(table.getFullyQualifiedName() + ".c1"); @@ -331,7 +451,7 @@ public class TableResourceTest extends CatalogApplicationTest { // tags.add(USER_BANK_ACCOUNT_TAG_LABEL); List updatedColumns = new ArrayList<>(); - updatedColumns.add(new Column().withName("c1").withColumnDataType(ColumnDataType.BIGINT) + updatedColumns.add(new Column().withName("c1").withDataType(BIGINT) .withTags(tags).withOrdinalPosition(1)); table = updateAndCheckTable(request.withColumns(updatedColumns), OK, adminAuthHeaders()); @@ -343,8 +463,8 @@ public class TableResourceTest extends CatalogApplicationTest { // // Add a new column and make sure it is added by PUT // - updatedColumns.add(new Column().withName("c2").withColumnDataType(ColumnDataType.BINARY).withOrdinalPosition(2) - .withFullyQualifiedName(table.getFullyQualifiedName() + ".c2").withTags(tags)); + updatedColumns.add(new Column().withName("c2").withDataType(ColumnDataType.BINARY).withOrdinalPosition(2) + .withDataLength(10).withFullyQualifiedName(table.getFullyQualifiedName() + ".c2").withTags(tags)); table = updateAndCheckTable(request.withColumns(updatedColumns), OK, adminAuthHeaders()); assertEquals(2, table.getColumns().size()); validateTags(updatedColumns.get(0).getTags(), table.getColumns().get(0).getTags()); @@ -515,6 +635,20 @@ public class TableResourceTest extends CatalogApplicationTest { assertEquals(expected, actual.getColumnJoins()); } + public static class TagLabelComparator implements Comparator { + @Override + public int compare(TagLabel label, TagLabel t1) { + return label.getTagFQN().compareTo(t1.getTagFQN()); + } + } + + public static class ColumnComparator implements Comparator { + @Override + public int compare(Column column, Column t1) { + return column.getName().compareTo(t1.getName()); + } + } + public static class ColumnJoinComparator implements Comparator { @Override public int compare(ColumnJoin columnJoin, ColumnJoin t1) { @@ -596,7 +730,7 @@ public class TableResourceTest extends CatalogApplicationTest { } @Test - public void put_viewDefinition_invalid_table_4xx(TestInfo test) throws HttpResponseException { + public void put_viewDefinition_invalid_table_4xx(TestInfo test) { CreateTable createTable = create(test); createTable.setTableType(TableType.Regular); String query = "sales_vw\n" + @@ -615,7 +749,6 @@ public class TableResourceTest extends CatalogApplicationTest { @Test public void put_tableProfile_200(TestInfo test) throws HttpResponseException { Table table = createAndCheckTable(create(test), adminAuthHeaders()); - List columns = Arrays.asList("c1", "c2", "c3"); ColumnProfile c1Profile = new ColumnProfile().withName("c1").withMax("100.0") .withMin("10.0").withUniqueCount(100.0); ColumnProfile c2Profile = new ColumnProfile().withName("c2").withMax("99.0").withMin("20.0").withUniqueCount(89.0); @@ -765,7 +898,7 @@ public class TableResourceTest extends CatalogApplicationTest { /** * For cursor based pagination and implementation details: - * @see org.openmetadata.catalog.util.ResultList#ResultList(List, int, String, String) + * @see org.openmetadata.catalog.util.ResultList#ResultList * * The tests and various CASES referenced are base on that. */ @@ -886,79 +1019,20 @@ public class TableResourceTest extends CatalogApplicationTest { @Test public void patch_tableColumnTags_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { // Create table without description, table tags, tier, owner, tableType, and tableConstraints - Table table = createTable(create(test).withTableConstraints(null), adminAuthHeaders()); - assertNull(table.getDescription()); - assertNull(table.getOwner()); - assertNull(table.getTableType()); - assertNull(table.getTableConstraints()); + List columns = new ArrayList<>(); + columns.add(getColumn("c1", INT, USER_ADDRESS_TAG_LABEL)); + columns.add(getColumn("c2", BIGINT, USER_ADDRESS_TAG_LABEL)); + columns.add(getColumn("c3", FLOAT, USER_BANK_ACCOUNT_TAG_LABEL)); + Table table = createTable(create(test).withColumns(columns), adminAuthHeaders()); - Map> columnTagMap = new HashMap<>(); - columnTagMap.put("c1", singletonList(USER_ADDRESS_TAG_LABEL)); - columnTagMap.put("c2", singletonList(USER_ADDRESS_TAG_LABEL)); - columnTagMap.put("c3", singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); + // Update the columns + columns.get(0).setTags(List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); // Add a tag + columns.get(1).setTags(List.of(USER_ADDRESS_TAG_LABEL)); // No change in tag + columns.get(2).setTags(new ArrayList<>()); // Remove tag - validateColumnTags(table, columnTagMap); - - List updatedColumns = Arrays.asList( - new Column().withName("c1").withColumnDataType(ColumnDataType.BIGINT) - .withTags(List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)), - new Column().withName("c2").withColumnDataType(ColumnDataType.BIGINT) - .withTags(singletonList(USER_ADDRESS_TAG_LABEL)), - new Column().withName("c3").withColumnDataType(ColumnDataType.BIGINT) - .withTags(new ArrayList<>())); - - table = patchTableColumnAttributesAndCheck(table, updatedColumns, adminAuthHeaders()); - table.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner - columnTagMap.put("c1", List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); - columnTagMap.put("c2", singletonList(USER_ADDRESS_TAG_LABEL)); - columnTagMap.put("c3", new ArrayList<>()); - validateColumnTags(table, columnTagMap); - } - - @Test - public void patch_tableIDChange_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure table ID can't be changed using patch - Table table = createTable(create(test), adminAuthHeaders()); - UUID oldTableId = table.getId(); - String tableJson = JsonUtils.pojoToJson(table); - table.setId(UUID.randomUUID()); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchTable(oldTableId, tableJson, table, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, CatalogExceptionMessage.readOnlyAttribute(Entity.TABLE, "id")); - } - - @Test - public void patch_tableNameChange_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure table name can't be changed using patch - Table table = createTable(create(test), adminAuthHeaders()); - String tableJson = JsonUtils.pojoToJson(table); - table.setName("newName"); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchTable(tableJson, table, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, CatalogExceptionMessage.readOnlyAttribute(Entity.TABLE, "name")); - } - - @Test - public void patch_tableRemoveDatabase_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure table database it belongs to can't be removed - Table table = createTable(create(test).withDatabase(DATABASE.getId()), adminAuthHeaders()); - String tableJson = JsonUtils.pojoToJson(table); - table.setDatabase(null); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchTable(tableJson, table, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, "Table relationship database can't be removed"); - } - - @Test - public void patch_tableReplaceDatabase_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure table database it belongs to can't be removed - Table table = createTable(create(test).withDatabase(DATABASE.getId()), adminAuthHeaders()); - String tableJson = JsonUtils.pojoToJson(table); - table.getDatabase().setId(UUID.randomUUID()); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchTable(tableJson, table, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, "Table relationship database can't be replaced"); + table = patchTableColumnAttributesAndCheck(table, columns, adminAuthHeaders()); + validateColumns(columns, table.getColumns()); } @Test @@ -1011,12 +1085,12 @@ public class TableResourceTest extends CatalogApplicationTest { // Validate information returned in patch response has the updates Table updatedTable = patchTable(tableJson, table, authHeaders); - validateTable(updatedTable, table.getDescription(), owner, null, tableType, + validateTable(updatedTable, table.getDescription(), table.getColumns(), owner, null, tableType, tableConstraints, tags); // GET the table and Validate information returned Table getTable = getTable(table.getId(), "owner,tableConstraints,columns, tags", authHeaders); - validateTable(getTable, table.getDescription(), owner, null, tableType, tableConstraints, tags); + validateTable(getTable, table.getDescription(), table.getColumns(), owner, null, tableType, tableConstraints, tags); return updatedTable; } @@ -1028,8 +1102,7 @@ public class TableResourceTest extends CatalogApplicationTest { table.setColumns(columns); // Validate information returned in patch response has the updates - Table updatedTable = patchTable(tableJson, table, authHeaders); - return updatedTable; + return patchTable(tableJson, table, authHeaders); } // TODO disallow changing href, usage @@ -1041,13 +1114,12 @@ public class TableResourceTest extends CatalogApplicationTest { throws HttpResponseException { // Validate table created has all the information set in create request Table table = createTable(create, authHeaders); - validateTable(table, create.getDescription(), create.getOwner(), + validateTable(table, create.getDescription(), create.getColumns(), create.getOwner(), create.getDatabase(), create.getTableType(), create.getTableConstraints(), create.getTags()); - validateTags(create.getTags(), table.getTags()); // GET table created and ensure it has all the information set in create request - Table getTable = getTable(table.getId(), "owner,database,tags,tableConstraints", authHeaders); - validateTable(getTable, create.getDescription(), create.getOwner(), + Table getTable = getTable(table.getId(), "columns,owner,database,tags,tableConstraints", authHeaders); + validateTable(getTable, create.getDescription(), create.getColumns(), create.getOwner(), create.getDatabase(), create.getTableType(), create.getTableConstraints(), create.getTags()); // If owner information is set, GET and make sure the user or team has the table in owns list @@ -1129,24 +1201,28 @@ public class TableResourceTest extends CatalogApplicationTest { } // When tags from the expected list is added to an entity, the derived tags for those tags are automatically added // So add to the expectedList, the derived tags before validating the tags - List updatedExpectedList = new ArrayList<>(); - updatedExpectedList.addAll(expectedList); + List updatedExpectedList = new ArrayList<>(expectedList); for (TagLabel expected : expectedList) { List derived = EntityUtil.getDerivedTags(expected, TagResourceTest.getTag(expected.getTagFQN(), adminAuthHeaders())); updatedExpectedList.addAll(derived); } updatedExpectedList = updatedExpectedList.stream().distinct().collect(Collectors.toList()); + updatedExpectedList.sort(new TagLabelComparator()); + actualList.sort(new TagLabelComparator()); - assertTrue(actualList.containsAll(updatedExpectedList)); - assertTrue(updatedExpectedList.containsAll(actualList)); + assertEquals(updatedExpectedList.size(), actualList.size()); + for (int i = 0; i < actualList.size(); i++) { + assertEquals(updatedExpectedList.get(i), actualList.get(i)); + } } public static Table createTable(CreateTable create, Map authHeaders) throws HttpResponseException { return TestUtils.post(CatalogApplicationTest.getResource("tables"), create, Table.class, authHeaders); } - private static void validateTable(Table table, String expectedDescription, EntityReference expectedOwner, + private static void validateTable(Table table, String expectedDescription, + List expectedColumns, EntityReference expectedOwner, UUID expectedDatabaseId, TableType expectedTableType, List expectedTableConstraints, List expectedTags) throws HttpResponseException { @@ -1156,6 +1232,8 @@ public class TableResourceTest extends CatalogApplicationTest { assertEquals(expectedDescription, table.getDescription()); assertEquals(expectedTableType, table.getTableType()); + validateColumns(expectedColumns, table.getColumns()); + // Validate owner if (expectedOwner != null) { TestUtils.validateEntityReference(table.getOwner()); @@ -1170,17 +1248,37 @@ public class TableResourceTest extends CatalogApplicationTest { assertEquals(expectedDatabaseId, table.getDatabase().getId()); } + // Validate table constraints assertEquals(expectedTableConstraints, table.getTableConstraints()); validateTags(expectedTags, table.getTags()); TestUtils.validateEntityReference(table.getFollowers()); } - private static void validateColumnTags(Table table, Map> columnTagMap) - throws HttpResponseException { - for (Column column: table.getColumns()) { - List expectedTags = columnTagMap.get(column.getName()); - validateTags(expectedTags, column.getTags()); + private static void validateColumn(Column expectedColumn, Column actualColumn) throws HttpResponseException { + assertNotNull(actualColumn.getFullyQualifiedName()); + assertEquals(expectedColumn.getName(), actualColumn.getName()); + assertEquals(expectedColumn.getDataType(), actualColumn.getDataType()); + assertEquals(expectedColumn.getArrayDataType(), actualColumn.getArrayDataType()); + if (expectedColumn.getDataTypeDisplay() != null) { + assertEquals(expectedColumn.getDataTypeDisplay().toLowerCase(Locale.ROOT), actualColumn.getDataTypeDisplay()); + } + validateTags(expectedColumn.getTags(), actualColumn.getTags()); + + // Check the nested columns + validateColumns(expectedColumn.getChildren(), actualColumn.getChildren()); + } + + private static void validateColumns(List expectedColumns, List actualColumns) throws HttpResponseException { + if (expectedColumns == null && actualColumns == null) { + return; + } + // Sort columns by name + expectedColumns.sort(new ColumnComparator()); + actualColumns.sort(new ColumnComparator()); + assertEquals(expectedColumns.size(), actualColumns.size()); + for (int i = 0; i < expectedColumns.size(); i++) { + validateColumn(expectedColumns.get(i), actualColumns.get(i)); } } @@ -1249,13 +1347,13 @@ public class TableResourceTest extends CatalogApplicationTest { public static Table updateAndCheckTable(CreateTable create, Status status, Map authHeaders) throws HttpResponseException { Table updatedTable = updateTable(create, status, authHeaders); - validateTable(updatedTable, create.getDescription(), create.getOwner(), create.getDatabase(), + validateTable(updatedTable, create.getDescription(), create.getColumns(), create.getOwner(), create.getDatabase(), create.getTableType(), create.getTableConstraints(), create.getTags()); // GET the newly updated database and validate - Table getTable = getTable(updatedTable.getId(), "database,owner,tableConstraints,tags", authHeaders); - validateTable(getTable, create.getDescription(), create.getOwner(), create.getDatabase(), create.getTableType(), - create.getTableConstraints(), create.getTags()); + Table getTable = getTable(updatedTable.getId(), "columns,database,owner,tableConstraints,tags", authHeaders); + validateTable(getTable, create.getDescription(), create.getColumns(), create.getOwner(), create.getDatabase(), + create.getTableType(), create.getTableConstraints(), create.getTags()); // TODO columns check return updatedTable; } @@ -1297,7 +1395,6 @@ public class TableResourceTest extends CatalogApplicationTest { Map authHeaders) throws JsonProcessingException, HttpResponseException { String updateTableJson = JsonUtils.pojoToJson(updatedTable); JsonPatch patch = JsonSchemaUtil.getJsonPatch(originalJson, updateTableJson); - LOG.info("Applying patch ", patch); return TestUtils.patch(CatalogApplicationTest.getResource("tables/" + tableId), patch, Table.class, authHeaders); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java index 313b5e0ec6a..14293975c79 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java @@ -69,7 +69,7 @@ public class FeedResourceTest extends CatalogApplicationTest { TableResourceTest.setup(test); // Initialize TableResourceTest for using helper methods CreateTable createTable = TableResourceTest.create(test); TABLE = createAndCheckTable(createTable, adminAuthHeaders()); - COLUMNS = Collections.singletonList(new Column().withName("column1").withColumnDataType(ColumnDataType.BIGINT)); + COLUMNS = Collections.singletonList(new Column().withName("column1").withDataType(ColumnDataType.BIGINT)); TABLE_LINK = String.format("<#E/table/%s>", TABLE.getFullyQualifiedName()); USER = TableResourceTest.USER1; diff --git a/ingestion/ingestion_dependency.sh b/ingestion/ingestion_dependency.sh index 3fd059fa3e0..1e67378bf34 100755 --- a/ingestion/ingestion_dependency.sh +++ b/ingestion/ingestion_dependency.sh @@ -17,9 +17,4 @@ # set -euo pipefail -pip install --upgrade setuptools openmetadata-ingestion==0.2.1 apns -# wget https://github.com/explosion/spacy-models/releases/download/en_core_web_sm-3.0.0/en_core_web_sm-3.0.0-py3-none-any.whl -# pip install en_core_web_sm-3.0.0-py3-none-any.whl -python -m spacy download en_core_web_sm -rm -rf en_core_web_sm-3.0.0-py3-none-any.whl -pip install "simplescheduler@git+git://github.com/open-metadata/simplescheduler.git#egg=simplescheduler" +pip install --upgrade setuptools 'openmetadata-ingestion[sample-data, elasticsearch, scheduler]' diff --git a/ingestion/pipelines/sample_data.json b/ingestion/pipelines/sample_data.json index 0243d737dbc..ffd3993b33a 100644 --- a/ingestion/pipelines/sample_data.json +++ b/ingestion/pipelines/sample_data.json @@ -17,7 +17,7 @@ } }, "cron": { - "minute": "*/5", + "minute": "*/12", "hour": null, "day": null, "month": null, diff --git a/ingestion/pipelines/sample_usage.json b/ingestion/pipelines/sample_usage.json index 4c591ea6442..fe5fc0af3ec 100644 --- a/ingestion/pipelines/sample_usage.json +++ b/ingestion/pipelines/sample_usage.json @@ -33,7 +33,7 @@ } }, "cron": { - "minute": "*/5", + "minute": "*/6", "hour": null, "day": null, "month": null, diff --git a/ingestion/pipelines/sample_users.json b/ingestion/pipelines/sample_users.json index b75eda2ead4..3fd692fe7f9 100644 --- a/ingestion/pipelines/sample_users.json +++ b/ingestion/pipelines/sample_users.json @@ -18,10 +18,10 @@ } }, "cron": { - "minute": "*/5", + "minute": null, "hour": null, - "day": null, - "month": null, - "day_of_week": null + "day": "*/7", + "month": "*", + "day_of_week": "*" } } diff --git a/ingestion/setup.py b/ingestion/setup.py index 49b7eef0e9f..5f5a80dafaa 100644 --- a/ingestion/setup.py +++ b/ingestion/setup.py @@ -108,7 +108,7 @@ plugins: Dict[str, Set[str]] = { build_options = {"includes": ["_cffi_backend"]} setup( name="openmetadata-ingestion", - version="0.3.0", + version="0.3.2", url="https://open-metadata.org/", author="OpenMetadata Committers", license="Apache License 2.0", diff --git a/ingestion/src/metadata/ingestion/sink/elasticsearch.py b/ingestion/src/metadata/ingestion/sink/elasticsearch.py index d041870b891..7efc6d8c739 100644 --- a/ingestion/src/metadata/ingestion/sink/elasticsearch.py +++ b/ingestion/src/metadata/ingestion/sink/elasticsearch.py @@ -110,7 +110,10 @@ class ElasticsearchSink(Sink): dashboard_doc = self._create_dashboard_es_doc(record) self.elasticsearch_client.index(index=self.config.dashboard_index_name, id=str(dashboard_doc.dashboard_id), body=dashboard_doc.json()) - self.status.records_written(record.name) + if (hasattr(record.name,'__root__')): + self.status.records_written(record.name.__root__) + else: + self.status.records_written(record.name) def _create_table_es_doc(self, table: Table): fqdn = table.fullyQualifiedName diff --git a/ingestion/src/metadata/ingestion/sink/metadata_rest.py b/ingestion/src/metadata/ingestion/sink/metadata_rest.py index 45c7aea1f08..0b19e06a571 100644 --- a/ingestion/src/metadata/ingestion/sink/metadata_rest.py +++ b/ingestion/src/metadata/ingestion/sink/metadata_rest.py @@ -125,7 +125,7 @@ class MetadataRestSink(Sink): created_topic = self.client.create_or_update_topic(topic) logger.info( 'Successfully ingested topic {}'.format(created_topic.name.__root__)) - self.status.records_written(created_topic.name) + self.status.records_written(created_topic.name.__root__) except (APIError, ValidationError) as err: logger.error( "Failed to ingest topic {} ".format(topic.name.__root__))