Fix #2989: Add delete apis for tests (#3020)

open-metadata · Mar 1, 2022 · c65cdbb · c65cdbb
1 parent 282c1f6
commit c65cdbb
Show file tree

Hide file tree

Showing 10 changed files with 372 additions and 132 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
@@ -50,6 +50,7 @@
 import org.openmetadata.catalog.entity.data.Table;
 import org.openmetadata.catalog.entity.services.DatabaseService;
 import org.openmetadata.catalog.exception.CatalogExceptionMessage;
+import org.openmetadata.catalog.exception.EntityNotFoundException;
 import org.openmetadata.catalog.resources.databases.TableResource;
 import org.openmetadata.catalog.tests.ColumnTest;
 import org.openmetadata.catalog.tests.TableTest;
@@ -260,27 +261,23 @@ public Table addTableTest(UUID tableId, TableTest tableTest) throws IOException,
     List<TableTest> storedTableTests = getTableTests(table);
     // we will override any test case name passed by user/client with tableName + testType
     // our assumption is there is only one instance of a test type as of now.
-    tableTest.setName(table.getName() + "." + tableTest.getTableTestCase().getTestType().toString());
+    tableTest.setName(table.getName() + "." + tableTest.getTestCase().getTableTestType().toString());
     Map<String, TableTest> storedMapTableTests = new HashMap<>();
     if (storedTableTests != null) {
       for (TableTest t : storedTableTests) {
         storedMapTableTests.put(t.getName(), t);
       }
     }
-    // new test add UUID
-    if (!storedMapTableTests.containsKey(tableTest.getName())) {
-      tableTest.setId(UUID.randomUUID());
-    }
-
-    // process test result
-    if (storedMapTableTests.containsKey(tableTest.getName())
-        && tableTest.getResults() != null
-        && !tableTest.getResults().isEmpty()) {
+    // existing test, use the previous UUID
+    if (storedMapTableTests.containsKey(tableTest.getName())) {
       TableTest prevTableTest = storedMapTableTests.get(tableTest.getName());
-      List<TestCaseResult> prevTestCaseResults = prevTableTest.getResults();
-      List<TestCaseResult> newTestCaseResults = tableTest.getResults();
-      newTestCaseResults.addAll(prevTestCaseResults);
-      tableTest.setResults(newTestCaseResults);
+      tableTest.setId(prevTableTest.getId());
+      // process test result
+      if (tableTest.getResults() != null && !tableTest.getResults().isEmpty()) {
+        List<TestCaseResult> prevTestCaseResults = prevTableTest.getResults();
+        prevTestCaseResults.addAll(tableTest.getResults());
+        tableTest.setResults(prevTestCaseResults);
+      }
     }
 
     storedMapTableTests.put(tableTest.getName(), tableTest);
@@ -289,7 +286,35 @@ public Table addTableTest(UUID tableId, TableTest tableTest) throws IOException,
         .entityExtensionDAO()
         .insert(tableId.toString(), "table.tableTests", "tableTest", JsonUtils.pojoToJson(updatedTests));
     setFields(table, Fields.EMPTY_FIELDS);
-    return table.withTableTests(getTableTests(table));
+    // return the only test instead of querying all tests and results
+    return table.withTableTests(List.of(tableTest));
+  }
+
+  @Transaction
+  public Table deleteTableTest(UUID tableId, String tableTestType) throws IOException, ParseException {
+    // Validate the request content
+    Table table = daoCollection.tableDAO().findEntityById(tableId);
+    // if ID is not passed we treat it as a new test case being added
+    List<TableTest> storedTableTests = getTableTests(table);
+    // we will override any test case name passed by user/client with tableName + testType
+    // our assumption is there is only one instance of a test type as of now.
+    String tableTestName = table.getName() + "." + tableTestType;
+    Map<String, TableTest> storedMapTableTests = new HashMap<>();
+    if (storedTableTests != null) {
+      for (TableTest t : storedTableTests) {
+        storedMapTableTests.put(t.getName(), t);
+      }
+    }
+    if (!storedMapTableTests.containsKey(tableTestName)) {
+      throw new EntityNotFoundException(String.format("Failed to find %s for %s", tableTestName, table.getName()));
+    }
+    TableTest deleteTableTest = storedMapTableTests.get(tableTestName);
+    storedMapTableTests.remove(tableTestName);
+    List<TableTest> updatedTests = new ArrayList<>(storedMapTableTests.values());
+    daoCollection
+        .entityExtensionDAO()
+        .insert(tableId.toString(), "table.tableTests", "tableTest", JsonUtils.pojoToJson(updatedTests));
+    return table.withTableTests(List.of(deleteTableTest));
   }
 
   @Transaction
