diff --git a/.github/playwright/impact-map.generated.json b/.github/playwright/impact-map.generated.json index 098342271d1b..9220db711969 100644 --- a/.github/playwright/impact-map.generated.json +++ b/.github/playwright/impact-map.generated.json @@ -804,6 +804,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -1721,6 +1722,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -2035,6 +2037,7 @@ "playwright/e2e/Features/DataAssetRulesEnabled.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/ExploreQuickFilters.spec.ts", "playwright/e2e/Features/IngestionListNameSorting.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPagePlatformHealth.spec.ts", @@ -3085,6 +3088,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -3307,6 +3311,7 @@ "playwright/e2e/Features/AdvancedSearch.spec.ts", "playwright/e2e/Features/ContextCenterDashboard.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerSqlQuery.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DomainFilterQueryFilter.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPageContextCenter.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPageCriticalAnnouncements.spec.ts", @@ -3497,6 +3502,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -4113,6 +4119,7 @@ "playwright/e2e/Features/DataQuality/DataQualityListAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/DataQualityPermissions.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerDateFilter.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -4845,6 +4852,7 @@ "playwright/e2e/Features/AIMode/ConnectionServiceDetails.spec.ts", "playwright/e2e/Features/Announcements/SystemAnnouncements.spec.ts", "playwright/e2e/Features/BulkEditEntity.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/OnlineUsers.spec.ts", "playwright/e2e/Features/TestSuitePipelineRedeploy.spec.ts", "playwright/e2e/Pages/AIProfile/ProfileMembers.spec.ts", @@ -5173,6 +5181,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -5281,6 +5290,7 @@ "playwright/e2e/Features/DataQuality/DataQuality.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerDateFilter.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerSqlQuery.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/EntitySuggestions.spec.ts", "playwright/e2e/Features/FailedTestCaseSampleData.spec.ts", @@ -6794,6 +6804,14 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts" ] }, + { + "sources": [ + "openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TableProfiler/QualityTab/QualityTab.component.tsx" + ], + "specs": [ + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts" + ] + }, { "sources": [ "openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TestSummary/RunSummaryTiles/RunSummaryTiles.tsx" @@ -12000,6 +12018,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", diff --git a/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql b/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql index 0f05be376676..c5f6fa0051c2 100644 --- a/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql +++ b/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql @@ -650,3 +650,36 @@ SET json = JSON_SET(json, '$.sourceConfig.config.sampleDataCount', 1000) WHERE JSON_UNQUOTE(JSON_EXTRACT(json, '$.sourceConfig.config.type')) = 'AutoClassification' AND JSON_TYPE(JSON_EXTRACT(json, '$.sourceConfig.config.sampleDataCount')) = 'INTEGER' AND JSON_EXTRACT(json, '$.sourceConfig.config.sampleDataCount') > 1000; + +-- Say how to choose tableCustomSQLQuery.strategy and what columnValuesToBeInSet.matchEnum changes, +-- so users and test generation read it from the definition. Seed data only creates missing definitions. +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'strategy', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.' +) +WHERE name = 'tableCustomSQLQuery' + AND JSON_SEARCH(json, 'one', 'strategy', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'matchEnum', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'If true, every observed value must be allowed. If false or omitted, the test passes when at least one allowed value occurs, even if other values are outside the set.' +) +WHERE name = 'columnValuesToBeInSet' + AND JSON_SEARCH(json, 'one', 'matchEnum', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'allowedValues', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'Values used for matching. Neither mode requires every allowed value to occur in the column.' +) +WHERE name = 'columnValuesToBeInSet' + AND JSON_SEARCH(json, 'one', 'allowedValues', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET(json, '$.description', 'Checks column values against an allowed set according to the matchEnum mode.') +WHERE name = 'columnValuesToBeInSet'; diff --git a/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql b/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql index a9c2e8e0b5ea..a6258e983378 100644 --- a/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql +++ b/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql @@ -623,3 +623,55 @@ SET json = jsonb_set(json::jsonb, '{sourceConfig,config,sampleDataCount}', '1000 WHERE json::jsonb -> 'sourceConfig' -> 'config' ->> 'type' = 'AutoClassification' AND jsonb_typeof(json::jsonb -> 'sourceConfig' -> 'config' -> 'sampleDataCount') = 'number' AND (json::jsonb -> 'sourceConfig' -> 'config' ->> 'sampleDataCount')::numeric > 1000; + +-- Say how to choose tableCustomSQLQuery.strategy and what columnValuesToBeInSet.matchEnum changes, +-- so users and test generation read it from the definition. Seed data only creates missing definitions. +UPDATE test_definition +SET json = jsonb_set( + json, + '{parameterDefinition}', + ( + SELECT jsonb_agg( + CASE + WHEN param->>'name' = 'strategy' THEN param || jsonb_build_object( + 'description', + 'How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.' + ) + ELSE param + END + ORDER BY position + ) + FROM jsonb_array_elements(json->'parameterDefinition') WITH ORDINALITY AS params(param, position) + ) +) +WHERE name = 'tableCustomSQLQuery' + AND json->'parameterDefinition' @> '[{"name": "strategy"}]'::jsonb; + +UPDATE test_definition +SET json = jsonb_set( + json, + '{parameterDefinition}', + ( + SELECT jsonb_agg( + CASE + WHEN param->>'name' = 'matchEnum' THEN param || jsonb_build_object( + 'description', + 'If true, every observed value must be allowed. If false or omitted, the test passes when at least one allowed value occurs, even if other values are outside the set.' + ) + WHEN param->>'name' = 'allowedValues' THEN param || jsonb_build_object( + 'description', + 'Values used for matching. Neither mode requires every allowed value to occur in the column.' + ) + ELSE param + END + ORDER BY position + ) + FROM jsonb_array_elements(json->'parameterDefinition') WITH ORDINALITY AS params(param, position) + ) +) +WHERE name = 'columnValuesToBeInSet' + AND json->'parameterDefinition' @> '[{"name": "matchEnum"}]'::jsonb; + +UPDATE test_definition +SET json = jsonb_set(json, '{description}', to_jsonb('Checks column values against an allowed set according to the matchEnum mode.'::text)) +WHERE name = 'columnValuesToBeInSet'; diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md new file mode 100644 index 000000000000..02910de1fe81 --- /dev/null +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -0,0 +1,57 @@ +# A new test case is checked against its definition's options and column types + +- **Status:** Accepted +- **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (checked before the basic test suite is created, and an update checked for what it changes) +- **Deciders:** Shreyansh Verma +- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`, `post_rejectedForConformance_leavesNoTestSuite`) +- **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) + +## Context + +`validateTestParameters` checked parameter names, required parameters and min/max rules. Nothing +compared a value with the parameter's optionValues, or the column's type with the definition's +supportedDataTypes, which no test case was checked against. Test cases that ingestion then +aborted on (`'SUM' is not a valid Strategy`) or that measured nothing were stored as sent. + +## Decision + +A new test case is rejected when it: + +- gives a parameter with optionValues a value that is none of them (compared case-insensitively, + as `TestCaseThresholdValidator` reads thresholdUnit; a null, empty or whitespace value is unset); +- targets a column whose dataType is not in a non-empty supportedDataTypes (an empty list means + every type, as the listing filter reads it; a column with no dataType is accepted). The column is + found case-insensitively, as `validateColumnTestCase` finds it. + +`prepare()` checks a test case when `update` is false, before it creates the table's basic test +suite, the gate the disabled-definition check uses. PUT-create and the CSV import miss that gate +for both checks: `createOrUpdate` prepares with `update` true, and the CSV import does not call +`prepare()`. #35667 fixes those callers. + +An update is checked only for what it changes (`TestUpdater`, through +`TestCaseDefinitionConformance.validateUpdate`): a parameter value it changes must be one of that +parameter's options, and a test moved to another column or definition is checked in full. A row +stored before these rules, or one whose column ingestion has since retyped, stays editable. + +useDynamicAssertion is not checked against supportsDynamicAssertion: a client can still send it on +a definition without dynamic support (the Collate UI does, from a toggle left on), and rejecting it +would break that client. Test generation already sends it only where it is supported. + +## Consequences + +Creates by POST (the UI and API clients), `createMany`, MCP `CreateTestCaseTool` and ODCS +materialization get these checks. Ingestion (dbt, Great Expectations and the Python client's +`create_or_update` create by PUT) and the CSV import get them once #35667 lands. ODCS +materialization catches the `IllegalArgumentException` and records the rule as skipped. dbt and +Great Expectations create their own definitions with no supportedDataTypes or optionValues, so +their test cases pass; a definition from the test library is held to the types and options its +author declared. + +`createMany` prepares every test of a batch before it stores any, so one rejected test fails the +batch with no test case stored. A basic test suite that an earlier test's `prepare()` created is +not rolled back, as with any other `prepare()` rejection in a batch: no transaction spans +`createMany`. + +The ai-platform test generator +applies the same reading before it sends a request (ADR:ai-platform/2026-10-12-generated-test-parameters-conform-to-their-definition), +so a server rejection there means the two have drifted. diff --git a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md new file mode 100644 index 000000000000..ecc61db8bdab --- /dev/null +++ b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md @@ -0,0 +1,36 @@ +# An allowed or forbidden set is read as a Python literal or JSON, in its column's type + +- **Status:** Accepted +- **Revisions:** v1 2026-10-16 (initial) +- **Deciders:** Shreyansh Verma +- **Guard:** `tests/unit/observability/data_quality/validations/test_utils.py` (`parse_set_values`, `is_boolean`), `test_validations_databases.py` (`..._boolean_strings`, `..._boolean_json`, `..._missing_column`), `test_validations_datalake.py` (`test_string_booleans_on_a_datalake_column_stay_strings`) +- **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line); ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create + +## Context + +`columnValuesToBeInSet` and `columnValuesToBeNotInSet` parsed `allowedValues` / `forbiddenValues` +with `literal_eval`. The UI writes a set as a JSON array of strings, and a generated test writes a +JSON array, whose booleans and null are lowercase (`[false]`). `literal_eval` rejects JSON's +lowercase `false` / `true` / `null`, which aborted the test, and on a BOOLEAN column it kept +`"true"` / `"false"` as strings, which an engine such as Databricks refuses to compare with a +boolean. + +## Decision + +`parse_set_values` reads a set as a Python literal and, when `literal_eval` rejects it, as JSON. +`literal_eval` runs first, so a value it read before reads the same. A value neither reads still +Aborts the test, as before. A value that parses but is not a list (a bare string or number) is now +rejected with a ValueError, so it Aborts too. On a column typed BOOLEAN, `"true"` / `"false"` in +any case become booleans; nothing else is mapped. + +The column type is the one OpenMetadata records for the column, which the SQL runner maps to +SQLAlchemy (`SQAValidatorMixin.get_set_values_type`). The pandas runner returns none, so a datalake +set keeps its values as written: `GenericDataFrameColumnParser` types a column of `"True"` / +`"False"` strings as BOOLEAN. + +## Consequences + +A set test no longer aborts on a JSON set, and on a column typed BOOLEAN `"true"` / `"false"` +compare as booleans. On a datalake the set is not typed, so its values compare as written. That is +deliberate: typing it by `GenericDataFrameColumnParser` would turn a set of `"True"` / `"False"` +strings into booleans, which the column's string values do not equal. diff --git a/ingestion/src/metadata/data_quality/validations/base_test_handler.py b/ingestion/src/metadata/data_quality/validations/base_test_handler.py index c5d998df259b..1a6fdce1dc42 100644 --- a/ingestion/src/metadata/data_quality/validations/base_test_handler.py +++ b/ingestion/src/metadata/data_quality/validations/base_test_handler.py @@ -23,6 +23,7 @@ from abc import ABC, abstractmethod from collections.abc import Callable from decimal import Decimal +from functools import partial from typing import ( TYPE_CHECKING, TypedDict, @@ -1356,6 +1357,21 @@ def get_max_bound(self, param_name: str) -> float | None: default=float("inf"), ) + def get_set_param_value(self, param_name: str) -> list: + """An allowed or forbidden set, typed by the column it is checked against""" + return self.get_test_case_param_value( + self.test_case.parameterValues, # type: ignore + param_name, + partial(utils.parse_set_values, column_type=self.get_set_values_type()), + ) + + def get_set_values_type(self): + """The column type a set is parsed against, or None to keep its values as written. + + Implementation provided by SQAValidatorMixin or PandasValidatorMixin via MRO chain. + """ + return super().get_set_values_type() # type: ignore + def get_bounds(self, min_param_name: str, max_param_name: str) -> tuple[float | None, float | None]: """Resolve the test case bounds and widen them by the failure threshold. diff --git a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py index ed3d77b627c2..41861015e365 100644 --- a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py +++ b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py @@ -25,6 +25,18 @@ from sqlalchemy.sql.elements import ClauseElement, ColumnElement +def is_unbounded(bound: Any) -> bool: + """Whether that side of the window lets everything through + + An unset bound resolves to ∓inf, and to None for a validator that resolves its bounds + dynamically and found none on that side. Neither excludes a value, so neither needs a + condition -- and None cannot be compared against at all. Only a float can be infinite: + a datetime window -- what a between test on a date column resolves to -- compares fine + and must not be handed to `math.isinf`, which only takes a number. + """ + return bound is None or (isinstance(bound, float) and math.isinf(bound)) + + class BetweenBoundsChecker(BaseValidationChecker): """Checks if value is outside [min_bound, max_bound]""" @@ -32,17 +44,6 @@ def __init__(self, min_bound: float, max_bound: float): self.min_bound = min_bound self.max_bound = max_bound - def _is_unbounded(self, bound: Any) -> bool: - """Whether that side of the window lets everything through - - An unset bound resolves to ∓inf, and to None for a validator that resolves its bounds - dynamically and found none on that side. Neither excludes a value, so neither needs a - condition -- and None cannot be compared against at all. Only a float can be infinite: - a datetime window -- what a between test on a date column resolves to -- compares fine - and must not be handed to `math.isinf`, which only takes a number. - """ - return bound is None or (isinstance(bound, float) and math.isinf(bound)) - def _check_violations(self, values): """Core violation check logic - works for both scalar and Series. @@ -61,9 +62,9 @@ def _check_violations(self, values): # builds only those conditions: an unset bound excludes nothing, and a None one cannot # be compared against at all. outside = None - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): outside = values < self.min_bound - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): above = values > self.max_bound outside = above if outside is None else (outside | above) @@ -108,9 +109,9 @@ def build_violation_sqa(self, metrics: list["ClauseElement"]) -> "ClauseElement" for expr in metrics: expr_conditions = [] - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): expr_conditions.append(and_(expr.isnot(None), expr < self.min_bound)) - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): expr_conditions.append(and_(expr.isnot(None), expr > self.max_bound)) if expr_conditions: @@ -138,9 +139,9 @@ def build_row_level_violations_sqa(self, column: "ClauseElement") -> "ColumnElem # Build condition: value NOT NULL AND (value < min OR value > max) conditions = [] - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): conditions.append(and_(column.isnot(None), column < self.min_bound)) - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): conditions.append(and_(column.isnot(None), column > self.max_bound)) if not conditions: diff --git a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py index 4f6d32f0dd5d..87bdfa51d5c4 100644 --- a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py @@ -15,7 +15,6 @@ import traceback from abc import abstractmethod -from ast import literal_eval from sqlalchemy import Column @@ -55,9 +54,8 @@ def _run_validation(self) -> TestCaseResult: Returns: TestCaseResult: The test case result for the overall validation """ - test_params = self._get_test_parameters() - try: + test_params = self._get_test_parameters() column: SQALikeColumn | Column = self.get_column() count_in_set = self._run_results(Metrics.countInSet, column, values=test_params[self.ALLOWED_VALUES]) @@ -104,11 +102,7 @@ def _get_test_parameters(self) -> dict: Returns: dict with keys: allowed_values, match_enum """ - allowed_values = self.get_test_case_param_value( - self.test_case.parameterValues, - self.ALLOWED_VALUES, - literal_eval, - ) + allowed_values = self.get_set_param_value(self.ALLOWED_VALUES) match_enum = utils.get_bool_test_case_param(self.test_case.parameterValues, self.MATCH_ENUM) return { diff --git a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py index c03deb512efd..392c85a57b77 100644 --- a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py @@ -15,7 +15,6 @@ import traceback from abc import abstractmethod -from ast import literal_eval from sqlalchemy import Column @@ -53,9 +52,8 @@ def _run_validation(self) -> TestCaseResult: Returns: TestCaseResult: The test case result for the overall validation """ - test_params = self._get_test_parameters() - try: + test_params = self._get_test_parameters() column: SQALikeColumn | Column = self.get_column() metric_values = self._run_results_with_row_count( Metrics.countInSet, column, values=test_params[self.FORBIDDEN_VALUES] @@ -91,11 +89,7 @@ def _get_test_parameters(self) -> dict: Returns: dict with keys: allowed_values, match_enum """ - forbidden_values = self.get_test_case_param_value( - self.test_case.parameterValues, - self.FORBIDDEN_VALUES, - literal_eval, - ) + forbidden_values = self.get_set_param_value(self.FORBIDDEN_VALUES) return { self.FORBIDDEN_VALUES: forbidden_values, diff --git a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py index c4f3574e0634..38c2a0d085b3 100644 --- a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py @@ -13,7 +13,6 @@ Validator for column value to be in set test case """ -from ast import literal_eval from collections import defaultdict from typing import cast @@ -190,11 +189,7 @@ def compute_row_count(self, column: SQALikeColumn): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, - "allowedValues", - literal_eval, - ) + items = self.get_set_param_value("allowedValues") return f"~{self.get_column().name}.isin({items})" def fetch_failed_rows_sample(self): diff --git a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py index 834828fadb74..7dd708d70367 100644 --- a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py @@ -13,7 +13,6 @@ Validator for column value to be not in set test case """ -from ast import literal_eval from collections import defaultdict from typing import cast @@ -178,11 +177,7 @@ def compute_row_count(self, column: SQALikeColumn): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, - "forbiddenValues", - literal_eval, - ) + items = self.get_set_param_value("forbiddenValues") return f"{self.get_column().name}.isin({items})" def fetch_failed_rows_sample(self): diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py index 0ac97b363c60..1ab315f5cbb0 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py @@ -13,7 +13,6 @@ Validator for column value length to be between test case """ -import math from typing import cast from sqlalchemy import Column @@ -22,6 +21,9 @@ DIMENSION_FAILED_COUNT_KEY, DIMENSION_TOTAL_COUNT_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.column.base.columnValueLengthsToBeBetween import ( BaseColumnValueLengthsToBeBetweenValidator, ) @@ -87,20 +89,13 @@ def compute_row_count(self, column: Column, min_bound: int, max_bound: int): NotImplementedError: """ row_count = self._compute_row_count(self.runner, column) - filters = [] - if min_bound > -math.inf: - filters.append((LenFn(column), "lt", min_bound)) - if max_bound < math.inf: - filters.append((LenFn(column), "gt", max_bound)) - failed_rows = self._compute_row_count_between( - self.runner, + failed_rows = self._count_rows_outside( + cast(QueryRunner, self.runner), # noqa: TC006 column, - { - "filters": filters, - "or_filter": True, - }, + LenFn(column), + min_bound, + max_bound, ) - return row_count, failed_rows def _build_dimension_metric_values(self, row, metrics_to_compute, test_params=None): @@ -166,9 +161,9 @@ def filter(self): min_bound = self.get_min_bound(self.MIN_BOUND) max_bound = self.get_max_bound(self.MAX_BOUND) filters = [] - if min_bound is not None and min_bound > float("-inf"): + if not is_unbounded(min_bound): filters.append((LenFn(self.get_column()), "lt", min_bound)) - if max_bound is not None and max_bound < float("inf"): + if not is_unbounded(max_bound): filters.append((LenFn(self.get_column()), "gt", max_bound)) return { "filters": filters, diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py index ba3c51592fb1..f7b1bb83e495 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py @@ -13,7 +13,6 @@ Validator for column values to be between test case """ -import math from datetime import datetime from typing import cast @@ -23,6 +22,9 @@ DIMENSION_FAILED_COUNT_KEY, DIMENSION_TOTAL_COUNT_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.column.base.columnValuesToBeBetween import ( BaseColumnValuesToBeBetweenValidator, ) @@ -145,20 +147,13 @@ def compute_row_count(self, column: Column, min_bound: int, max_bound: int): NotImplementedError: """ row_count = self._compute_row_count(self.runner, column) - filters = [] - if not isinstance(min_bound, (int, float)) or min_bound > -math.inf: - filters.append((column, "lt", min_bound)) - if not isinstance(max_bound, (int, float)) or max_bound < math.inf: - filters.append((column, "gt", max_bound)) - failed_rows = self._compute_row_count_between( - self.runner, + failed_rows = self._count_rows_outside( + cast(QueryRunner, self.runner), # noqa: TC006 + column, column, - { - "filters": filters, - "or_filter": True, - }, + min_bound, + max_bound, ) - return row_count, failed_rows def filter(self): @@ -186,9 +181,9 @@ def filter(self): max_bound = self.get_max_bound(self.MAX_BOUND) filters = [] - if min_bound is not None: + if not is_unbounded(min_bound): filters.append((column, "lt", min_bound)) - if max_bound is not None: + if not is_unbounded(max_bound): filters.append((column, "gt", max_bound)) return { "filters": filters, diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py index 7c95e717b42a..35ba462de23d 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py @@ -13,8 +13,6 @@ Validator for column value to be in set test case """ -from ast import literal_eval - from sqlalchemy import Column, literal from metadata.data_quality.validations.base_test_handler import ( @@ -125,11 +123,7 @@ def compute_row_count(self, column: Column): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "allowedValues", - literal_eval, - ) + items = self.get_set_param_value("allowedValues") return { "filters": [ ( diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py index d6f0551ee2a6..cd2dcd3e741d 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py @@ -13,7 +13,6 @@ Validator for column value to be not in set test case """ -from ast import literal_eval from typing import cast from sqlalchemy import Column @@ -129,11 +128,7 @@ def compute_row_count(self, column: Column): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "forbiddenValues", - literal_eval, - ) + items = self.get_set_param_value("forbiddenValues") return { "filters": [(self.get_column(), "in", items)], "or_filter": False, diff --git a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py index 1dddd2225a03..ec1e951ec938 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py @@ -49,6 +49,11 @@ class PandasValidatorMixin: runner: "PandasRunner" + def get_set_values_type(self) -> None: + """None: GenericDataFrameColumnParser types a column of "True"/"False" strings as BOOLEAN, + so a set keeps its values as written.""" + return + def get_column(self: HasValidatorContext, column_name: str | None = None) -> SQALikeColumn: """Get column object for the given column name diff --git a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py index 3d7c49b404f9..5dbf9cbfdd43 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py @@ -40,6 +40,9 @@ DIMENSION_TOTAL_COUNT_KEY, DIMENSION_VALUE_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.impact_score import ( DEFAULT_TOP_DIMENSIONS, get_impact_score_expression, @@ -76,6 +79,10 @@ class DataQualityQueryType(Enum): class SQAValidatorMixin: """Validator mixin for SQA test cases""" + def get_set_values_type(self): + """The column's type as OpenMetadata records it, mapped to SQLAlchemy, so a set compares in that type""" + return self.get_column().type # type: ignore + def get_column(self: HasValidatorContext, column_name: str | None = None) -> Column: """Get column object for the given column name @@ -235,6 +242,28 @@ def _compute_row_count_between( return res + def _count_rows_outside( + self, + runner: QueryRunner, + column: Column, + expression: ColumnElement, + min_bound: Any, + max_bound: Any, + ) -> int | None: + """Rows whose `expression` falls outside [min_bound, max_bound] + + An unbounded side excludes nothing. With both sides unbounded no row is outside, and a + count with no filter at all would count the whole table. + """ + filters = [ + (expression, operator, bound) + for operator, bound in (("lt", min_bound), ("gt", max_bound)) + if not is_unbounded(bound) + ] + return ( + self._compute_row_count_between(runner, column, {"filters": filters, "or_filter": True}) if filters else 0 + ) + def _compute_row_violations(self, runner: QueryRunner, violations_expr: ColumnElement) -> tuple[Any, Any]: """Count the rows read and the violating ones among them, in a single aggregate query diff --git a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py index 18ffec936cc1..1504b2d0d5c8 100644 --- a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py @@ -78,16 +78,11 @@ def _run_validation(self) -> TestCaseResult: default=0, ) - strategy = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "strategy", - Strategy, - ) + strategy = self._get_strategy() operator = cast(str, operator) # satisfy mypy # noqa: TC006 sql_expression = cast(str, sql_expression) # satisfy mypy # noqa: TC006 threshold = cast(float, threshold) # satisfy mypy # noqa: TC006 - strategy = cast(Strategy, strategy) # satisfy mypy # noqa: TC006 row_count = None try: @@ -140,6 +135,21 @@ def _run_validation(self) -> TestCaseResult: passed_rows=passed_rows, ) + def _get_strategy(self) -> Strategy: + """Read `strategy` in any case, ignoring surrounding spaces; a missing or blank value is ROWS + + ROWS is the definition's default, so a test without a strategy is treated like ROWS, + failed-row samples included. + """ + raw_strategy = self.get_test_case_param_value( + self.test_case.parameterValues, # type: ignore + "strategy", + str, + default="", + ) + value = str(raw_strategy).strip().upper() + return Strategy(value) if value else Strategy.ROWS + def _aborted(self, msg: str) -> TestCaseResult: logger.error(msg) return self.get_test_case_result_object( diff --git a/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py index 645ca071a69b..07dea14b803e 100644 --- a/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py @@ -362,13 +362,6 @@ def compute_row_count(self) -> int | None: return row._asdict().get(Metrics.rowCount.name) return None - def _get_strategy(self): - return self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "strategy", - Strategy, - ) - def fetch_failed_rows_sample(self): cols, rows = self._get_custom_sql_failed_rows() return TableData(columns=cols, rows=rows) diff --git a/ingestion/src/metadata/data_quality/validations/utils.py b/ingestion/src/metadata/data_quality/validations/utils.py index c9c107e4286b..fa0a1b28ed62 100644 --- a/ingestion/src/metadata/data_quality/validations/utils.py +++ b/ingestion/src/metadata/data_quality/validations/utils.py @@ -2,6 +2,8 @@ Data quality validation utility functions. """ +import json +from ast import literal_eval from collections.abc import Callable from typing import Any, TypeVar from urllib.parse import quote @@ -12,6 +14,7 @@ from sqlalchemy.engine import URL from metadata.generated.schema.tests.testCase import TestCaseParameterValue +from metadata.profiler.orm.registry import is_boolean from metadata.utils.logger import test_suite_logger logger = test_suite_logger() @@ -24,6 +27,34 @@ USERNAME_RESERVED_CHARACTERS = ":/?#" +def parse_set_values(raw: str, column_type) -> list: + """Parse an allowed or forbidden set; on a boolean column "true"/"false" strings become booleans + + Only "true"/"false" (any case) are mapped: "1"/"0", "t"/"f" and "yes"/"no" stay as written. + """ + values = _parse_literal(raw) + if not isinstance(values, (list, tuple, set)): + raise ValueError(f"A set of values must be a list, got {raw}") # noqa: TRY004 the set validators abort on ValueError + return [_as_boolean(value) for value in values] if is_boolean(column_type) else list(values) + + +def _parse_literal(raw: str) -> Any: + """A Python literal, or JSON when literal_eval rejects it: JSON writes booleans and null in lowercase + + literal_eval runs first, so a value it read before reads the same. Its ValueError and + SyntaxError fall through to JSON; any other error it raises propagates. + """ + try: + value = literal_eval(raw) + except (ValueError, SyntaxError): + value = json.loads(raw) + return value + + +def _as_boolean(value): + return {"true": True, "false": False}.get(value.lower(), value) if isinstance(value, str) else value + + def get_test_case_param_value( test_case_param_vals: list[TestCaseParameterValue], name: str, diff --git a/ingestion/src/metadata/profiler/orm/registry.py b/ingestion/src/metadata/profiler/orm/registry.py index bddd9a332875..be60810ec3b2 100644 --- a/ingestion/src/metadata/profiler/orm/registry.py +++ b/ingestion/src/metadata/profiler/orm/registry.py @@ -18,7 +18,7 @@ from enum import Enum import sqlalchemy -from sqlalchemy import Date, DateTime, Integer, Numeric, Time +from sqlalchemy import Boolean, Date, DateTime, Integer, Numeric, Time from sqlalchemy.sql.sqltypes import Concatenable, LargeBinary, Text from metadata.generated.schema.entity.data.table import DataType @@ -195,6 +195,13 @@ def is_date_time(_type) -> bool: ) +def is_boolean(_type) -> bool: + """ + Check if _type is a boolean, as a sqlalchemy type or an OpenMetadata DataType + """ + return _type == DataType.BOOLEAN if isinstance(_type, DataType) else isinstance(_type, Boolean) + + def is_quantifiable(_type) -> bool: """ Check if sqlalchemy _type is either integer or numeric diff --git a/ingestion/tests/unit/observability/data_quality/conftest.py b/ingestion/tests/unit/observability/data_quality/conftest.py index 4c7e9721f4ed..ba93dbbc1672 100644 --- a/ingestion/tests/unit/observability/data_quality/conftest.py +++ b/ingestion/tests/unit/observability/data_quality/conftest.py @@ -275,6 +275,32 @@ def test_case_column_value_length_to_be_between_no_min(): ) # type: ignore +@pytest.fixture +def test_case_column_value_length_to_be_between_no_bounds(): + """A lengths window with neither side set lets every row through""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_FNAME, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[], + computePassedFailedRowCount=True, + ) # type: ignore + + +@pytest.fixture +def test_case_column_values_to_be_between_no_bounds(): + """A values window with neither side set lets every row through""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_AGE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[], + computePassedFailedRowCount=True, + ) # type: ignore + + @pytest.fixture def test_case_column_value_max_to_be_between(): """Test case for test column_value_max_to_be_between""" @@ -667,6 +693,37 @@ def test_case_table_custom_sql_query_with_threshold_success(): ) # type: ignore +@pytest.fixture +def test_case_table_custom_sql_query_lowercase_strategy(): + """`strategy` in any case, with spaces around it, reads as COUNT""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_USER, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="sqlExpression", value="SELECT COUNT(*) FROM users WHERE age > 30"), + TestCaseParameterValue(name="strategy", value=" count "), + TestCaseParameterValue(name="threshold", value="50"), + ], + ) # type: ignore + + +@pytest.fixture +def test_case_table_custom_sql_query_blank_strategy(): + """A blank `strategy` reads as ROWS, the definition's default""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_NAME, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="sqlExpression", value="SELECT * FROM users WHERE age > 20"), + TestCaseParameterValue(name="strategy", value=" "), + ], + ) # type: ignore + + @pytest.fixture def test_case_table_custom_sql_unsafe_query_aborted(): """Test case for test column_value_median_to_be_between""" @@ -866,6 +923,65 @@ def test_case_column_value_in_set_boolean(): ) +@pytest.fixture +def test_case_column_values_not_in_set_boolean_strings(): + """The forbidden set as the UI writes it, and a generated test can: a JSON array of strings""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="forbiddenValues", value='["true"]'), + ], + computePassedFailedRowCount=True, + ) + + +@pytest.fixture +def test_case_column_values_not_in_set_boolean_json(): + """The forbidden set as a generated test can write it: a JSON array with a lowercase boolean""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="forbiddenValues", value="[false]"), + ], + computePassedFailedRowCount=True, + ) + + +@pytest.fixture +def test_case_column_value_in_set_missing_column(): + """A set test on a column the table no longer has""" + return TestCase( + name=TEST_CASE_NAME, + entityLink="<#E::table::service.db.users::columns::dropped_column>", + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="allowedValues", value='["John"]'), + ], + ) + + +@pytest.fixture +def test_case_column_value_in_set_boolean_strings(): + """The allowed set as the UI writes it, and a generated test can: a JSON array of strings""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="allowedValues", value='["true", "false"]'), + ], + computePassedFailedRowCount=True, + ) + + @pytest.fixture def test_case_column_values_to_be_in_set_dimensional_match_enum(): return TestCase( diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py index 4e187db2af83..2c404f8c50c9 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py @@ -42,6 +42,11 @@ "tableCustomSQLQuery", } +ABORTED_BEFORE_ROW_COUNT = { + "test_case_table_custom_sql_unsafe_query_aborted", + "test_case_column_value_in_set_missing_column", +} + # pylint: disable=line-too-long @pytest.mark.parametrize( @@ -109,6 +114,13 @@ (TestCaseResult, None, None, TestCaseStatus.Success, 80.0, 0.0, 100.0, 0.0), None, ), + ( + "test_case_column_value_length_to_be_between_no_bounds", + "columnValueLengthsToBeBetween", + "COLUMN", + (TestCaseResult, None, None, TestCaseStatus.Success, 80.0, 0.0, 100.0, 0.0), + None, + ), ( "test_case_column_value_max_to_be_between", "columnValueMaxToBeBetween", @@ -290,6 +302,45 @@ ), None, ), + ( + "test_case_column_value_in_set_missing_column", + "columnValuesToBeInSet", + "COLUMN", + (TestCaseResult, None, None, TestCaseStatus.Aborted, None, None, None, None), + None, + ), + ( + "test_case_column_values_not_in_set_boolean_strings", + "columnValuesToBeNotInSet", + "COLUMN", + ( + TestCaseResult, + "40", + None, + TestCaseStatus.Failed, + 40.0, + 40.0, + 50.0, + 50.0, + ), + None, + ), + ( + "test_case_column_values_not_in_set_boolean_json", + "columnValuesToBeNotInSet", + "COLUMN", + ( + TestCaseResult, + "30", + None, + TestCaseStatus.Failed, + 50.0, + 30.0, + 62.5, + 37.5, + ), + None, + ), ( "test_case_column_sum_to_be_between", "columnValuesSumToBeBetween", @@ -322,6 +373,13 @@ ), None, ), + ( + "test_case_column_values_to_be_between_no_bounds", + "columnValuesToBeBetween", + "COLUMN", + (TestCaseResult, "30", None, TestCaseStatus.Success, 80, 0, 100, 0), + None, + ), ( "test_case_column_values_to_be_not_null", "columnValuesToBeNotNull", @@ -488,6 +546,20 @@ ), None, ), + ( + "test_case_table_custom_sql_query_lowercase_strategy", + "tableCustomSQLQuery", + "TABLE", + (TestCaseResult, "30", None, TestCaseStatus.Success, None, None, None, None), + None, + ), + ( + "test_case_table_custom_sql_query_blank_strategy", + "tableCustomSQLQuery", + "TABLE", + (TestCaseResult, "60", None, TestCaseStatus.Failed, None, None, None, None), + None, + ), ( "test_case_table_custom_sql_unsafe_query_aborted", "tableCustomSQLQuery", @@ -536,6 +608,22 @@ ), None, ), + ( + "test_case_column_value_in_set_boolean_strings", + "columnValuesToBeInSet", + "COLUMN", + ( + TestCaseResult, + "70", + None, + TestCaseStatus.Success, + 70.0, + 10.0, + 87.5, + 12.5, + ), + None, + ), ( "test_case_column_values_to_be_in_set_dimensional_match_enum", "columnValuesToBeInSet", @@ -1114,10 +1202,7 @@ def test_suite_validation_database( # noqa: C901 if failed_percentage: assert round(res.failedRowsPercentage, 2) == failed_percentage assert res.testCaseStatus == status - if ( - test_case_type in TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED - and test_case_name != "test_case_table_custom_sql_unsafe_query_aborted" - ): + if test_case_type in TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED and test_case_name not in ABORTED_BEFORE_ROW_COUNT: assert res.failedRows is not None assert res.failedRowsPercentage is not None assert res.passedRows is not None diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py index 2936ba695a24..5e2c10bc56c4 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py @@ -16,6 +16,7 @@ """ from datetime import datetime, timedelta +from uuid import uuid4 import pytest from freezegun import freeze_time @@ -23,6 +24,8 @@ from metadata.data_quality.validations.base_test_handler import PandasRunner from metadata.generated.schema.tests.basic import TestCaseResult, TestCaseStatus +from metadata.generated.schema.tests.testCase import TestCase, TestCaseParameterValue +from metadata.generated.schema.type.entityReference import EntityReference from metadata.utils.importer import import_test_case_class TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED = { @@ -1294,3 +1297,47 @@ def test_suite_validation_datalake( assert dim.failedRowsPercentage == expected_dim[5] assert dim.impactScore == expected_dim[6] + + +@pytest.mark.parametrize( + "test_case_type,parameter_values,status,failed_rows", + [ + ( + "columnValuesToBeInSet", + [ + TestCaseParameterValue(name="allowedValues", value='["True", "False"]'), + TestCaseParameterValue(name="matchEnum", value="true"), + ], + TestCaseStatus.Success, + 0, + ), + ( + "columnValuesToBeNotInSet", + [TestCaseParameterValue(name="forbiddenValues", value='["False"]')], + TestCaseStatus.Failed, + 1, + ), + ], +) +def test_string_booleans_on_a_datalake_column_stay_strings(test_case_type, parameter_values, status, failed_rows): + """GenericDataFrameColumnParser types a column of "True"/"False" strings as BOOLEAN; its sets must still compare as strings""" + test_case = TestCase( + name="string_booleans", + entityLink="<#E::table::service.db.users::columns::flag>", + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=parameter_values, + computePassedFailedRowCount=True, + ) # type: ignore + validator = import_test_case_class( + "COLUMN", "pandas", test_case_type, f"{test_case_type[0].upper()}{test_case_type[1:]}Validator" + )( + PandasRunner(dataset=lambda: iter((DataFrame({"flag": ["True", "False", "True", "True"]}),)), raw_dataset=None), + test_case=test_case, + execution_date=EXECUTION_DATE.timestamp(), + ) + + res = validator.run_validation() + + assert res.testCaseStatus == status + assert res.failedRows == failed_rows diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py b/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py index 8c799e4348c5..f351ba7236db 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py @@ -360,6 +360,43 @@ def test_an_unset_bound_excludes_nothing(pandas_runner, min_bound, max_bound, vi assert counted == violations +@pytest.mark.parametrize( + "validator_cls, entity_link", + [ + (SQAValuesValidator, ENTITY_LINK_VALUE), + (SQALengthsValidator, ENTITY_LINK_LABEL), + ], +) +def test_sqa_an_unbounded_window_fails_no_row(sqa_runner, validator_cls, entity_link): + """With neither bound set nothing is outside the window, so no row may be reported failed""" + validator = validator_cls(sqa_runner, _test_case(entity_link, [], None, None), EXECUTION_DATE.timestamp()) + + assert validator.compute_row_count(validator.get_column(), float("-inf"), float("inf")) == (EXPECTED_ROWS, 0) + + +def test_sqa_one_open_side_counts_only_the_other(sqa_runner): + """Only the lower bound set: the rows below it fail, and no upper side is invented""" + validator = SQAValuesValidator( + sqa_runner, _test_case(ENTITY_LINK_VALUE, [], None, None), EXECUTION_DATE.timestamp() + ) + + assert validator.compute_row_count(validator.get_column(), MIN_BOUND, float("inf")) == (EXPECTED_ROWS, 1) + + +@pytest.mark.parametrize( + "validator_cls, entity_link", + [ + (SQAValuesValidator, ENTITY_LINK_VALUE), + (SQALengthsValidator, ENTITY_LINK_LABEL), + ], +) +def test_sqa_an_unbounded_window_samples_no_filter(sqa_runner, validator_cls, entity_link): + """The failed-row sample is drawn with the same window, so an open window filters nothing""" + validator = validator_cls(sqa_runner, _test_case(entity_link, [], None, None), EXECUTION_DATE.timestamp()) + + assert validator.filter()["filters"] == [] + + def test_the_window_is_never_widened_by_the_threshold(sqa_runner): """The tolerance is spent on rows here, so spending it on the bounds too would double it""" validator = SQAValuesValidator(sqa_runner, values_test_case(threshold=5), EXECUTION_DATE.timestamp()) diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py b/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py index a1c49f7f6dee..37795fd9cc46 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py @@ -18,7 +18,7 @@ from uuid import uuid4 import pytest -from sqlalchemy import event +from sqlalchemy import Column, Integer, MetaData, String, Table, event from sqlalchemy.exc import SQLAlchemyError from metadata.data_quality.validations.base_test_handler import FailureThreshold, ThresholdUnit @@ -64,10 +64,14 @@ def build_test_case(parameter_values, compute_passed_failed_row_count=False, ent def build_validator(validator_class, parameter_values, compute_passed_failed_row_count=False): - """Build a validator whose only live dependency is its test case""" - return validator_class( - MagicMock(), build_test_case(parameter_values, compute_passed_failed_row_count), EXECUTION_DATE - ) + """Build a validator whose only live dependencies are its test case and the declared column + + A set validator parses its values in the column's declared type, so the runner declares the + table; nothing is ever queried. + """ + runner = MagicMock() + runner.dataset = Table("users", MetaData(), Column("nickname", String), Column("age", Integer)) + return validator_class(runner, build_test_case(parameter_values, compute_passed_failed_row_count), EXECUTION_DATE) @contextmanager diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py index 4b4ce41a4248..1a88c9074302 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py @@ -1,7 +1,16 @@ +from datetime import datetime + import pytest +import sqlalchemy as sqa +from metadata.data_quality.validations import utils +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.utils import get_bool_test_case_param +from metadata.generated.schema.entity.data.table import DataType from metadata.generated.schema.tests.testCase import TestCaseParameterValue +from metadata.profiler.orm.registry import is_boolean @pytest.mark.parametrize( @@ -20,3 +29,62 @@ ) def test_get_bool_test_case_param(test_case_param_vals, name, expected): assert get_bool_test_case_param(test_case_param_vals, name) == expected + + +@pytest.mark.parametrize( + "bound, unbounded", + [ + (None, True), + (float("-inf"), True), + (float("inf"), True), + (3, False), + (3.5, False), + (datetime(2021, 7, 1), False), + ], +) +def test_is_unbounded(bound, unbounded): + assert is_unbounded(bound) is unbounded + + +@pytest.mark.parametrize("column_type", [sqa.Boolean(), DataType.BOOLEAN]) +def test_true_and_false_strings_become_booleans_on_a_boolean_column(column_type): + assert utils.parse_set_values(raw='["true", "False"]', column_type=column_type) == [True, False] + + +@pytest.mark.parametrize("column_type", [sqa.Boolean(), DataType.BOOLEAN]) +def test_python_literals_on_a_boolean_column_are_kept(column_type): + assert utils.parse_set_values(raw="[True, False]", column_type=column_type) == [True, False] + + +@pytest.mark.parametrize("column_type", [sqa.String(), DataType.VARCHAR, None]) +def test_strings_on_a_string_or_unknown_column_stay_strings(column_type): + assert utils.parse_set_values(raw='["true", "false"]', column_type=column_type) == ["true", "false"] + + +@pytest.mark.parametrize( + "raw, column_type, expected", + [ + ("[false]", sqa.Boolean(), [False]), + ("[true, false]", DataType.BOOLEAN, [True, False]), + ("[null]", sqa.String(), [None]), + ('["a", 1.5]', None, ["a", 1.5]), + ], +) +def test_a_json_written_set_is_read_like_a_python_one(raw, column_type, expected): + """The UI and generated tests write JSON, where booleans and null are lowercase""" + assert utils.parse_set_values(raw=raw, column_type=column_type) == expected + + +def test_a_set_that_is_neither_python_nor_json_is_rejected(): + with pytest.raises(ValueError): + utils.parse_set_values(raw="[OPEN, CLOSED]", column_type=sqa.String()) + + +def test_a_set_that_is_not_a_collection_is_rejected(): + with pytest.raises(ValueError, match="must be a list"): + utils.parse_set_values(raw='"true"', column_type=sqa.Boolean()) + + +def test_is_boolean_reads_both_type_systems(): + assert is_boolean(sqa.Boolean()) and is_boolean(DataType.BOOLEAN) + assert not is_boolean(sqa.Integer()) and not is_boolean(DataType.INT) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java index ec80fb17bf6e..661c3bc4960a 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java @@ -65,7 +65,6 @@ import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.ForbiddenException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; @@ -580,13 +579,8 @@ void test_incidents_restrictedUserSeesOnlyOwnDomain(TestNamespace ns) throws Exc Table ownTable = createTable(admin, p + "_own", schema, ownDomain, cleanup); Table foreignTable = createTable(admin, p + "_foreign", schema, foreignDomain, cleanup); - String testDefinitionFqn = - admin - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // Lists INT among its supported types, so it fits the INT `id` column the incidents are on. + String testDefinitionFqn = "columnValuesToBeNotNull"; String ownIncidentFqn = createIncident(admin, ownTable, p + "_own_tc", testDefinitionFqn); String foreignIncidentFqn = createIncident(admin, foreignTable, p + "_foreign_tc", testDefinitionFqn); diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java index 90c7d2f9ac79..78ed728c1e93 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java @@ -80,13 +80,9 @@ public void setup() throws Exception { Table table = createTestTable(); tableFqn = table.getFullyQualifiedName(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; for (int i = 0; i < TEST_DATA_SIZE; i++) { TestCase testCase = createTestCase(table, i, testDefFqn); diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java index 57ad6bcc39a4..d9ec78744054 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java @@ -55,7 +55,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.fluent.AIApplications; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; import org.openmetadata.service.Entity; @@ -334,18 +333,13 @@ private Table createTable(TestNamespace ns, String prefix) throws Exception { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. CreateTestCase createTestCase = new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn); + .withTestDefinition("columnValuesToBeNotNull"); return client.testCases().create(createTestCase); } diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java index 6dd07f1a985b..844b64fedb69 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java @@ -49,7 +49,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.OpenMetadataException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; import org.openmetadata.service.Entity; @@ -338,18 +337,13 @@ private int countRowsById(String table, String id) { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. CreateTestCase createTestCase = new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn); + .withTestDefinition("columnValuesToBeNotNull"); return client.testCases().create(createTestCase); } diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java index 41af409a5f21..9b3c92cb01be 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java @@ -53,7 +53,6 @@ import org.openmetadata.schema.type.TestDefinitionEntityType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.OpenMetadataException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.service.Entity; import org.openmetadata.service.jdbi3.TestCaseResultRepository; import org.openmetadata.service.util.FullyQualifiedName; @@ -294,20 +293,15 @@ private DatabaseService createDatabaseService(TestNamespace ns) { private TestCase createSystemTestCase(Table table, String name) { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. return client .testCases() .create( new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn)); + .withTestDefinition("columnValuesToBeNotNull")); } private TestCase createCustomTestCase(Table table, String name, String testDefinitionFqn) { diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java index 040299817fa9..237473d000d6 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java @@ -41,7 +41,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.schema.type.Relationship; import org.openmetadata.sdk.client.OpenMetadataClient; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.service.Entity; import org.openmetadata.service.util.FullyQualifiedName; import org.openmetadata.service.util.OrphanTestCaseRelationshipCleanup; @@ -141,20 +140,15 @@ private Table createTable(TestNamespace ns) throws Exception { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. return client .testCases() .create( new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn)); + .withTestDefinition("columnValuesToBeNotNull")); } private void seedResult(String testCaseFqn) { diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java index dfc527a207b1..5231ef747f0b 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java @@ -106,13 +106,9 @@ void listingApiSurvivesLegacyTestSuitesFieldOnIncidentDoc() throws Exception { List.of( new Column().withName("id").withDataType(ColumnDataType.BIGINT)))); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; TestCase testCase = client diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java index 168212196ed3..d84c2660b20b 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java @@ -894,6 +894,132 @@ void post_testWithInvalidParameterValue_4xx(TestNamespace ns) { Exception.class, () -> createEntity(request), "Should fail with invalid parameter name"); } + @Test + void post_testWithUnofferedOptionValue_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("bad_strategy")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); + request.setTestDefinition("tableCustomSQLQuery"); + request.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + Exception error = assertThrows(Exception.class, () -> createEntity(request)); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + } + + @Test + void post_testOnUnsupportedColumnType_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("length_on_number")); + // createTable's `id` column is INT, which columnValueLengthsToBeBetween does not support. + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + Exception error = assertThrows(Exception.class, () -> createEntity(request)); + assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); + } + + @Test + void post_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("post_no_suite")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + assertThrows(Exception.class, () -> createEntity(request)); + assertNull(tableTestSuite(table)); + } + + private static EntityReference tableTestSuite(Table table) { + return SdkClients.adminClient() + .tables() + .get(table.getId().toString(), "testSuite") + .getTestSuite(); + } + + @Test + void patch_toUnofferedOptionValue_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("patch_bad_strategy")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); + request.setTestDefinition("tableCustomSQLQuery"); + request.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("ROWS"))); + TestCase testCase = createEntity(request); + + testCase.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + Exception error = + assertThrows(Exception.class, () -> patchEntity(testCase.getId().toString(), testCase)); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + } + + @Test + void createMany_withOneUnofferedOptionValue_createsNone(TestNamespace ns) { + Table table = createTable(ns); + String tableLink = "<#E::table::" + table.getFullyQualifiedName() + ">"; + CreateTestCase valid = new CreateTestCase(); + valid.setName(ns.prefix("many_valid")); + valid.setEntityLink(tableLink); + valid.setTestDefinition("tableRowCountToEqual"); + valid.setParameterValues( + List.of(new TestCaseParameterValue().withName("value").withValue("100"))); + CreateTestCase invalid = new CreateTestCase(); + invalid.setName(ns.prefix("many_bad_strategy")); + invalid.setEntityLink(tableLink); + invalid.setTestDefinition("tableCustomSQLQuery"); + invalid.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + OpenMetadataClient client = SdkClients.adminClient(); + Exception error = + assertThrows(Exception.class, () -> client.testCases().createMany(List.of(valid, invalid))); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + // The batch wrote nothing: the valid test can still be created on its own. + assertEquals(valid.getName(), createEntity(valid).getName()); + } + + @Test + void patch_keepsATestOnARetypedColumnEditable(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("patch_gate")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::name>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + TestCase testCase = createEntity(request); + + // The stored test now targets a column type its definition does not support, as a row created + // before the create-time check, or one whose column was retyped by ingestion, does. + table.getColumns().stream() + .filter(column -> column.getName().equals("name")) + .forEach(column -> column.withDataType(ColumnDataType.INT).withDataLength(null)); + SdkClients.adminClient().tables().update(table.getId().toString(), table); + + testCase.setDescription("edited"); + TestCase updated = patchEntity(testCase.getId().toString(), testCase); + + assertEquals("edited", updated.getDescription()); + } + @Test void test_testCaseWithOwner(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java index 2674f2c2bc8c..90c93f7d0839 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java @@ -102,13 +102,9 @@ void softDeletingTestCaseDoesNotPollutePropagatedTimeSeriesDocs() throws Excepti List.of( new Column().withName("id").withDataType(ColumnDataType.BIGINT)))); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; testCase = client diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java new file mode 100644 index 000000000000..7e8b1e9797c5 --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java @@ -0,0 +1,132 @@ +package org.openmetadata.service.jdbi3; + +import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.UUID; +import java.util.function.Supplier; +import java.util.stream.Collectors; +import org.openmetadata.schema.entity.data.Table; +import org.openmetadata.schema.tests.TestCase; +import org.openmetadata.schema.tests.TestCaseParameter; +import org.openmetadata.schema.tests.TestCaseParameterValue; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.type.Column; +import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; + +/** + * What a new test case must agree with in its definition + * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). + */ +final class TestCaseDefinitionConformance { + private TestCaseDefinitionConformance() {} + + static void validate( + TestCase test, TestDefinition definition, Table table, EntityLink entityLink) { + validateOptionValues( + listOrEmpty(test.getParameterValues()), listOrEmpty(definition.getParameterDefinition())); + validateColumnType(definition, table, entityLink); + } + + /** + * Checks only what an update changes, so a row stored before these rules, or on a column + * retyped since, stays editable. A test moved to another column or definition is checked in + * full. + */ + static void validateUpdate( + TestCase original, + TestCase updated, + Supplier definition, + Supplier table) { + List changed = changedValues(original, updated); + if (isRetargeted(original, updated)) { + validate(updated, definition.get(), table.get(), EntityLink.parse(updated.getEntityLink())); + } else if (!changed.isEmpty()) { + validateOptionValues(changed, listOrEmpty(definition.get().getParameterDefinition())); + } + } + + private static boolean isRetargeted(TestCase original, TestCase updated) { + return !Objects.equals(original.getEntityLink(), updated.getEntityLink()) + || !Objects.equals(definitionId(original), definitionId(updated)); + } + + private static UUID definitionId(TestCase test) { + return Optional.ofNullable(test.getTestDefinition()).map(EntityReference::getId).orElse(null); + } + + private static List changedValues(TestCase original, TestCase updated) { + List stored = listOrEmpty(original.getParameterValues()); + return listOrEmpty(updated.getParameterValues()).stream() + .filter(value -> !stored.contains(value)) + .toList(); + } + + private static void validateOptionValues( + List values, List parameters) { + Map> options = optionsByParameter(parameters); + values.stream() + .filter(value -> options.containsKey(value.getName()) && !isUnset(value.getValue())) + .filter(value -> !isOption(value.getValue(), options.get(value.getName()))) + .findFirst() + .ifPresent( + value -> { + throw new IllegalArgumentException( + String.format( + "Parameter '%s' must be one of %s, but was '%s'.", + value.getName(), options.get(value.getName()), value.getValue())); + }); + } + + private static Map> optionsByParameter(List parameters) { + return parameters.stream() + .filter(parameter -> !nullOrEmpty(parameter.getOptionValues())) + .collect( + Collectors.toMap( + TestCaseParameter::getName, + TestCaseParameter::getOptionValues, + (first, duplicate) -> first)); + } + + /** A blank value is unset, as the test generator reads it, so it is not checked here. */ + private static boolean isUnset(String value) { + return nullOrEmpty(value) || value.isBlank(); + } + + // Case-insensitive, as TestCaseThresholdValidator already reads thresholdUnit. + private static boolean isOption(String value, List options) { + return options.stream().anyMatch(option -> String.valueOf(option).equalsIgnoreCase(value)); + } + + private static void validateColumnType( + TestDefinition definition, Table table, EntityLink entityLink) { + List supported = listOrEmpty(definition.getSupportedDataTypes()); + Optional type = + column(table, entityLink.getArrayFieldName()).map(Column::getDataType); + boolean unsupported = + !supported.isEmpty() && type.isPresent() && !supported.contains(type.get()); + if (unsupported) { + throw new IllegalArgumentException( + String.format( + "Test definition '%s' does not support column type %s. Supported types are: %s", + definition.getName(), type.get(), supported)); + } + } + + // validateColumnTestCase matches the column name case-insensitively; this must find the same one. + private static Optional column(Table table, String name) { + return Optional.ofNullable(name) + .flatMap( + columnName -> + listOrEmpty(table.getColumns()).stream() + .filter(Objects::nonNull) + .filter(candidate -> candidate.getName().equalsIgnoreCase(columnName)) + .findFirst()); + } +} 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 8a714f862771..1c3e10df2461 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 @@ -901,6 +901,10 @@ public void prepare(TestCase test, boolean update) { validateDimensionColumns(test, table); setDataQualityDimension(test, testDefinition); + if (!update) { + TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); + } + // Create/resolve the basic test suite only after all validations pass. // This avoids creating side entities when request validation fails early. var testSuite = getOrCreateTestSuite(test, table); @@ -2001,6 +2005,14 @@ private void updateDataQualityDimension() { } } + private void validateChangedConformance() { + TestCaseDefinitionConformance.validateUpdate( + original, + updated, + () -> Entity.getEntity(updated.getTestDefinition(), "", ALL), + () -> Entity.getEntity(EntityLink.parse(updated.getEntityLink()), "columns", ALL)); + } + @Override protected boolean consolidateChanges(TestCase original, TestCase updated, Operation operation) { return false; @@ -2009,6 +2021,7 @@ protected boolean consolidateChanges(TestCase original, TestCase updated, Operat @Transaction @Override public void entitySpecificUpdate(boolean consolidatingChanges) { + validateChangedConformance(); compareAndUpdate( "entity", () -> { diff --git a/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json b/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json index 93dab7e88294..05a7e9e0ad27 100644 --- a/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json +++ b/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json @@ -16,7 +16,7 @@ { "name": "strategy", "displayName": "Strategy", - "description": "Strategy to use to run the custom SQL query (i.e. `SELECT COUNT()` or `SELECT (defaults to ROWS)", + "description": "How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.", "dataType": "ARRAY", "optionValues": ["ROWS", "COUNT"], "required": false diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java new file mode 100644 index 000000000000..aa36b0da6bc0 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java @@ -0,0 +1,27 @@ +package org.openmetadata.service.jdbi3; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; +import org.openmetadata.common.utils.CommonUtil; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.utils.JsonUtils; + +/** The test definitions as they are seeded, read straight off the classpath. */ +final class SeededTestDefinitions { + private SeededTestDefinitions() {} + + static List all() throws IOException { + List definitions = new ArrayList<>(); + for (String resource : + CommonUtil.getResources(Pattern.compile(".*json/data/tests/.*\\.json$"))) { + definitions.add( + JsonUtils.readValue( + CommonUtil.getResourceAsStream( + SeededTestDefinitions.class.getClassLoader(), resource), + TestDefinition.class)); + } + return definitions; + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java new file mode 100644 index 000000000000..4dfe4ac1c5b5 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java @@ -0,0 +1,259 @@ +package org.openmetadata.service.jdbi3; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.List; +import java.util.UUID; +import org.junit.jupiter.api.Test; +import org.openmetadata.schema.entity.data.Table; +import org.openmetadata.schema.tests.TestCase; +import org.openmetadata.schema.tests.TestCaseParameter; +import org.openmetadata.schema.tests.TestCaseParameterValue; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.type.Column; +import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.schema.type.TestDefinitionEntityType; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; + +class TestCaseDefinitionConformanceTest { + + private static final Table TABLE = + new Table() + .withColumns( + List.of( + new Column().withName("Amount").withDataType(ColumnDataType.NUMERIC), + new Column().withName("note").withDataType(ColumnDataType.VARCHAR), + new Column().withName("untyped"))); + + private static final TestDefinition CUSTOM_SQL = + new TestDefinition() + .withName("tableCustomSQLQuery") + .withEntityType(TestDefinitionEntityType.TABLE) + .withParameterDefinition( + List.of( + new TestCaseParameter() + .withName("strategy") + .withOptionValues(List.of("ROWS", "COUNT")))); + + private static final TestDefinition LENGTHS = + new TestDefinition() + .withName("columnValueLengthsToBeBetween") + .withEntityType(TestDefinitionEntityType.COLUMN) + .withSupportedDataTypes(List.of(ColumnDataType.VARCHAR, ColumnDataType.STRING)) + .withSupportsDynamicAssertion(true); + + private static EntityLink table() { + return EntityLink.parse("<#E::table::svc.db.sch.orders>"); + } + + private static EntityLink column(String name) { + return EntityLink.parse("<#E::table::svc.db.sch.orders::columns::" + name + ">"); + } + + private static TestCase withParameter(String name, String value) { + return new TestCase() + .withParameterValues(List.of(new TestCaseParameterValue().withName(name).withValue(value))); + } + + @Test + void anOptionValueIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "COUNT"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void anOptionValueInAnotherCaseIsAcceptedAsThresholdUnitAlreadyIs() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "count"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aValueThatIsNoneOfTheOptionsIsRejected() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "[\"ROWS\"]"), CUSTOM_SQL, TABLE, table())); + assertTrue(error.getMessage().contains("must be one of [ROWS, COUNT]"), error.getMessage()); + } + + @Test + void aBlankOptionalValueIsTreatedAsUnset() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", ""), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aWhitespaceValueIsTreatedAsUnsetAsTheGeneratorReadsIt() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", " "), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aParameterTheDefinitionGivesNoOptionsIsNotChecked() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("sqlExpression", "SELECT 1"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aColumnOfAnUnsupportedTypeIsRejectedWhateverTheNameCase() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), LENGTHS, TABLE, column("amount"))); + assertTrue(error.getMessage().contains("NUMERIC"), error.getMessage()); + } + + @Test + void aColumnOfASupportedTypeIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate(new TestCase(), LENGTHS, TABLE, column("note"))); + } + + @Test + void aColumnOfUnknownTypeIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), LENGTHS, TABLE, column("untyped"))); + } + + @Test + void anEmptySupportedTypeListAcceptsEveryColumn() { + TestDefinition anyType = + new TestDefinition() + .withName("columnValuesToBeNotNull") + .withEntityType(TestDefinitionEntityType.COLUMN); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), anyType, TABLE, column("Amount"))); + } + + @Test + void aTableTestIsNotCheckedAgainstColumnTypes() { + assertDoesNotThrow( + () -> TestCaseDefinitionConformance.validate(new TestCase(), LENGTHS, TABLE, table())); + } + + @Test + void anUpdateThatChangesAnOptionToOneNotOfferedIsRejected() { + TestCase stored = withParameter("strategy", "ROWS").withEntityLink(table().getLinkString()); + TestCase updated = withParameter("strategy", "SUM").withEntityLink(table().getLinkString()); + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + assertTrue(error.getMessage().contains("must be one of [ROWS, COUNT]"), error.getMessage()); + } + + @Test + void anUpdateKeepsAStoredValueOutsideTheOptionsEditable() { + TestCase stored = withParameter("strategy", "SUM").withEntityLink(table().getLinkString()); + TestCase updated = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withDescription("edited"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + } + + @Test + void anUpdateKeepsATestOnAColumnRetypedSinceEditable() { + TestCase stored = new TestCase().withEntityLink(column("Amount").getLinkString()); + TestCase updated = + new TestCase().withEntityLink(column("Amount").getLinkString()).withDescription("edited"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> LENGTHS, () -> TABLE)); + } + + /** An inspection query is stored by an update that changes no parameter. */ + @Test + void anUpdateThatChangesNoParameterDoesNotLoadTheDefinition() { + TestCase stored = withParameter("strategy", "ROWS").withEntityLink(table().getLinkString()); + TestCase updated = + withParameter("strategy", "ROWS") + .withEntityLink(table().getLinkString()) + .withInspectionQuery("SELECT 1"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, + updated, + () -> { + throw new AssertionError("the definition was loaded"); + }, + () -> TABLE)); + } + + @Test + void anUpdateThatMovesTheTestToAnUnsupportedColumnIsRejected() { + TestCase stored = new TestCase().withEntityLink(column("note").getLinkString()); + TestCase updated = new TestCase().withEntityLink(column("Amount").getLinkString()); + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> LENGTHS, () -> TABLE)); + assertTrue(error.getMessage().contains("NUMERIC"), error.getMessage()); + } + + @Test + void anUpdateThatChangesTheDefinitionChecksEveryValueAgainstIt() { + TestCase stored = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withTestDefinition(new EntityReference().withId(UUID.randomUUID())); + TestCase updated = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withTestDefinition(new EntityReference().withId(UUID.randomUUID())); + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + } + + /** + * ai-platform reads a bound as a parameter with a validationRule; every dynamic seed must mark + * its bounds so. + */ + @Test + void everySeededDynamicDefinitionMarksItsBoundsWithAValidationRule() throws IOException { + for (TestDefinition definition : SeededTestDefinitions.all()) { + if (Boolean.TRUE.equals(definition.getSupportsDynamicAssertion())) { + long bounds = + definition.getParameterDefinition().stream() + .filter(parameter -> parameter.getValidationRule() != null) + .count(); + assertTrue(bounds >= 2, definition.getName() + " has " + bounds + " ruled bounds"); + } + } + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java index e6859905213e..b75cfba7d81e 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java @@ -5,23 +5,19 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; -import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeSet; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; -import org.openmetadata.common.utils.CommonUtil; import org.openmetadata.schema.tests.TestCaseParameter; import org.openmetadata.schema.tests.TestDefinition; import org.openmetadata.schema.type.TestCaseParameterDataType; -import org.openmetadata.schema.utils.JsonUtils; class TestCaseThresholdValidatorTest { @@ -476,20 +472,6 @@ void leavesTableCustomSqlQueryComparisonBoundsAlone() { /** Definitions that declare `thresholdUnit` without their `threshold` being a tolerance. */ private static final Set NOT_A_TOLERANCE = Set.of("tableCustomSQLQuery"); - /** The definitions as they are seeded, read straight off the classpath. */ - private static List seededDefinitions() throws IOException { - List definitions = new ArrayList<>(); - for (String resource : - CommonUtil.getResources(Pattern.compile(".*json/data/tests/.*\\.json$"))) { - definitions.add( - JsonUtils.readValue( - CommonUtil.getResourceAsStream( - TestCaseThresholdValidatorTest.class.getClassLoader(), resource), - TestDefinition.class)); - } - return definitions; - } - /** * The classification is a named list, so a newly seeded definition that opts in to failure * thresholds has to be placed in one of the three buckets rather than silently inheriting @@ -498,7 +480,7 @@ private static List seededDefinitions() throws IOException { @Test void everySeededThresholdDefinitionIsClassified() throws IOException { Set optedIn = - seededDefinitions().stream() + SeededTestDefinitions.all().stream() .filter( definition -> definition.getParameterDefinition().stream() @@ -519,7 +501,7 @@ void everySeededThresholdDefinitionIsClassified() throws IOException { /** The seeded definitions, as shipped, land on the side of the cap this test declares. */ @Test void theSeededDefinitionsAreCappedAccordingToTheirClass() throws IOException { - for (TestDefinition definition : seededDefinitions()) { + for (TestDefinition definition : SeededTestDefinitions.all()) { String name = definition.getName(); // `matchEnum` has its own rule; satisfy it so this test only exercises the cap. Map aboveTheCap = diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts index 08193e47c19c..b064ad163d6d 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts @@ -1764,20 +1764,20 @@ pickEntityMatrix( const testCase1 = await tableEntity.createTestCase(apiContext, { name: `pw_col_dq_success_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 1 }, - { name: 'maxLength', value: 10 }, + { name: 'minValue', value: 1 }, + { name: 'maxValue', value: 10 }, ], }); const testCase2 = await tableEntity.createTestCase(apiContext, { name: `pw_col_dq_failed_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 100 }, - { name: 'maxLength', value: 200 }, + { name: 'minValue', value: 100 }, + { name: 'maxValue', value: 200 }, ], }); @@ -1903,10 +1903,10 @@ pickEntityMatrix( const testCase = await tableEntity.createTestCase(apiContext, { name: `pw_col_incident_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 100 }, - { name: 'maxLength', value: 200 }, + { name: 'minValue', value: 100 }, + { name: 'maxValue', value: 200 }, ], });