diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestDefinitionResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestDefinitionResourceIT.java index e65a45312c9d..bdca8fd627e9 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestDefinitionResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestDefinitionResourceIT.java @@ -1,24 +1,29 @@ package org.openmetadata.it.tests; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Arrays; import java.util.List; +import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.openmetadata.it.util.SdkClients; import org.openmetadata.it.util.TestNamespace; +import org.openmetadata.schema.api.teams.CreateUser; import org.openmetadata.schema.api.tests.CreateTestDefinition; import org.openmetadata.schema.tests.TestDefinition; import org.openmetadata.schema.tests.TestPlatform; import org.openmetadata.schema.type.EntityHistory; import org.openmetadata.schema.type.TestDefinitionEntityType; import org.openmetadata.sdk.client.OpenMetadataClient; +import org.openmetadata.sdk.exceptions.InvalidRequestException; import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.models.ListResponse; import org.openmetadata.service.resources.dqtests.TestDefinitionResource; @@ -33,6 +38,11 @@ @Execution(ExecutionMode.CONCURRENT) public class TestDefinitionResourceIT extends BaseEntityIT { + private static final List ENTITY_TYPE_CASINGS = + List.of("COLUMN", "Column", "column", " Column "); + private static final List BLANK_ENTITY_TYPES = List.of("", " "); + private static final int ENTITY_TYPE_FILTER_LIMIT = 1000000; + // Disable tests that don't apply to TestDefinition { supportsFollowers = false; // TestDefinition doesn't support followers @@ -255,4 +265,150 @@ void test_testDefinitionNameUniqueness(TestNamespace ns) { () -> createEntity(request2), "Creating duplicate test definition should fail"); } + + // =================================================================== + // ENTITY TYPE FILTER — issue #29542 + // =================================================================== + + @Test + void list_entityTypeFilterIsCaseInsensitive_200_OK(TestNamespace ns) { + TestDefinition columnDefinition = createColumnTestDefinition(ns, "casing_column"); + TestDefinition tableDefinition = createTableTestDefinition(ns, "casing_table"); + + for (String casing : ENTITY_TYPE_CASINGS) { + assertColumnOnlyListing(SdkClients.adminClient(), casing, columnDefinition, tableDefinition); + } + } + + @Test + void list_entityTypeFilterRejectsUnknownEntityType_400() { + InvalidRequestException exception = + assertThrows( + InvalidRequestException.class, + () -> listByEntityType(SdkClients.adminClient(), "Banana")); + + assertEquals(400, exception.getStatusCode()); + assertTrue( + exception.getMessage().contains("Banana"), + "Error message must name the rejected value, was: " + exception.getMessage()); + } + + @Test + void list_emptyEntityTypeFilterIsIgnored_200_OK(TestNamespace ns) { + TestDefinition columnDefinition = createColumnTestDefinition(ns, "empty_column"); + TestDefinition tableDefinition = createTableTestDefinition(ns, "empty_table"); + + for (String blank : BLANK_ENTITY_TYPES) { + Set fullyQualifiedNames = + fullyQualifiedNamesOf(listByEntityType(SdkClients.adminClient(), blank).getData()); + + assertTrue( + fullyQualifiedNames.contains(columnDefinition.getFullyQualifiedName()), + "A blank entityType must not filter out COLUMN test definitions"); + assertTrue( + fullyQualifiedNames.contains(tableDefinition.getFullyQualifiedName()), + "A blank entityType must not filter out TABLE test definitions"); + } + } + + /** + * Proves that a caller holding no roles at all sees exactly what an admin sees, which is what the + * issue disputed. It does not cover the one configuration that genuinely does diverge: a + * {@code DomainOnlyAccessRole} holder with no domains, whose listing is broken by + * {@code EntityUtil.addDomainQueryParam} overwriting this very query param with the resource type. + * That is a separate defect, unrelated to casing, and is not fixed or exercised here. + */ + @Test + void list_entityTypeFilterYieldsSameResultForAdminAndRoleLessUser_200_OK(TestNamespace ns) { + TestDefinition columnDefinition = createColumnTestDefinition(ns, "rbac_column"); + TestDefinition tableDefinition = createTableTestDefinition(ns, "rbac_table"); + + OpenMetadataClient nonAdminClient = createNonAdminClient(ns); + + for (String casing : ENTITY_TYPE_CASINGS) { + assertColumnOnlyListing(SdkClients.adminClient(), casing, columnDefinition, tableDefinition); + assertColumnOnlyListing(nonAdminClient, casing, columnDefinition, tableDefinition); + } + } + + private OpenMetadataClient createNonAdminClient(TestNamespace ns) { + String name = ns.shortPrefix("etfilter"); + String email = name + "@test.openmetadata.org"; + SdkClients.adminClient().users().create(new CreateUser().withName(name).withEmail(email)); + + return SdkClients.createClient(email, email, new String[] {}); + } + + /** + * Asserts the filtered listing by set membership rather than by set equality: test definitions are + * a global collection and this class runs with {@link ExecutionMode#CONCURRENT}, so a sibling test + * can add or remove one between two list calls. Membership of definitions this test owns is stable + * and still distinguishes a working filter from one that silently returns nothing. + */ + private void assertColumnOnlyListing( + OpenMetadataClient client, + String entityTypeParam, + TestDefinition columnDefinition, + TestDefinition tableDefinition) { + ListResponse response = listByEntityType(client, entityTypeParam); + List definitions = response.getData(); + Set fullyQualifiedNames = fullyQualifiedNamesOf(definitions); + + assertTrue( + definitions.stream().allMatch(d -> d.getEntityType() == TestDefinitionEntityType.COLUMN), + "entityType=" + entityTypeParam + " must return only COLUMN test definitions"); + assertTrue( + response.getPaging().getTotal() > 0, + "entityType=" + + entityTypeParam + + " must produce a non-zero paging total, which is served by the DAO's separate" + + " listCount query"); + assertTrue( + fullyQualifiedNames.contains(columnDefinition.getFullyQualifiedName()), + "entityType=" + + entityTypeParam + + " must return the COLUMN test definition " + + columnDefinition.getFullyQualifiedName()); + assertFalse( + fullyQualifiedNames.contains(tableDefinition.getFullyQualifiedName()), + "entityType=" + + entityTypeParam + + " must not return the TABLE test definition " + + tableDefinition.getFullyQualifiedName()); + } + + private static ListResponse listByEntityType( + OpenMetadataClient client, String entityTypeParam) { + ListParams params = + new ListParams() + .setLimit(ENTITY_TYPE_FILTER_LIMIT) + .addFilter("entityType", entityTypeParam); + + return client.testDefinitions().list(params); + } + + private static Set fullyQualifiedNamesOf(List definitions) { + return definitions.stream() + .map(TestDefinition::getFullyQualifiedName) + .collect(Collectors.toSet()); + } + + private TestDefinition createColumnTestDefinition(TestNamespace ns, String name) { + return createTestDefinition(ns, name, TestDefinitionEntityType.COLUMN); + } + + private TestDefinition createTableTestDefinition(TestNamespace ns, String name) { + return createTestDefinition(ns, name, TestDefinitionEntityType.TABLE); + } + + private TestDefinition createTestDefinition( + TestNamespace ns, String name, TestDefinitionEntityType entityType) { + CreateTestDefinition request = new CreateTestDefinition(); + request.setName(ns.prefix(name)); + request.setDescription("Test definition for entityType filtering"); + request.setEntityType(entityType); + request.setTestPlatforms(List.of(TestPlatform.OPEN_METADATA)); + + return createEntity(request); + } } diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/mcp/McpToolsValidationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/mcp/McpToolsValidationIT.java index c8e4ecdaea6a..a69d8344cf90 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/mcp/McpToolsValidationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/mcp/McpToolsValidationIT.java @@ -343,6 +343,25 @@ void testGetTestDefinitionsForColumn() throws Exception { assertThat(response.get("data").isArray()).isTrue(); } + /** + * The MCP door reaches the same DAO as the REST door, so it must canonicalize {@code entityType} + * the same way — see issue #29542. An LLM caller naturally writes {@code "Column"}, which matched + * nothing on PostgreSQL before the fix. + */ + @Test + @Order(22) + void testGetTestDefinitionsForMixedCaseEntityType() throws Exception { + Map toolCall = McpTestUtils.createGetTestDefinitionsToolCall("Column"); + JsonNode result = executeToolCall(toolCall); + + JsonNode response = OBJECT_MAPPER.readTree(result.get("content").get(0).get("text").asText()); + JsonNode definitions = response.get("data"); + assertThat(definitions).isNotEmpty(); + for (JsonNode definition : definitions) { + assertThat(definition.get("entityType").asText()).isEqualTo("COLUMN"); + } + } + @Test @Order(12) void testCreateTestCase() throws Exception { diff --git a/openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/TestDefinitionsTool.java b/openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/TestDefinitionsTool.java index 72b26451c304..3720bbb8ca9e 100644 --- a/openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/TestDefinitionsTool.java +++ b/openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/TestDefinitionsTool.java @@ -92,9 +92,7 @@ private static String stringParam(Map params, String key, String private static ListFilter buildFilter(String entityType, String testPlatform) { ListFilter filter = new ListFilter(Include.NON_DELETED); - if (entityType != null) { - filter.addQueryParam("entityType", entityType); - } + TestDefinitionRepository.addEntityTypeFilter(filter, entityType); if (testPlatform != null) { filter.addQueryParam("testPlatform", testPlatform); } diff --git a/openmetadata-mcp/src/main/resources/json/data/mcp/tools.json b/openmetadata-mcp/src/main/resources/json/data/mcp/tools.json index 577c23d98ea8..fa1517752c4a 100644 --- a/openmetadata-mcp/src/main/resources/json/data/mcp/tools.json +++ b/openmetadata-mcp/src/main/resources/json/data/mcp/tools.json @@ -768,8 +768,10 @@ "properties": { "entityType": { "type": "string", - "description": "Entity Type can be 'TABLE' for table asset/entity or 'COLUMN' for column level tests. Default is TABLE." - }, + "description": "Entity Type (case-insensitive). Use 'TABLE' for table asset/entity or 'COLUMN' for column level tests. Default is TABLE.", + "default": "TABLE", + "examples": ["TABLE", "COLUMN"] + } "testPlatform": { "type": "string", "description": "Default value can be 'OpenMetadata'. Other Platform can be 'OpenMetadata','GreatExpectations', 'DBT', 'Deequ', 'Soda', 'Other' if the user specifically gives the platform name." diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestDefinitionRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestDefinitionRepository.java index 19699eb0d1d0..0eb8bd31e6b4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestDefinitionRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestDefinitionRepository.java @@ -12,6 +12,7 @@ import org.openmetadata.schema.type.Include; import org.openmetadata.schema.type.ProviderType; import org.openmetadata.schema.type.Relationship; +import org.openmetadata.schema.type.TestDefinitionEntityType; import org.openmetadata.schema.type.change.ChangeSource; import org.openmetadata.service.Entity; import org.openmetadata.service.exception.CatalogExceptionMessage; @@ -21,6 +22,10 @@ @Slf4j public class TestDefinitionRepository extends EntityRepository { + private static final String ENTITY_TYPE_PARAM = "entityType"; + private static final List ENTITY_TYPES = + List.of(TestDefinitionEntityType.values()); + public TestDefinitionRepository() { super( TestDefinitionResource.COLLECTION_PATH, @@ -146,6 +151,36 @@ private void requireNoDependentTestCases(UUID testDefinitionId) { } } + /** + * Canonicalizes the {@code entityType} listing filter for every door into {@link + * CollectionDAO.TestDefinitionDAO}. The DAO compares the value against the {@code + * test_definition.entityType} generated column with {@code =}, which PostgreSQL evaluates + * case-sensitively under the deterministic collations it ships with, so an un-normalized {@code + * Column} silently matched nothing while MySQL's case-insensitive collation matched it — see issue + * #29542. Canonicalizing here keeps both engines and both callers (the REST resource and the MCP + * tool) identical, and turns an unknown value into a {@code 400} instead of an empty page. A blank + * value stays an absent filter so that clients serializing an unset filter are not rejected. + */ + public static void addEntityTypeFilter(ListFilter filter, String entityType) { + String value = CommonUtil.nullOrEmpty(entityType) ? "" : entityType.trim(); + if (!value.isEmpty()) { + filter.addQueryParam(ENTITY_TYPE_PARAM, parseEntityType(value).value()); + } + } + + private static TestDefinitionEntityType parseEntityType(String entityType) { + return ENTITY_TYPES.stream() + .filter(candidate -> candidate.value().equalsIgnoreCase(entityType)) + .findFirst() + .orElseThrow( + () -> + new IllegalArgumentException( + String.format( + "Invalid entityType '%s'. Must be one of %s", + entityType, + ENTITY_TYPES.stream().map(TestDefinitionEntityType::value).toList()))); + } + @Override public EntityRepository.EntityUpdater getUpdater( TestDefinition original, diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java index 923e699ee405..f04bb25d460f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java @@ -101,7 +101,10 @@ public static class TestDefinitionList extends ResultList { @Content( mediaType = "application/json", schema = - @Schema(implementation = TestDefinitionResource.TestDefinitionList.class))) + @Schema(implementation = TestDefinitionResource.TestDefinitionList.class))), + @ApiResponse( + responseCode = "400", + description = "entityType is not one of the TestDefinitionEntityType values") }) public ResultList list( @Context UriInfo uriInfo, @@ -163,9 +166,7 @@ public ResultList list( @QueryParam("enabled") Boolean enabledParam) { ListFilter filter = new ListFilter(include); - if (entityType != null) { - filter.addQueryParam("entityType", entityType); - } + TestDefinitionRepository.addEntityTypeFilter(filter, entityType); if (testPlatformParam != null) { filter.addQueryParam("testPlatform", testPlatformParam); }