@@ -300,7 +325,7 @@ public Table addColumnTest(UUID tableId, ColumnTest columnTest) throws IOExcepti
     validateColumn(table, columnName);
     // we will override any test case name passed by user/client with columnName + testType
     // our assumption is there is only one instance of a test type as of now.
-    columnTest.setName(columnName + "." + columnTest.getTestCase().getTestType().toString());
+    columnTest.setName(columnName + "." + columnTest.getTestCase().getColumnTestType().toString());
     List<ColumnTest> storedColumnTests = getColumnTests(table, columnName);
     Map<String, ColumnTest> storedMapColumnTests = new HashMap<>();
     if (storedColumnTests != null) {
@@ -309,20 +334,17 @@ public Table addColumnTest(UUID tableId, ColumnTest columnTest) throws IOExcepti
       }
     }
 
-    // new test, generate UUID
-    if (!storedMapColumnTests.containsKey(columnTest.getName())) {
-      columnTest.setId(UUID.randomUUID());
-    }
-
-    // process test result
-    if (storedMapColumnTests.containsKey(columnTest.getName())
-        && columnTest.getResults() != null
-        && !columnTest.getResults().isEmpty()) {
+    // existingTest use the previous UUID
+    if (storedMapColumnTests.containsKey(columnTest.getName())) {
       ColumnTest prevColumnTest = storedMapColumnTests.get(columnTest.getName());
-      List<TestCaseResult> prevTestCaseResults = prevColumnTest.getResults();
-      List<TestCaseResult> newTestCaseResults = columnTest.getResults();
-      newTestCaseResults.addAll(prevTestCaseResults);
-      columnTest.setResults(newTestCaseResults);
+      columnTest.setId(prevColumnTest.getId());
+
+      // process test results
+      if (columnTest.getResults() != null && !columnTest.getResults().isEmpty()) {
+        List<TestCaseResult> prevTestCaseResults = prevColumnTest.getResults();
+        prevTestCaseResults.addAll(columnTest.getResults());
+        columnTest.setResults(prevTestCaseResults);
+      }
     }
 
     storedMapColumnTests.put(columnTest.getName(), columnTest);
@@ -332,7 +354,48 @@ public Table addColumnTest(UUID tableId, ColumnTest columnTest) throws IOExcepti
         .entityExtensionDAO()
         .insert(table.getId().toString(), extension, "columnTest", JsonUtils.pojoToJson(updatedTests));
     setFields(table, Fields.EMPTY_FIELDS);
-    getColumnTests(true, table);
+    // return the newly created/updated column test only
+    for (Column column : table.getColumns()) {
+      if (column.getName().equals(columnName)) {
+        column.setColumnTests(List.of(columnTest));
+      }
+    }
+    return table;
+  }
+
+  @Transaction
+  public Table deleteColumnTest(UUID tableId, String columnName, String columnTestType) throws IOException {
+    // Validate the request content
+    Table table = daoCollection.tableDAO().findEntityById(tableId);
+    validateColumn(table, columnName);
+    // we will override any test case name passed by user/client with columnName + testType
+    // our assumption is there is only one instance of a test type as of now.
+    String columnTestName = columnName + "." + columnTestType;
+    List<ColumnTest> storedColumnTests = getColumnTests(table, columnName);
+    Map<String, ColumnTest> storedMapColumnTests = new HashMap<>();
+    if (storedColumnTests != null) {
+      for (ColumnTest ct : storedColumnTests) {
+        storedMapColumnTests.put(ct.getName(), ct);
+      }
+    }
+
+    if (!storedMapColumnTests.containsKey(columnTestName)) {
+      throw new EntityNotFoundException(String.format("Failed to find %s for %s", columnTestName, table.getName()));
+    }
+
+    ColumnTest deleteColumnTest = storedMapColumnTests.get(columnTestName);
+    storedMapColumnTests.remove(columnTestName);
+    List<ColumnTest> updatedTests = new ArrayList<>(storedMapColumnTests.values());
+    String extension = "table.column." + columnName + ".tests";
+    daoCollection
+        .entityExtensionDAO()
+        .insert(table.getId().toString(), extension, "columnTest", JsonUtils.pojoToJson(updatedTests));
+    // return the newly created/updated column test only
+    for (Column column : table.getColumns()) {
+      if (column.getName().equals(columnName)) {
+        column.setColumnTests(List.of(deleteColumnTest));
+      }
+    }
     return table;
   }
 

diff --git a/...est-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java b/...est-service/src/main/java/org/openmetadata/catalog/resources/databases/TableResource.java
@@ -52,6 +52,8 @@
 import javax.ws.rs.core.UriInfo;
 import org.openmetadata.catalog.Entity;
 import org.openmetadata.catalog.api.data.CreateTable;
