From b92fd5cde4df674a3038d52ae428809c0fd1f230 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Mon, 8 Jan 2024 07:42:15 +0100 Subject: [PATCH] MINOR - Move test case incident to the top of the data_quality_data_time_series (#14600) * Add field and index * MINOR - Move test case incident to the top of the data_quality_data_time_series table * Fix test * Fix compile * Format * Update incidentId * Rename field * Fix patch --- .../native/1.3.0/mysql/schemaChanges.sql | 4 ++ .../native/1.3.0/postgres/schemaChanges.sql | 4 ++ ingestion/src/metadata/utils/logger.py | 6 +- .../service/jdbi3/CollectionDAO.java | 47 +++++++----- .../service/jdbi3/TestCaseRepository.java | 72 ++++++++++++------- .../resources/dqtests/TestCaseResource.java | 2 +- .../dqtests/TestCaseResourceTest.java | 46 +++++++++++- .../resources/json/schema/tests/basic.json | 4 -- .../resources/json/schema/tests/testCase.json | 9 +-- 9 files changed, 137 insertions(+), 57 deletions(-) diff --git a/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql b/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql index 21cc7fd81a8..760041f8321 100644 --- a/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql +++ b/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql @@ -132,3 +132,7 @@ WHERE de.serviceType = 'Databricks' UPDATE dbservice_entity de SET de.json = JSON_REMOVE(de.json, '$.connection.config.useUnityCatalog') WHERE de.serviceType IN ('Databricks','UnityCatalog'); + +-- Add Incident ID for test case results +ALTER TABLE data_quality_data_time_series ADD COLUMN incidentId varchar(36); +ALTER TABLE data_quality_data_time_series ADD INDEX data_quality_data_time_series_incidentId(incidentId); \ No newline at end of file diff --git a/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql b/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql index 13d387face8..e7353801d25 100644 --- a/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql +++ b/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql @@ -144,3 +144,7 @@ WHERE de.serviceType = 'Databricks' UPDATE dbservice_entity de SET json = json #- '{connection,config,useUnityCatalog}' WHERE de.serviceType IN ('Databricks','UnityCatalog'); + +-- Add Incident ID for test case results +ALTER TABLE data_quality_data_time_series ADD COLUMN incidentId varchar(36); +CREATE INDEX IF NOT EXISTS data_quality_data_time_series_incidentId ON data_quality_data_time_series(incidentId); diff --git a/ingestion/src/metadata/utils/logger.py b/ingestion/src/metadata/utils/logger.py index 44a01a8a147..45572abd56d 100644 --- a/ingestion/src/metadata/utils/logger.py +++ b/ingestion/src/metadata/utils/logger.py @@ -254,9 +254,11 @@ def _(record: TableAndTests) -> str: @get_log_name.register -def _(_: TestCaseResults) -> Optional[str]: +def _(record: TestCaseResults) -> str: """We don't want to log this in the status""" - return None + return ",".join( + set(result.testCase.name.__root__ for result in record.test_results) + ) @get_log_name.register diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java index 4b91c7fda29..b8c83cbaf31 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java @@ -3593,31 +3593,46 @@ public interface CollectionDAO { return "data_quality_data_time_series"; } - @ConnectionAwareSqlQuery( + @SqlQuery( value = - "SELECT json FROM data_quality_data_time_series where entityFQNHash = :testCaseFQNHash " - + "AND JSON_EXTRACT(json, '$.incidentId') IS NOT NULL", - connectionType = MYSQL) - @ConnectionAwareSqlQuery( + "SELECT DISTINCT incidentId FROM data_quality_data_time_series " + + "WHERE entityFQNHash = :testCaseFQNHash AND incidentId IS NOT NULL") + List getResultsWithIncidents(@BindFQN("testCaseFQNHash") String testCaseFQNHash); + + @SqlUpdate( value = - "SELECT json FROM data_quality_data_time_series where entityFQNHash = :testCaseFQNHash " - + "AND json ->> 'incidentId' IS NOT NULL", - connectionType = POSTGRES) - List getResultsWithIncidents(@Bind("testCaseFQNHash") String testCaseFQNHash); + "UPDATE data_quality_data_time_series SET incidentId = NULL " + + "WHERE entityFQNHash = :testCaseFQNHash and incidentId = :incidentStateId") + void cleanTestCaseIncident( + @BindFQN("testCaseFQNHash") String testCaseFQNHash, + @Bind("incidentStateId") String incidentStateId); @ConnectionAwareSqlUpdate( value = - "SELECT json FROM data_quality_data_time_series where entityFQNHash = :entityFQNHash " - + "AND JSON_EXTRACT(json, '$.incidentId') IS NOT NULL", + "INSERT INTO data_quality_data_time_series(entityFQNHash, extension, jsonSchema, json, incidentId) " + + "VALUES (:testCaseFQNHash, :extension, :jsonSchema, :json, :incidentStateId)", connectionType = MYSQL) @ConnectionAwareSqlUpdate( value = - "SELECT json FROM data_quality_data_time_series where entityFQNHash = :entityFQNHash " - + "AND json ->> 'incidentId' IS NOT NULL", + "INSERT INTO data_quality_data_time_series(entityFQNHash, extension, jsonSchema, json, incidentId) " + + "VALUES (:testCaseFQNHash, :extension, :jsonSchema, (:json :: jsonb), :incidentStateId)", connectionType = POSTGRES) - // TODO: need to find the right way to get this cleaned - void cleanTestCaseIncidents( - @Bind("entityFQNHash") String entityFQNHash, @Bind("stateId") String stateId); + void insert( + @Define("table") String table, + @BindFQN("testCaseFQNHash") String testCaseFQNHash, + @Bind("extension") String extension, + @Bind("jsonSchema") String jsonSchema, + @Bind("json") String json, + @Bind("incidentStateId") String incidentStateId); + + default void insert( + String entityFQNHash, + String extension, + String jsonSchema, + String json, + String incidentStateId) { + insert(getTimeSeriesTableName(), entityFQNHash, extension, jsonSchema, json, incidentStateId); + } } interface TestCaseResolutionStatusTimeSeriesDAO extends EntityTimeSeriesDAO { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index 7b457fe9853..7eb514d769f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -1,6 +1,7 @@ package org.openmetadata.service.jdbi3; import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; import static org.openmetadata.service.Entity.TEST_CASE; import static org.openmetadata.service.Entity.TEST_DEFINITION; import static org.openmetadata.service.Entity.TEST_SUITE; @@ -15,7 +16,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.UUID; -import java.util.stream.Collectors; import javax.json.JsonPatch; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; @@ -58,7 +58,7 @@ import org.openmetadata.service.util.ResultList; public class TestCaseRepository extends EntityRepository { private static final String TEST_SUITE_FIELD = "testSuite"; private static final String TEST_CASE_RESULT_FIELD = "testCaseResult"; - private static final String INCIDENTS_FIELD = "incidents"; + private static final String INCIDENTS_FIELD = "incidentId"; public static final String COLLECTION_PATH = "/v1/dataQuality/testCases"; private static final String UPDATE_FIELDS = "owner,entityLink,testSuite,testDefinition"; private static final String PATCH_FIELDS = "owner,entityLink,testSuite,testDefinition"; @@ -85,8 +85,8 @@ public class TestCaseRepository extends EntityRepository { fields.contains(TEST_CASE_RESULT_FIELD) ? getTestCaseResult(test) : test.getTestCaseResult()); - test.setIncidents( - fields.contains(INCIDENTS_FIELD) ? getIncidentIds(test) : test.getIncidents()); + test.setIncidentId( + fields.contains(INCIDENTS_FIELD) ? getIncidentId(test) : test.getIncidentId()); } @Override @@ -243,11 +243,13 @@ public class TestCaseRepository extends EntityRepository { // Validate the request content TestCase testCase = findByName(fqn, Include.NON_DELETED); - // set the test case resolution status reference if test failed - testCaseResult.setIncidentId( - testCaseResult.getTestCaseStatus() == TestCaseStatus.Failed - ? createIncidentOnFailure(testCase, updatedBy) - : null); + // set the test case resolution status reference if test failed, by either + // creating a new incident or returning the stateId of an unresolved incident + // for this test case + UUID incidentStateId = null; + if (TestCaseStatus.Failed.equals(testCaseResult.getTestCaseStatus())) { + incidentStateId = getOrCreateIncidentOnFailure(testCase, updatedBy); + } daoCollection .dataQualityDataTimeSeriesDao() @@ -255,7 +257,8 @@ public class TestCaseRepository extends EntityRepository { testCase.getFullyQualifiedName(), TESTCASE_RESULT_EXTENSION, TEST_CASE_RESULT_FIELD, - JsonUtils.pojoToJson(testCaseResult)); + JsonUtils.pojoToJson(testCaseResult), + incidentStateId != null ? incidentStateId.toString() : null); setFieldsInternal(testCase, new EntityUtil.Fields(allowedFields, TEST_SUITE_FIELD)); setTestSuiteSummary( @@ -270,7 +273,7 @@ public class TestCaseRepository extends EntityRepository { Response.Status.CREATED, changeEvent, RestUtil.ENTITY_FIELDS_CHANGED); } - private UUID createIncidentOnFailure(TestCase testCase, String updatedBy) { + private UUID getOrCreateIncidentOnFailure(TestCase testCase, String updatedBy) { TestCaseResolutionStatusRepository testCaseResolutionStatusRepository = (TestCaseResolutionStatusRepository) @@ -510,21 +513,22 @@ public class TestCaseRepository extends EntityRepository { /** * Check all the test case results that have an ongoing incident and get the stateId of the incident */ - private List getIncidentIds(TestCase test) { - List testCaseResults; - testCaseResults = - JsonUtils.readObjects( - daoCollection - .dataQualityDataTimeSeriesDao() - .getResultsWithIncidents( - FullyQualifiedName.buildHash(test.getFullyQualifiedName())), - TestCaseResult.class); + private UUID getIncidentId(TestCase test) { + UUID ongoingIncident = null; - return testCaseResults.stream() - .map(TestCaseResult::getIncidentId) - .collect(Collectors.toSet()) - .stream() - .toList(); + List incidents = + daoCollection + .dataQualityDataTimeSeriesDao() + .getResultsWithIncidents(test.getFullyQualifiedName()) + .stream() + .map(UUID::fromString) + .toList(); + + if (!nullOrEmpty(incidents)) { + ongoingIncident = incidents.get(0); + } + + return ongoingIncident; } public int getTestCaseCount(List testCaseIds) { @@ -715,12 +719,15 @@ public class TestCaseRepository extends EntityRepository { public static class TestCaseFailureResolutionTaskWorkflow extends FeedRepository.TaskWorkflow { final TestCaseResolutionStatusRepository testCaseResolutionStatusRepository; + final CollectionDAO.DataQualityDataTimeSeriesDAO dataQualityDataTimeSeriesDao; TestCaseFailureResolutionTaskWorkflow(FeedRepository.ThreadContext threadContext) { super(threadContext); this.testCaseResolutionStatusRepository = (TestCaseResolutionStatusRepository) Entity.getEntityTimeSeriesRepository(Entity.TEST_CASE_RESOLUTION_STATUS); + + this.dataQualityDataTimeSeriesDao = Entity.getCollectionDAO().dataQualityDataTimeSeriesDao(); } /** @@ -763,9 +770,20 @@ public class TestCaseRepository extends EntityRepository { JsonUtils.pojoToJson(testCaseResolutionStatus)); testCaseResolutionStatusRepository.postCreate(testCaseResolutionStatus); - // TODO: remove incident ID from test case result + // When we resolve a task, we clean up the test case results associated + // with the resolved stateId + dataQualityDataTimeSeriesDao.cleanTestCaseIncident( + latestTestCaseResolutionStatus.getTestCaseReference().getFullyQualifiedName(), + latestTestCaseResolutionStatus.getStateId().toString()); - return Entity.getEntity(testCaseResolutionStatus.getTestCaseReference(), "", Include.ALL); + // Return the TestCase with the StateId to avoid any unnecessary PATCH when resolving the task + // in the feed repo, + // since the `threadContext.getAboutEntity()` will give us the task with the `incidentId` + // informed, which + // we'll remove here. + TestCase testCaseEntity = + Entity.getEntity(testCaseResolutionStatus.getTestCaseReference(), "", Include.ALL); + return testCaseEntity.withIncidentId(latestTestCaseResolutionStatus.getStateId()); } /** diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java index 91a55862053..4a4133e6dbc 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java @@ -73,7 +73,7 @@ import org.openmetadata.service.util.ResultList; public class TestCaseResource extends EntityResource { public static final String COLLECTION_PATH = "/v1/dataQuality/testCases"; - static final String FIELDS = "owner,testSuite,testDefinition,testSuites,incidents"; + static final String FIELDS = "owner,testSuite,testDefinition,testSuites,incidentId"; @Override public TestCase addHref(UriInfo uriInfo, TestCase test) { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java index ce9a7f138e5..ebb84b86644 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java @@ -1089,6 +1089,44 @@ public class TestCaseResourceTest extends EntityResourceTest authHeaders) + throws HttpResponseException { + WebTarget target = getCollection().path("/name/" + fqn); + target = target.queryParam("fields", "incidentId"); + return TestUtils.get(target, TestCase.class, authHeaders); + } + private TestSummary getTestSummary(Map authHeaders, String testSuiteId) throws IOException { TestSuiteResourceTest testSuiteResourceTest = new TestSuiteResourceTest(); @@ -1571,7 +1616,6 @@ public class TestCaseResourceTest extends EntityResourceTest testCaseResultMap = new HashMap<>(); for (TestCaseResult result : actualTestCaseResults.getData()) { - result.setIncidentId(null); testCaseResultMap.put(result.getTimestamp(), result); } for (TestCaseResult result : expectedTestCaseResults) { diff --git a/openmetadata-spec/src/main/resources/json/schema/tests/basic.json b/openmetadata-spec/src/main/resources/json/schema/tests/basic.json index 77aa37d55a8..8ffdf250a5b 100644 --- a/openmetadata-spec/src/main/resources/json/schema/tests/basic.json +++ b/openmetadata-spec/src/main/resources/json/schema/tests/basic.json @@ -86,10 +86,6 @@ "$ref": "#/definitions/testResultValue" } }, - "incidentId": { - "description": "Reference to an ongoing Incident ID (stateId) for this result.", - "$ref": "../type/basic.json#/definitions/uuid" - }, "passedRows": { "description": "Number of rows that passed.", "type": "integer" diff --git a/openmetadata-spec/src/main/resources/json/schema/tests/testCase.json b/openmetadata-spec/src/main/resources/json/schema/tests/testCase.json index d80f6cd9eb4..5c7abd4433f 100644 --- a/openmetadata-spec/src/main/resources/json/schema/tests/testCase.json +++ b/openmetadata-spec/src/main/resources/json/schema/tests/testCase.json @@ -107,12 +107,9 @@ "type": "boolean", "default": false }, - "incidents": { - "description": "List of incident IDs (stateId) for any testCaseResult of a given test case.", - "type": "array", - "items": { - "$ref": "../type/basic.json#/definitions/uuid" - } + "incidentId": { + "description": "Reference to an ongoing Incident ID (stateId) for this test case.", + "$ref": "../type/basic.json#/definitions/uuid" } }, "required": ["name", "testDefinition", "entityLink", "testSuite"],