From 7407b3c9f5b3486db8f060fc867ddded8a3ee50c Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Sun, 3 Aug 2025 08:12:01 +0200 Subject: [PATCH] MINOR - Don't enforce data contract on PATCH & cleanups (#22709) * MINOR - Don't enforce data contract on PATCH & cleanups * fix pipeline refs for test suite * fix tests --- .../service/jdbi3/DataContractRepository.java | 93 ++++++++++++++----- .../service/rules/RuleEngine.java | 69 +++++++++----- .../service/resources/EntityResourceTest.java | 2 +- .../data/DataContractResourceTest.java | 29 +++++- .../resources/feeds/FeedResourceTest.java | 36 ++----- .../service/rules/RuleEngineTests.java | 39 ++++---- 6 files changed, 169 insertions(+), 99 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java index 930500f6f56..9a5fbef75b0 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java @@ -17,7 +17,6 @@ import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; import static org.openmetadata.schema.type.EventType.ENTITY_CREATED; import static org.openmetadata.schema.type.EventType.ENTITY_UPDATED; import static org.openmetadata.service.Entity.ADMIN_USER_NAME; -import static org.openmetadata.service.Entity.TEST_SUITE; import jakarta.ws.rs.core.Response; import java.util.ArrayList; @@ -45,6 +44,7 @@ import org.openmetadata.schema.entity.datacontract.SchemaValidation; import org.openmetadata.schema.entity.datacontract.SemanticsValidation; import org.openmetadata.schema.entity.services.ingestionPipelines.AirflowConfig; import org.openmetadata.schema.entity.services.ingestionPipelines.IngestionPipeline; +import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineServiceClientResponse; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineType; import org.openmetadata.schema.metadataIngestion.SourceConfig; import org.openmetadata.schema.metadataIngestion.TestSuitePipeline; @@ -151,12 +151,56 @@ public class DataContractRepository extends EntityRepository { if (!nullOrEmpty(dataContract.getReviewers())) { dataContract.setReviewers(EntityUtil.populateEntityReferences(dataContract.getReviewers())); } + createOrUpdateDataContractTestSuite(dataContract); + } - TestSuite testSuite = createOrUpdateDataContractTestSuite(dataContract); - // Create the ingestion pipeline only if needed - if (testSuite != null && nullOrEmpty(testSuite.getPipelines())) { - IngestionPipeline pipeline = createIngestionPipeline(testSuite); - prepareAndDeployIngestionPipeline(pipeline, testSuite); + // Ensure we have a pipeline after creation if needed + @Override + protected void postCreate(DataContract dataContract) { + super.postCreate(dataContract); + postCreateOrUpdate(dataContract); + } + + // If we update the contract adding DQ validation, add the pipeline if needed + @Override + protected void postUpdate(DataContract original, DataContract updated) { + super.postUpdate(original, updated); + postCreateOrUpdate(updated); + } + + @Override + protected void postDelete(DataContract dataContract) { + super.postDelete(dataContract); + if (!nullOrEmpty(dataContract.getQualityExpectations())) { + TestSuite testSuite = getOrCreateTestSuite(dataContract); + TestSuiteRepository testSuiteRepository = + (TestSuiteRepository) Entity.getEntityRepository(Entity.TEST_SUITE); + testSuiteRepository.delete(ADMIN_USER_NAME, testSuite.getId(), true, true); + } + // Clean status + daoCollection + .entityExtensionTimeSeriesDao() + .delete(dataContract.getFullyQualifiedName(), RESULT_EXTENSION); + } + + private void postCreateOrUpdate(DataContract dataContract) { + if (!nullOrEmpty(dataContract.getQualityExpectations())) { + TestSuite testSuite = getOrCreateTestSuite(dataContract); + // Create the ingestion pipeline only if needed + if (testSuite != null && nullOrEmpty(testSuite.getPipelines())) { + IngestionPipeline pipeline = createIngestionPipeline(testSuite); + EntityReference pipelineRef = + Entity.getEntityReference( + new EntityReference().withId(pipeline.getId()).withType(Entity.INGESTION_PIPELINE), + Include.NON_DELETED); + testSuite.setPipelines(List.of(pipelineRef)); + TestSuiteRepository testSuiteRepository = + (TestSuiteRepository) Entity.getEntityRepository(Entity.TEST_SUITE); + testSuiteRepository.createOrUpdate(null, testSuite, ADMIN_USER_NAME); + if (!pipeline.getDeployed()) { + prepareAndDeployIngestionPipeline(pipeline, testSuite); + } + } } } @@ -320,23 +364,12 @@ public class DataContractRepository extends EntityRepository { .withFullyQualifiedName(dataContract.getFullyQualifiedName()) .withType(Entity.DATA_CONTRACT)); TestSuite newTestSuite = testSuiteMapper.createToEntity(createTestSuite, ADMIN_USER_NAME); - TestSuite createdSuite = testSuiteRepository.create(null, newTestSuite); - storeTestSuiteRelationship(dataContract, createdSuite); - return createdSuite; + return testSuiteRepository.create(null, newTestSuite); } return maybeTestSuite.get(); } - public void storeTestSuiteRelationship(DataContract dataContract, TestSuite testSuite) { - addRelationship( - dataContract.getId(), - testSuite.getId(), - Entity.DATA_CONTRACT, - TEST_SUITE, - Relationship.CONTAINS); - } - // Prepare the Ingestion Pipeline from the test suite that will handle the execution private IngestionPipeline createIngestionPipeline(TestSuite testSuite) { IngestionPipelineRepository pipelineRepository = @@ -414,7 +447,7 @@ public class DataContractRepository extends EntityRepository { // Otherwise, keep it Running and wait for the DQ results to kick in if (!nullOrEmpty(dataContract.getQualityExpectations())) { try { - triggerAndDeployDQValidation(dataContract); + deployAndTriggerDQValidation(dataContract); compileResult(result, ContractExecutionStatus.Running); } catch (Exception e) { LOG.error( @@ -435,7 +468,7 @@ public class DataContractRepository extends EntityRepository { return result; } - public void triggerAndDeployDQValidation(DataContract dataContract) { + public void deployAndTriggerDQValidation(DataContract dataContract) { if (dataContract.getTestSuite() == null) { throw DataContractValidationException.byMessage( String.format( @@ -455,7 +488,11 @@ public class DataContractRepository extends EntityRepository { IngestionPipeline pipeline = Entity.getEntity(testSuite.getPipelines().get(0), "*", Include.NON_DELETED); - prepareAndDeployIngestionPipeline(pipeline, testSuite); + // ensure pipeline is deployed before running + // we deploy the pipeline during post create + if (!pipeline.getDeployed()) { + prepareAndDeployIngestionPipeline(pipeline, testSuite); + } pipelineServiceClient.runPipeline(pipeline, testSuite); } @@ -466,7 +503,14 @@ public class DataContractRepository extends EntityRepository { SecretsManagerFactory.getSecretsManager() .encryptOpenMetadataConnection(openMetadataServerConnection, false)); - pipelineServiceClient.deployPipeline(pipeline, testSuite); + PipelineServiceClientResponse response = + pipelineServiceClient.deployPipeline(pipeline, testSuite); + if (response.getCode() == 200) { + pipeline.setDeployed(true); + IngestionPipelineRepository ingestionPipelineRepository = + (IngestionPipelineRepository) Entity.getEntityRepository(Entity.INGESTION_PIPELINE); + ingestionPipelineRepository.createOrUpdate(null, pipeline, ADMIN_USER_NAME); + } } private SemanticsValidation validateSemantics(DataContract dataContract) { @@ -481,8 +525,11 @@ public class DataContractRepository extends EntityRepository { "*", Include.NON_DELETED); + // We don't enforce the contract since we don't want to load it again. We're already passing + // its rules List failedRules = - RuleEngine.getInstance().evaluateAndReturn(entity, dataContract.getSemantics(), true); + RuleEngine.getInstance() + .evaluateAndReturn(entity, dataContract.getSemantics(), false, false); validation .withFailed(failedRules.size()) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/rules/RuleEngine.java b/openmetadata-service/src/main/java/org/openmetadata/service/rules/RuleEngine.java index 3038c735ace..420eceaa337 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/rules/RuleEngine.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/rules/RuleEngine.java @@ -43,12 +43,16 @@ public class RuleEngine { * Evaluates the default platform entity semantics rules against the provided entity */ public void evaluate(EntityInterface facts) { - evaluate(facts, null, false); + evaluate(facts, null, true, false); + } + + public void evaluate(EntityInterface facts, boolean enforcePlatform, boolean enforceContract) { + evaluate(facts, null, enforcePlatform, enforceContract); } public void evaluateUpdate(EntityInterface original, EntityInterface updated) { - List originalErrors = evaluateAndReturn(original, null, false); - List updatedErrors = evaluateAndReturn(updated, null, false); + List originalErrors = evaluateAndReturn(original, null, true, false); + List updatedErrors = evaluateAndReturn(updated, null, true, false); // If the updated entity is not fixing anything, throw a validation exception if (!nullOrEmpty(updatedErrors) && updatedErrors.size() >= originalErrors.size()) { @@ -56,12 +60,13 @@ public class RuleEngine { } } - public void evaluate(EntityInterface facts, List rules) { - evaluate(facts, rules, false); - } - - public void evaluate(EntityInterface facts, List rules, boolean incomingOnly) { - List erroredRules = evaluateAndReturn(facts, rules, incomingOnly); + public void evaluate( + EntityInterface facts, + List rules, + boolean enforcePlatform, + boolean enforceContract) { + List erroredRules = + evaluateAndReturn(facts, rules, enforcePlatform, enforceContract); raiseErroredRules(erroredRules); } @@ -76,10 +81,37 @@ public class RuleEngine { } public List evaluateAndReturn( - EntityInterface facts, List rules, boolean incomingOnly) { + EntityInterface facts, + List rules, + boolean enforcePlatform, + boolean enforceContract) { + List rulesToEvaluate = + getRulesToEvaluate(facts, rules, enforcePlatform, enforceContract); + List erroredRules = new ArrayList<>(); + rulesToEvaluate.forEach( + rule -> { + if (shouldApplyRule(facts, rule)) { + try { + validateRule(facts, rule); + } catch (RuleValidationException e) { + erroredRules.add(rule); + } + } + }); + + return erroredRules; + } + + private List getRulesToEvaluate( + EntityInterface facts, + List rules, + boolean enforcePlatform, + boolean enforceContract) { ArrayList rulesToEvaluate = new ArrayList<>(); - if (!incomingOnly) { + if (enforcePlatform) { rulesToEvaluate.addAll(getEnabledEntitySemantics()); + } + if (enforceContract) { DataContract entityContract = dataContractRepository.getEntityDataContractSafely(facts); if (entityContract != null && entityContract.getStatus() == ContractStatus.Active @@ -94,20 +126,7 @@ public class RuleEngine { if (nullOrEmpty(rulesToEvaluate)) { return List.of(); // No rules to evaluate } - - List erroredRules = new ArrayList<>(); - rulesToEvaluate.forEach( - rule -> { - if (shouldApplyRule(facts, rule)) { - try { - validateRule(facts, rule); - } catch (RuleValidationException e) { - erroredRules.add(rule); - } - } - }); - - return erroredRules; + return rulesToEvaluate; } public Boolean shouldApplyRule(EntityInterface facts, SemanticsRule rule) { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java index eba09e0e406..2a505327889 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java @@ -678,7 +678,7 @@ public abstract class EntityResourceTest testSuiteResourceTest.getEntityByName(expectedTestSuiteName, "*", ADMIN_AUTH_HEADERS)); + + // Pipeline is also deleted + assertThrows( + HttpResponseException.class, + () -> ingestionPipelineResourceTest.getEntity(pipeline.getId(), "*", ADMIN_AUTH_HEADERS)); } @Test diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/feeds/FeedResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/feeds/FeedResourceTest.java index 215223ee8c0..d4f6be6b938 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/feeds/FeedResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/feeds/FeedResourceTest.java @@ -36,7 +36,6 @@ import static org.openmetadata.service.exception.CatalogExceptionMessage.permiss import static org.openmetadata.service.jdbi3.RoleRepository.DOMAIN_ONLY_ACCESS_ROLE; import static org.openmetadata.service.resources.EntityResourceTest.C1; import static org.openmetadata.service.resources.EntityResourceTest.DOMAIN_ONLY_ACCESS_ROLE_REF; -import static org.openmetadata.service.resources.EntityResourceTest.MULTI_DOMAIN_RULE; import static org.openmetadata.service.resources.EntityResourceTest.USER1; import static org.openmetadata.service.resources.EntityResourceTest.USER2_REF; import static org.openmetadata.service.resources.EntityResourceTest.USER_ADDRESS_TAG_LABEL; @@ -91,15 +90,12 @@ import org.openmetadata.schema.api.feed.CreateThread; import org.openmetadata.schema.api.feed.ResolveTask; import org.openmetadata.schema.api.feed.ThreadCount; import org.openmetadata.schema.api.teams.CreateTeam; -import org.openmetadata.schema.configuration.EntityRulesSettings; import org.openmetadata.schema.entity.data.Table; import org.openmetadata.schema.entity.domains.Domain; import org.openmetadata.schema.entity.events.EventSubscription; import org.openmetadata.schema.entity.feed.Thread; import org.openmetadata.schema.entity.teams.Team; import org.openmetadata.schema.entity.teams.User; -import org.openmetadata.schema.settings.Settings; -import org.openmetadata.schema.settings.SettingsType; import org.openmetadata.schema.type.AnnouncementDetails; import org.openmetadata.schema.type.ChatbotDetails; import org.openmetadata.schema.type.Column; @@ -124,7 +120,7 @@ import org.openmetadata.service.formatter.decorators.MessageDecorator; import org.openmetadata.service.formatter.util.FeedMessage; import org.openmetadata.service.jdbi3.FeedRepository.FilterType; import org.openmetadata.service.jdbi3.RoleRepository; -import org.openmetadata.service.jdbi3.SystemRepository; +import org.openmetadata.service.resources.EntityResourceTest; import org.openmetadata.service.resources.databases.TableResourceTest; import org.openmetadata.service.resources.domains.DomainResourceTest; import org.openmetadata.service.resources.events.EventSubscriptionResourceTest; @@ -1534,8 +1530,8 @@ public class FeedResourceTest extends OpenMetadataApplicationTest { } @Test - void list_threadsWithUserHavingMultipleDomains() throws HttpResponseException, IOException { - toggleMultiDomainSupport(false); // Disable multi-domain support rule for this test + void list_threadsWithUserHavingMultipleDomains() throws IOException { + EntityResourceTest.toggleMultiDomainSupport(false); // Create domains for this test DomainResourceTest domainResourceTest = new DomainResourceTest(); Domain testDomain1 = @@ -1638,13 +1634,13 @@ public class FeedResourceTest extends OpenMetadataApplicationTest { // Clean up domains domainResourceTest.deleteEntity(testDomain1.getId(), ADMIN_AUTH_HEADERS); domainResourceTest.deleteEntity(testDomain2.getId(), ADMIN_AUTH_HEADERS); + EntityResourceTest.toggleMultiDomainSupport(true); } - toggleMultiDomainSupport(true); } @Test - void list_threadsWithComplexDomainScenarios() throws HttpResponseException, IOException { - toggleMultiDomainSupport(false); // Disable multi-domain support rule for this test + void list_threadsWithComplexDomainScenarios() throws IOException { + EntityResourceTest.toggleMultiDomainSupport(false); // Create test domains DomainResourceTest domainResourceTest = new DomainResourceTest(); Domain testDomain1 = @@ -1801,8 +1797,8 @@ public class FeedResourceTest extends OpenMetadataApplicationTest { domainResourceTest.deleteEntity(testDomain1.getId(), ADMIN_AUTH_HEADERS); domainResourceTest.deleteEntity(testDomain2.getId(), ADMIN_AUTH_HEADERS); domainResourceTest.deleteEntity(testDomain3.getId(), ADMIN_AUTH_HEADERS); + EntityResourceTest.toggleMultiDomainSupport(true); } - toggleMultiDomainSupport(true); } @Test @@ -2601,22 +2597,4 @@ public class FeedResourceTest extends OpenMetadataApplicationTest { public static String buildTableFieldLink(String tableFqn, String field) { return String.format("<#E::table::%s::%s>", tableFqn, field); } - - public void toggleMultiDomainSupport(Boolean enable) { - SystemRepository systemRepository = Entity.getSystemRepository(); - - Settings currentSettings = - systemRepository.getConfigWithKey(SettingsType.ENTITY_RULES_SETTINGS.toString()); - EntityRulesSettings entityRulesSettings = - (EntityRulesSettings) currentSettings.getConfigValue(); - entityRulesSettings - .getEntitySemantics() - .forEach( - rule -> { - if (MULTI_DOMAIN_RULE.equals(rule.getName())) { - rule.setEnabled(enable); - } - }); - systemRepository.updateSetting(currentSettings); - } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/rules/RuleEngineTests.java b/openmetadata-service/src/test/java/org/openmetadata/service/rules/RuleEngineTests.java index f7bf76ea5e5..1edce81e38e 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/rules/RuleEngineTests.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/rules/RuleEngineTests.java @@ -174,11 +174,11 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { table.withOwners(List.of(USER1_REF)); assertThrows( RuleValidationException.class, - () -> RuleEngine.getInstance().evaluate(table, List.of(rule), true)); + () -> RuleEngine.getInstance().evaluate(table, List.of(rule), false, false)); // Single team ownership should pass the Semantics Rule table.withOwners(List.of(TEAM11_REF)); - RuleEngine.getInstance().evaluate(table, List.of(rule), true); + RuleEngine.getInstance().evaluate(table, List.of(rule), false, false); } @Test @@ -202,7 +202,7 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { "No glossary validation rule found for tables. Review the entityRulesSettings.json file.")); // No glossary terms, should pass - RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), true); + RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), false, false); // Single glossary term, should pass table.withTags( @@ -211,7 +211,7 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { .withTagFQN("Glossary.Term1") .withSource(TagLabel.TagSource.GLOSSARY))); - RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), true); + RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), false, false); // Multiple glossary terms, should fail table.withTags( @@ -224,7 +224,7 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { .withSource(TagLabel.TagSource.GLOSSARY))); assertThrows( RuleValidationException.class, - () -> RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), true)); + () -> RuleEngine.getInstance().evaluate(table, List.of(glossaryRule), false, false)); } @Test @@ -287,19 +287,21 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { String tableJsonWithContract = JsonUtils.pojoToJson(table); table.withOwners(List.of(TEAM11_REF)); - assertResponse( - () -> - tableResourceTest.patchEntity( - table.getId(), tableJsonWithContract, table, ADMIN_AUTH_HEADERS), - Response.Status.BAD_REQUEST, - "Rule [Description can't be empty] validation failed: Entity does not satisfy the rule. Rule context: Validates that the table has a description."); - - // However, I can PATCH the table to add a proper description - table.withDescription("This is a valid description"); + // I can still PATCH the table to change the owners, even if the contract is broken. + // We don't have hard contract enforcement yet, so this is allowed. Table patched = tableResourceTest.patchEntity( table.getId(), tableJsonWithContract, table, ADMIN_AUTH_HEADERS); assertNotNull(patched); + assertEquals(table.getOwners().getFirst().getId(), TEAM11_REF.getId()); + + // I can PATCH the table to add a proper description as well + tableJsonWithContract = JsonUtils.pojoToJson(patched); + table.withDescription("This is a valid description"); + patched = + tableResourceTest.patchEntity( + table.getId(), tableJsonWithContract, table, ADMIN_AUTH_HEADERS); + assertNotNull(patched); assertEquals("This is a valid description", patched.getDescription()); assertEquals(table.getOwners().getFirst().getId(), TEAM11_REF.getId()); } @@ -350,7 +352,8 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { // Table does indeed blow up assertThrows( - RuleValidationException.class, () -> RuleEngine.getInstance().evaluate(tableWithContract)); + RuleValidationException.class, + () -> RuleEngine.getInstance().evaluate(tableWithContract, false, true)); String tableJsonWithContract = JsonUtils.pojoToJson(tableWithContract); tableWithContract.withDescription("This is a valid description"); @@ -364,7 +367,9 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { ADMIN_AUTH_HEADERS); // The patched table still blows up, since we've only fixed one rule - assertThrows(RuleValidationException.class, () -> RuleEngine.getInstance().evaluate(patched)); + assertThrows( + RuleValidationException.class, + () -> RuleEngine.getInstance().evaluate(patched, true, true)); String patchedJson = JsonUtils.pojoToJson(patched); patched.withOwners(List.of(TEAM11_REF)); @@ -376,7 +381,7 @@ public class RuleEngineTests extends OpenMetadataApplicationTest { // Table is patched properly and is not blowing up anymore assertNotNull(fixedTable); - RuleEngine.getInstance().evaluate(fixedTable); + RuleEngine.getInstance().evaluate(fixedTable, true, true); } @Test