+import org.openmetadata.catalog.api.tests.CreateColumnTest;
+import org.openmetadata.catalog.api.tests.CreateTableTest;
 import org.openmetadata.catalog.entity.data.Table;
 import org.openmetadata.catalog.jdbi3.CollectionDAO;
 import org.openmetadata.catalog.jdbi3.TableRepository;
@@ -521,27 +523,64 @@ public Table addTableTest(
       @Context UriInfo uriInfo,
       @Context SecurityContext securityContext,
       @Parameter(description = "Id of the table", schema = @Schema(type = "string")) @PathParam("id") String id,
-      TableTest tableTest)
+      CreateTableTest createTableTest)
       throws IOException, ParseException {
     SecurityUtil.checkAdminOrBotRole(authorizer, securityContext);
+    TableTest tableTest = getTableTest(securityContext, createTableTest);
     Table table = dao.addTableTest(UUID.fromString(id), tableTest);
     return addHref(uriInfo, table);
   }
 
+  @DELETE
+  @Path("/{id}/tableTest/{tableTestType}")
+  @Operation(summary = "delete table test case", tags = "tables", description = "Delete test case from the table.")
+  public Table deleteTableTest(
+      @Context UriInfo uriInfo,
+      @Context SecurityContext securityContext,
+      @Parameter(description = "Id of the table", schema = @Schema(type = "string")) @PathParam("id") String id,
+      @Parameter(description = "Table Test Type", schema = @Schema(type = "string")) @PathParam("tableTestType")
+          String tableTestType)
+      throws IOException, ParseException {
+    SecurityUtil.checkAdminOrBotRole(authorizer, securityContext);
+    Table table = dao.deleteTableTest(UUID.fromString(id), tableTestType);
+    return addHref(uriInfo, table);
+  }
+
   @PUT
   @Path("/{id}/columnTest")
   @Operation(summary = "Add table test cases", tags = "tables", description = "Add test cases to the table.")
   public Table addColumnTest(
       @Context UriInfo uriInfo,
       @Context SecurityContext securityContext,
       @Parameter(description = "Id of the table", schema = @Schema(type = "string")) @PathParam("id") String id,
-      ColumnTest columnTest)
+      CreateColumnTest createColumnTest)
       throws IOException, ParseException {
     SecurityUtil.checkAdminOrBotRole(authorizer, securityContext);
+    ColumnTest columnTest = getColumnTest(securityContext, createColumnTest);
     Table table = dao.addColumnTest(UUID.fromString(id), columnTest);
     return addHref(uriInfo, table);
   }
 
+  @DELETE
+  @Path("/{id}/columnTest/{columnName}/{columnTestType}")
+  @Operation(
+      summary = "delete column test case",
+      tags = "tables",
+      description = "Delete column test case from the table.")
+  public Table deleteColumnTest(
+      @Context UriInfo uriInfo,
+      @Context SecurityContext securityContext,
+      @Parameter(description = "Id of the table", schema = @Schema(type = "string")) @PathParam("id") String id,
+      @Parameter(description = "column of the table", schema = @Schema(type = "string")) @PathParam("columnName")
+          String columnName,
+      @Parameter(description = "column Test Type", schema = @Schema(type = "string")) @PathParam("columnTestType")
+          String columnTestType)
+      throws IOException, ParseException {
+    SecurityUtil.checkAdminOrBotRole(authorizer, securityContext);
+    Table table = dao.deleteColumnTest(UUID.fromString(id), columnName, columnTestType);
+    return addHref(uriInfo, table);
+  }
+
   @DELETE
   @Path("/{id}/followers/{userId}")
   @Operation(
@@ -598,4 +637,29 @@ private Table getTable(SecurityContext securityContext, CreateTable create) {
         .withUpdatedAt(System.currentTimeMillis())
         .withDatabase(create.getDatabase());
   }
+
+  private TableTest getTableTest(SecurityContext securityContext, CreateTableTest create) {
+    return new TableTest()
+        .withId(UUID.randomUUID())
+        .withDescription(create.getDescription())
+        .withTestCase(create.getTestCase())
+        .withOwner(create.getOwner())
+        .withExecutionFrequency(create.getExecutionFrequency())
+        .withResults(create.getResult() != null ? List.of(create.getResult()) : new ArrayList<>())
+        .withUpdatedBy(securityContext.getUserPrincipal().getName())
+        .withUpdatedAt(System.currentTimeMillis());
+  }
+
+  private ColumnTest getColumnTest(SecurityContext securityContext, CreateColumnTest create) {
+    return new ColumnTest()
+        .withId(UUID.randomUUID())
+        .withDescription(create.getDescription())
+        .withTestCase(create.getTestCase())
+        .withColumnName(create.getColumnName())
+        .withOwner(create.getOwner())
+        .withExecutionFrequency(create.getExecutionFrequency())
+        .withResults(create.getResult() != null ? List.of(create.getResult()) : new ArrayList<>())
+        .withUpdatedBy(securityContext.getUserPrincipal().getName())
+        .withUpdatedAt(System.currentTimeMillis());
+  }
 }
