From 11ec3772455e9e942c3fb4c052e64c1d94e0a412 Mon Sep 17 00:00:00 2001 From: Teddy Date: Fri, 25 Jul 2025 09:01:36 +0200 Subject: [PATCH] Add parameter validation for test case parameter names (#22493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add parameter validation for test case parameter names Ensure that parameter names in test case parameterValues match the names defined in the test definition parameterDefinition. This addresses issue #10623 by preventing test cases from being created with invalid parameter names. - Enhanced validateTestParameters method with parameter name validation - Added comprehensive error messaging - Added test coverage for the new validation logic 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Co-authored-by: Teddy * fix: test case + GX exception * fix: breaking tests * fix: failing test --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Co-authored-by: Teddy --- CLAUDE.md | 24 +++++++- .../service/jdbi3/TestCaseRepository.java | 61 +++++++++++++++---- .../dqtests/TestCaseResourceTest.java | 48 ++++++++++----- 3 files changed, 103 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9779f5f224f..d33331d9722 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,4 +109,26 @@ yarn parse-schema # Parse JSON schemas for frontend - JWT-based authentication with OAuth2/SAML support - Role-based access control defined in Java entities - Security configurations in `conf/openmetadata.yaml` -- Never commit secrets - use environment variables or secure vaults \ No newline at end of file +- Never commit secrets - use environment variables or secure vaults + +## Code Generation Standards + +### Comments Policy +- **Do NOT add unnecessary comments** - write self-documenting code +- Only include comments for: + - Complex business logic that isn't obvious + - Non-obvious algorithms or workarounds + - Public API JavaDoc documentation + - TODO/FIXME with ticket references +- Avoid obvious comments like `// increment counter` or `// create new user` + +### Java Code Requirements +- **Always mention** running `mvn spotless:apply` when generating/modifying .java files +- Use clear, descriptive variable and method names instead of comments +- Follow existing project patterns and conventions +- Generate production-ready code, not tutorial code + +### Response Format +- Provide clean code blocks without unnecessary explanations +- Assume readers are experienced developers +- Focus on functionality over education 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 f2969320331..6028f7d6603 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 @@ -26,12 +26,15 @@ import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.stream.Collectors; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.jdbi.v3.sqlobject.transaction.Transaction; +import org.jetbrains.annotations.NotNull; import org.openmetadata.schema.EntityInterface; import org.openmetadata.schema.EntityTimeSeriesInterface; import org.openmetadata.schema.api.feed.CloseTask; @@ -44,6 +47,7 @@ import org.openmetadata.schema.tests.TestCaseParameter; import org.openmetadata.schema.tests.TestCaseParameterValidationRule; import org.openmetadata.schema.tests.TestCaseParameterValue; import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.tests.TestPlatform; import org.openmetadata.schema.tests.TestSuite; import org.openmetadata.schema.tests.type.Resolved; import org.openmetadata.schema.tests.type.TestCaseFailureReasonType; @@ -427,7 +431,11 @@ public class TestCaseRepository extends EntityRepository { Entity.getEntity(test.getTestDefinition(), "", Include.NON_DELETED); test.setTestDefinition(testDefinition.getEntityReference()); - validateTestParameters(test.getParameterValues(), testDefinition.getParameterDefinition()); + validateTestParameters( + test.getParameterValues(), + testDefinition.getParameterDefinition(), + testDefinition.getTestPlatforms(), + testDefinition); validateColumnTestCase(entityLink, testDefinition.getEntityType()); } @@ -500,30 +508,57 @@ public class TestCaseRepository extends EntityRepository { } private void validateTestParameters( - List parameterValues, List parameterDefinition) { + List parameterValues, + List parameterDefinition, + List testPlatforms, + TestDefinition testDefinition) { if (parameterValues != null) { if (parameterDefinition.isEmpty() && !parameterValues.isEmpty()) { throw new IllegalArgumentException( "Parameter Values doesn't match Test Definition Parameters"); } - Map values = new HashMap<>(); - for (TestCaseParameterValue testCaseParameterValue : parameterValues) { - values.put(testCaseParameterValue.getName(), testCaseParameterValue.getValue()); - } - for (TestCaseParameter parameter : parameterDefinition) { - if (Boolean.TRUE.equals(parameter.getRequired()) - && (!values.containsKey(parameter.getName()) - || values.get(parameter.getName()) == null)) { - throw new IllegalArgumentException( - "Required parameter " + parameter.getName() + " is not passed in parameterValues"); + if (!testPlatforms.contains(TestPlatform.GREAT_EXPECTATIONS) + && !testDefinition.getName().equals("tableDiff")) { + Set definedParameterNames = + parameterDefinition.stream() + .map(TestCaseParameter::getName) + .collect(Collectors.toSet()); + + Map values = getParameterValuesMap(parameterValues, definedParameterNames); + for (TestCaseParameter parameter : parameterDefinition) { + if (Boolean.TRUE.equals(parameter.getRequired()) + && (!values.containsKey(parameter.getName()) + || values.get(parameter.getName()) == null)) { + throw new IllegalArgumentException( + "Required parameter " + parameter.getName() + " is not passed in parameterValues"); + } + validateParameterRule(parameter, values); } - validateParameterRule(parameter, values); } } } + private @NotNull Map getParameterValuesMap( + List parameterValues, Set definedParameterNames) { + Map values = new HashMap<>(); + + for (TestCaseParameterValue testCaseParameterValue : parameterValues) { + String parameterName = testCaseParameterValue.getName(); + + if (!definedParameterNames.contains(parameterName)) { + throw new IllegalArgumentException( + String.format( + "Parameter '%s' is not defined in the test definition. Defined parameters are: %s", + parameterName, definedParameterNames)); + } + + values.put(parameterName, testCaseParameterValue.getValue()); + } + return values; + } + private void validateColumnTestCase( EntityLink entityLink, TestDefinitionEntityType testDefinitionEntityType) { if (testDefinitionEntityType.equals(TestDefinitionEntityType.COLUMN)) { 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 07013274472..d2f69bca1f4 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 @@ -516,6 +516,19 @@ public class TestCaseResourceTest extends EntityResourceTest createAndCheckEntity(create1, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Required parameter missingCountValue is not passed in parameterValues"); + + CreateTestCase create2 = createRequest(test); + create2 + .withEntityLink(TABLE_LINK) + .withTestDefinition(TEST_DEFINITION3.getFullyQualifiedName()) + .withParameterValues( + List.of( + new TestCaseParameterValue().withName("missingCountValue").withValue("10"), + new TestCaseParameterValue().withName("invalidParameter").withValue("20"))); + assertResponseContains( + () -> createAndCheckEntity(create2, ADMIN_AUTH_HEADERS), + BAD_REQUEST, + "Parameter 'invalidParameter' is not defined in the test definition. Defined parameters are: [missingValueMatch, missingCountValue]"); } @Test @@ -526,7 +539,7 @@ public class TestCaseResourceTest extends EntityResourceTest queryParams = new HashMap<>(); @@ -816,7 +829,7 @@ public class TestCaseResourceTest extends EntityResourceTest", tableFQN)) .withTestDefinition(TEST_DEFINITION4.getFullyQualifiedName()) .withParameterValues( - List.of(new TestCaseParameterValue().withValue("20").withName("min"))); + List.of(new TestCaseParameterValue().withValue("20").withName("minValue"))); TestCase testCase = createEntity(create, ADMIN_AUTH_HEADERS); testCaseCount++; CreateTestCaseResult createTestCaseResult = @@ -902,7 +915,7 @@ public class TestCaseResourceTest extends EntityResourceTest", tableFQN)) .withTestDefinition(TEST_DEFINITION4.getFullyQualifiedName()) .withParameterValues( - List.of(new TestCaseParameterValue().withValue("20").withName("max"))); + List.of(new TestCaseParameterValue().withValue("20").withName("maxValue"))); if (i == 2) { // create 1 test cases with USER21_TEAM as owner create.withOwners(List.of(teamRef)); @@ -1133,7 +1146,7 @@ public class TestCaseResourceTest extends EntityResourceTest", table.getFullyQualifiedName())) .withTestDefinition(TEST_DEFINITION4.getFullyQualifiedName()) .withParameterValues( - List.of(new TestCaseParameterValue().withValue("20").withName("min"))); + List.of(new TestCaseParameterValue().withValue("20").withName("minValue"))); createEntity(create, ADMIN_AUTH_HEADERS); create = createRequest(testInfo) @@ -2086,13 +2099,14 @@ public class TestCaseResourceTest extends EntityResourceTest createEntity(invalidTestCase, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Value"); + () -> createEntity(invalidTestCase, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Parameter"); CreateTestCase invalidTestCaseMixedTypes = createRequest(test, 3); invalidTestCaseMixedTypes @@ -2115,7 +2129,9 @@ public class TestCaseResourceTest extends EntityResourceTest createEntity(invalidTestCaseMixedTypes, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Value"); + () -> createEntity(invalidTestCaseMixedTypes, ADMIN_AUTH_HEADERS), + BAD_REQUEST, + "Parameter"); } @Test @@ -2155,7 +2171,7 @@ public class TestCaseResourceTest extends EntityResourceTest columns = Arrays.asList(C1, C2, C3); @@ -2209,7 +2225,7 @@ public class TestCaseResourceTest extends EntityResourceTest columns = Arrays.asList("NOT_A_COLUMN", C1, C2, C3); @@ -2248,7 +2264,7 @@ public class TestCaseResourceTest extends EntityResourceTest columns = Arrays.asList(C1, C2, C3); @@ -2338,7 +2354,7 @@ public class TestCaseResourceTest extends EntityResourceTest