diff --git a/catalog-rest-service/src/main/resources/json/schema/api/tests/createColumnTest.json b/catalog-rest-service/src/main/resources/json/schema/api/tests/createColumnTest.json
@@ -0,0 +1,41 @@
+{
+  "$id": "https://open-metadata.org/schema/api/tests/columnTest.json",
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "CreateColumnTestRequest",
+  "description": "ColumnTest is a test definition to capture data quality tests against tables and columns.",
+  "type": "object",
+  "properties": {
+    "description": {
+      "description": "Description of the testcase.",
+      "type": "string"
+    },
+    "columnName": {
+      "description": "Name of the column in a table.",
+      "type": "string"
+    },
+    "testCase": {
+      "$ref": "../../tests/columnTest.json#/definitions/columnTestCase"
+    },
+    "executionFrequency": {
+      "$ref": "../../tests/basic.json#/definitions/testCaseExecutionFrequency"
+    },
+    "result": {
+      "$ref": "../../tests/basic.json#/definitions/testCaseResult"
+    },
+    "owner": {
+      "description": "Owner of this Pipeline.",
+      "$ref": "../../type/entityReference.json",
+      "default": null
+    },
+    "updatedAt": {
+      "description": "Last update time corresponding to the new version of the entity in Unix epoch time milliseconds.",
+      "$ref": "../../type/basic.json#/definitions/timestamp"
+    },
+    "updatedBy": {
+      "description": "User who made the update.",
+      "type": "string"
+    }
+  },
+  "required": ["columnName", "testCase"],
+  "additionalProperties": false
+}
diff --git a/catalog-rest-service/src/main/resources/json/schema/api/tests/createTableTest.json b/catalog-rest-service/src/main/resources/json/schema/api/tests/createTableTest.json
@@ -0,0 +1,37 @@
+{
+  "$id": "https://open-metadata.org/schema/api/tests/tableTest.json",
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "CreateTableTestRequest",
+  "description": "TableTest is a test definition to capture data quality tests against tables and columns.",
+  "type": "object",
+  "properties": {
+    "description": {
+      "description": "Description of the testcase.",
+      "type": "string"
+    },
+    "testCase": {
+      "$ref": "../../tests/tableTest.json#/definitions/tableTestCase"
+    },
+    "executionFrequency": {
+      "$ref": "../../tests/basic.json#/definitions/testCaseExecutionFrequency"
+    },
+    "result": {
+      "$ref": "../../tests/basic.json#/definitions/testCaseResult"
+    },
+    "owner": {
+      "description": "Owner of this Pipeline.",
+      "$ref": "../../type/entityReference.json",
+      "default": null
+    },
+    "updatedAt": {
+      "description": "Last update time corresponding to the new version of the entity in Unix epoch time milliseconds.",
+      "$ref": "../../type/basic.json#/definitions/timestamp"
+    },
+    "updatedBy": {
+      "description": "User who made the update.",
+      "type": "string"
+    }
+  },
+  "required": ["testCase"],
+  "additionalProperties": false
+}
diff --git a/catalog-rest-service/src/main/resources/json/schema/tests/basic.json b/catalog-rest-service/src/main/resources/json/schema/tests/basic.json
@@ -13,7 +13,7 @@
           "description": "Data one which profile is taken.",
           "$ref": "../type/basic.json#/definitions/timestamp"
         },
-        "status": {
+        "testCaseStatus": {
           "description": "Status of Test Case run.",
           "javaType": "org.openmetadata.catalog.tests.type.TestCaseStatus",
           "type": "string",

diff --git a/catalog-rest-service/src/main/resources/json/schema/tests/columnTest.json b/catalog-rest-service/src/main/resources/json/schema/tests/columnTest.json
@@ -9,6 +9,7 @@
     "columnTestCase": {
       "description": "Column Test Case.",
       "type": "object",
+      "javaType": "org.openmetadata.catalog.tests.ColumnTestCase",
       "properties": {
         "config": {
           "oneOf": [
@@ -35,7 +36,7 @@
             }
           ]
         },
-        "testType": {
+        "columnTestType": {
           "enum": [
             "columnValuesToBeUnique",
             "columnValuesToBeNotNull",
@@ -94,11 +95,6 @@
     "updatedBy": {
       "description": "User who made the update.",
       "type": "string"
-    },
-    "deleted": {
-      "description": "When `true` indicates the entity has been soft deleted.",
-      "type": "boolean",
-      "default": false
     }
   },
   "required": ["name", "column", "testCase"],