From cfd34af2e58a1e5a9b7686e9eaa0fe27192b3ba8 Mon Sep 17 00:00:00 2001 From: iamrajatrana Date: Thu, 20 Aug 2026 02:04:48 +0530 Subject: [PATCH 1/3] fix(validation): reject duplicate mapping keys (#336) Signed-off-by: iamrajatrana --- validation/test_validate.py | 191 ++++++++++++++++++++++++++++++++++++ validation/validate.py | 41 +++++++- 2 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 validation/test_validate.py diff --git a/validation/test_validate.py b/validation/test_validate.py new file mode 100644 index 00000000..fa26f73b --- /dev/null +++ b/validation/test_validate.py @@ -0,0 +1,191 @@ +# /// script +# requires-python = ">=3.11" +# dependencies = [ +# "jsonschema>=4.26.0", +# "pyyaml>=6.0.3", +# "sqlglot>=30.12.0", +# ] +# /// + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +import yaml +from validate import UniqueKeyLoader + + +class UniqueKeyLoaderTest(unittest.TestCase): + def load(self, content: str): + return yaml.load(content, Loader=UniqueKeyLoader) + + def assert_duplicate_key(self, content: str, key: str): + with self.assertRaisesRegex( + yaml.constructor.ConstructorError, + rf"found duplicate key {key!r}", + ): + self.load(content) + + def test_rejects_duplicate_top_level_key(self): + self.assert_duplicate_key( + "version: 0.1.0\nversion: 0.2.0.dev0\n", + "version", + ) + + def test_rejects_duplicate_nested_key(self): + self.assert_duplicate_key( + "dataset:\n name: orders\n source: staging.orders\n source: production.orders\n", + "source", + ) + + def test_rejects_quoted_equivalent_key(self): + self.assert_duplicate_key( + 'name: sales\n"name": finance\n', + "name", + ) + + def test_rejects_explicitly_tagged_equivalent_key(self): + self.assert_duplicate_key( + "name: sales\n!!str name: finance\n", + "name", + ) + + def test_rejects_duplicate_json_object_key(self): + self.assert_duplicate_key( + '{"name": "sales", "name": "finance"}', + "name", + ) + + def test_rejects_duplicate_collection_key(self): + self.assert_duplicate_key( + "datasets:\n - name: orders\ndatasets:\n - name: customers\n", + "datasets", + ) + + def test_rejects_duplicate_that_would_hide_invalid_value(self): + self.assert_duplicate_key( + "source:\nsource: analytics.orders\n", + "source", + ) + + def test_rejects_explicit_duplicate_after_merge(self): + self.assert_duplicate_key( + "dataset:\n" + " <<: &defaults\n" + " source: staging.orders\n" + " source: warehouse.orders\n" + " source: production.orders\n", + "source", + ) + + def test_rejects_repeated_merge_key(self): + self.assert_duplicate_key( + "dataset:\n <<: &first\n source: staging.orders\n <<: &second\n name: orders\n", + "<<", + ) + + def test_allows_same_key_in_separate_mappings(self): + loaded = self.load( + "datasets:\n" + " - name: orders\n" + " source: analytics.orders\n" + " - name: customers\n" + " source: analytics.customers\n" + ) + + self.assertEqual(loaded["datasets"][0]["name"], "orders") + self.assertEqual(loaded["datasets"][1]["name"], "customers") + + def test_allows_aliases(self): + loaded = self.load( + "primary: &source analytics.orders\nbackup: *source\n" + ) + + self.assertEqual(loaded["primary"], "analytics.orders") + self.assertEqual(loaded["backup"], "analytics.orders") + + def test_allows_merge_key_override(self): + loaded = self.load( + "defaults: &defaults\n source: staging.orders\ndataset:\n <<: *defaults\n source: production.orders\n" + ) + + self.assertEqual(loaded["dataset"]["source"], "production.orders") + + def test_distinguishes_merge_key_from_quoted_literal(self): + loaded = self.load( + 'defaults: &defaults\n source: staging.orders\ndataset:\n <<: *defaults\n "<<": literal\n' + ) + + self.assertEqual(loaded["dataset"]["source"], "staging.orders") + self.assertEqual(loaded["dataset"]["<<"], "literal") + + def test_duplicate_error_reports_both_locations(self): + with self.assertRaises(yaml.constructor.ConstructorError) as caught: + self.load("name: sales\nname: finance\n") + + error = caught.exception + self.assertEqual(error.context_mark.line, 0) + self.assertEqual(error.problem_mark.line, 1) + + +class ValidatorIntegrationTest(unittest.TestCase): + def run_validator(self, content: str) -> subprocess.CompletedProcess[str]: + with tempfile.TemporaryDirectory() as temp_dir: + model_path = Path(temp_dir) / "model.yaml" + model_path.write_text(content) + return subprocess.run( + [sys.executable, Path(__file__).with_name("validate.py"), model_path], + check=False, + capture_output=True, + text=True, + ) + + def test_duplicate_key_exits_nonzero(self): + result = self.run_validator( + "version: 0.2.0.dev0\n" + "semantic_model:\n" + " - name: sales\n" + " name: finance\n" + " datasets:\n" + " - name: orders\n" + " source: analytics.orders\n" + ) + + self.assertEqual(result.returncode, 1) + self.assertIn("Error: Invalid YAML", result.stdout) + self.assertIn("found duplicate key 'name'", result.stdout) + + def test_valid_model_still_passes(self): + result = self.run_validator( + "version: 0.2.0.dev0\n" + "semantic_model:\n" + " - name: sales\n" + " datasets:\n" + " - name: orders\n" + " source: analytics.orders\n" + ) + + self.assertEqual(result.returncode, 0) + self.assertIn("Validation PASSED", result.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/validation/validate.py b/validation/validate.py index 258d34f1..d4b09645 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -43,11 +43,13 @@ import json import sys +from collections.abc import Hashable from pathlib import Path try: import yaml from jsonschema import Draft202012Validator + from yaml.constructor import ConstructorError except ImportError: print("Missing dependencies. Install with:") print(" pip install pyyaml jsonschema") @@ -75,6 +77,43 @@ SKIP_SQL_VALIDATION = {"MDX", "TABLEAU", "MAQL"} +class UniqueKeyLoader(yaml.SafeLoader): + """Safe YAML loader that rejects duplicate explicit mapping keys.""" + + def construct_mapping(self, node: yaml.MappingNode, deep: bool = False) -> dict: + seen = set() + merge_tag = "tag:yaml.org,2002:merge" + merge_key = object() + + for key_node, _ in node.value: + if key_node.tag == merge_tag: + key = merge_key + display_key = "<<" + else: + key = self.construct_object(key_node, deep=deep) + display_key = key + + if not isinstance(key, Hashable): + raise ConstructorError( + "while constructing a mapping", + node.start_mark, + "found an unhashable key", + key_node.start_mark, + ) + + if key in seen: + raise ConstructorError( + "while constructing a mapping", + node.start_mark, + f"found duplicate key {display_key!r}", + key_node.start_mark, + ) + seen.add(key) + + # Delegate construction (including merge-key flattening) to SafeLoader. + return super().construct_mapping(node, deep=deep) + + def validate_schema(data: dict, schema: dict) -> list[str]: """Validate against JSON Schema.""" validator = Draft202012Validator(schema) @@ -249,7 +288,7 @@ def main(): with open(yaml_path) as f: try: - data = yaml.safe_load(f) + data = yaml.load(f, Loader=UniqueKeyLoader) except yaml.YAMLError as e: print(f"Error: Invalid YAML: {e}") sys.exit(1) From 070fece86f9dc6d03f07cfa402ac757ae61482af Mon Sep 17 00:00:00 2001 From: iamrajatrana Date: Thu, 20 Aug 2026 02:16:28 +0530 Subject: [PATCH 2/3] ci(validation): run validator tests (#336) Signed-off-by: iamrajatrana --- .github/workflows/validation-ci.yml | 63 +++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 .github/workflows/validation-ci.yml diff --git a/.github/workflows/validation-ci.yml b/.github/workflows/validation-ci.yml new file mode 100644 index 00000000..c8df26ae --- /dev/null +++ b/.github/workflows/validation-ci.yml @@ -0,0 +1,63 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +name: Validation CI + +on: + push: + branches: ["main"] + paths: + - "validation/**" + - "core-spec/**" + - "examples/tpcds_semantic_model.yaml" + - ".github/workflows/validation-ci.yml" + pull_request: + branches: ["main"] + paths: + - "validation/**" + - "core-spec/**" + - "examples/tpcds_semantic_model.yaml" + - ".github/workflows/validation-ci.yml" + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + python-version: ["3.11", "3.12", "3.13", "3.14"] + + steps: + - name: Checkout project + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: ${{ matrix.python-version }} + + - name: Install uv + run: | + curl -LsSf https://astral.sh/uv/install.sh | sh + echo "${HOME}/.local/bin" >> "${GITHUB_PATH}" + + - name: Run validator tests + run: uv run validation/test_validate.py + + - name: Validate canonical example + run: uv run validation/validate.py examples/tpcds_semantic_model.yaml From 97b4a2e28ed261119dc06f250d018c9c5e36889d Mon Sep 17 00:00:00 2001 From: iamrajatrana Date: Tue, 25 Aug 2026 09:37:42 +0530 Subject: [PATCH 3/3] fix(validation): check duplicate keys before merge flattening (#336) The duplicate-key check ran inside construct_mapping, which SafeConstructor invokes only after flatten_mapping has already rewritten the node graph. That produced two defects reported in review of #337: - False negative: flatten_mapping recurses into a mapping used directly as a "<<" merge source without constructing it as a mapping, so duplicates there were never seen. "<<: {name: orders, name: customers}" silently reduced to "name: customers" and validation passed. - False positive: flatten_mapping mutates node.value in place. When an alias reused a mapping that had already been merged, the check scanned the mutated node and reported a legal merge override as a duplicate, rejecting a document that stock SafeLoader accepts. Replace the construct_mapping override with a construct_document pre-pass that walks the composed node graph before any constructor runs, guarding on node identity so aliases and recursive anchors are visited exactly once. Only scalar key nodes are constructed; collection keys are rejected directly rather than constructed, which would run flatten_mapping on the graph the walk must not disturb. Error type, message, and both source marks are unchanged, as is merge-key support and rejection of a repeated explicit "<<". Add 18 regression tests: both reported cases, anchored merge sources, construction-order independence, sequence-form merges, recursive mapping and sequence aliases, unhashable keys with their marks, scalar-key equivalence (1/01, yes/true, "1" versus 1), and multi-document loading. Co-Authored-By: Claude Opus 5 --- validation/test_validate.py | 149 +++++++++++++++++++++++++++++++++++- validation/validate.py | 87 +++++++++++++-------- 2 files changed, 203 insertions(+), 33 deletions(-) diff --git a/validation/test_validate.py b/validation/test_validate.py index fa26f73b..339396a4 100644 --- a/validation/test_validate.py +++ b/validation/test_validate.py @@ -27,6 +27,7 @@ import sys import tempfile import unittest +from collections.abc import Hashable from pathlib import Path import yaml @@ -37,7 +38,7 @@ class UniqueKeyLoaderTest(unittest.TestCase): def load(self, content: str): return yaml.load(content, Loader=UniqueKeyLoader) - def assert_duplicate_key(self, content: str, key: str): + def assert_duplicate_key(self, content: str, key: Hashable): with self.assertRaisesRegex( yaml.constructor.ConstructorError, rf"found duplicate key {key!r}", @@ -145,6 +146,152 @@ def test_duplicate_error_reports_both_locations(self): self.assertEqual(error.context_mark.line, 0) self.assertEqual(error.problem_mark.line, 1) + def assert_unhashable_key(self, content: str): + with self.assertRaisesRegex( + yaml.constructor.ConstructorError, + "found an unhashable key", + ): + self.load(content) + + def test_rejects_duplicate_inside_inline_merge_source(self): + # flatten_mapping() recurses into a merge source without constructing it + # as a mapping, so the duplicate is only visible before construction. + self.assert_duplicate_key( + "dataset:\n <<: {name: orders, name: customers}\n", + "name", + ) + + def test_rejects_duplicate_inside_anchored_merge_source(self): + self.assert_duplicate_key( + "defaults: &defaults\n" + " source: staging.orders\n" + " source: production.orders\n" + "dataset:\n" + " <<: *defaults\n", + "source", + ) + + def test_allows_merge_override_reused_through_alias(self): + # flatten_mapping() rewrites the anchored mapping in place while building + # "first"; "second" must still be judged against the authored keys. + loaded = self.load( + "first:\n" + " <<: &defaults\n" + " <<: &base\n" + " source: staging.orders\n" + " source: production.orders\n" + "second: *defaults\n" + ) + + self.assertEqual(loaded["first"]["source"], "production.orders") + self.assertEqual(loaded["second"]["source"], "production.orders") + + def test_allows_merge_override_alias_before_merge_consumer(self): + # Same document as above with the alias resolved first: the verdict must + # not depend on the order in which mappings are constructed. + loaded = self.load( + "anchors:\n" + " defaults: &defaults\n" + " <<: &base\n" + " source: staging.orders\n" + " source: production.orders\n" + "second: *defaults\n" + "first:\n" + " <<: *defaults\n" + ) + + self.assertEqual(loaded["first"]["source"], "production.orders") + self.assertEqual(loaded["second"]["source"], "production.orders") + + def test_allows_sequence_form_merge(self): + loaded = self.load( + "base: &base\n source: staging.orders\n" + "extra: &extra\n owner: analytics\n" + "dataset:\n <<: [*base, *extra]\n name: orders\n" + ) + + self.assertEqual(loaded["dataset"]["source"], "staging.orders") + self.assertEqual(loaded["dataset"]["owner"], "analytics") + self.assertEqual(loaded["dataset"]["name"], "orders") + + def test_rejects_duplicate_inside_sequence_element(self): + self.assert_duplicate_key( + "datasets:\n - name: orders\n source: a\n source: b\n", + "source", + ) + + def test_rejects_unhashable_mapping_key(self): + # SafeLoader cannot use a collection as a key; the loader rejects such a + # key directly rather than constructing it during the pre-construction walk. + self.assert_unhashable_key("? {name: orders}\n: value\n") + + def test_rejects_unhashable_sequence_key(self): + self.assert_unhashable_key("? [orders, customers]\n: value\n") + + def test_unhashable_key_error_reports_both_locations(self): + with self.assertRaises(yaml.constructor.ConstructorError) as caught: + self.load("name: sales\n? {name: orders}\n: value\n") + + error = caught.exception + self.assertEqual(error.context_mark.line, 0) + self.assertEqual(error.problem_mark.line, 1) + + def test_rejects_equivalent_integer_spellings(self): + # Keys are compared as constructed scalars, so 01 (octal) equals 1. + self.assert_duplicate_key("1: first\n01: second\n", 1) + + def test_rejects_equivalent_boolean_spellings(self): + self.assert_duplicate_key("yes: first\ntrue: second\n", True) + + def test_rejects_boolean_key_equal_to_integer_key(self): + # Documented consequence of comparing constructed scalars: True == 1, and + # SafeLoader would otherwise collapse the two keys into one dict entry. + self.assert_duplicate_key("1: first\ntrue: second\n", True) + + def test_allows_quoted_and_integer_keys_that_differ(self): + loaded = self.load('"1": quoted\n1: integer\n') + + self.assertEqual(loaded["1"], "quoted") + self.assertEqual(loaded[1], "integer") + + def test_rejects_duplicate_in_later_document(self): + # Loader-level coverage only: validate.py loads a single document. + with self.assertRaisesRegex( + yaml.constructor.ConstructorError, + r"found duplicate key 'name'", + ): + list( + yaml.load_all( + "name: sales\n---\nname: finance\nname: ops\n", + Loader=UniqueKeyLoader, + ) + ) + + def test_allows_valid_multi_document_input(self): + loaded = list( + yaml.load_all("name: sales\n---\nname: finance\n", Loader=UniqueKeyLoader) + ) + + self.assertEqual([document["name"] for document in loaded], ["sales", "finance"]) + + def test_allows_recursive_mapping_alias(self): + # The pre-pass walks the node graph, so a self-referencing anchor must be + # guarded by node identity or the walk would not terminate. + loaded = self.load("root: &root\n self: *root\n") + + self.assertIs(loaded["root"]["self"], loaded["root"]) + + def test_allows_recursive_sequence_alias(self): + loaded = self.load("root: &root [*root]\n") + + self.assertIs(loaded["root"][0], loaded["root"]) + + def test_rejects_duplicate_in_recursive_mapping_alias(self): + self.assert_duplicate_key( + "root: &root\n self: *root\n self: other\n", + "self", + ) + class ValidatorIntegrationTest(unittest.TestCase): def run_validator(self, content: str) -> subprocess.CompletedProcess[str]: diff --git a/validation/validate.py b/validation/validate.py index d4b09645..f81918bf 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -80,38 +80,61 @@ class UniqueKeyLoader(yaml.SafeLoader): """Safe YAML loader that rejects duplicate explicit mapping keys.""" - def construct_mapping(self, node: yaml.MappingNode, deep: bool = False) -> dict: - seen = set() - merge_tag = "tag:yaml.org,2002:merge" - merge_key = object() - - for key_node, _ in node.value: - if key_node.tag == merge_tag: - key = merge_key - display_key = "<<" - else: - key = self.construct_object(key_node, deep=deep) - display_key = key - - if not isinstance(key, Hashable): - raise ConstructorError( - "while constructing a mapping", - node.start_mark, - "found an unhashable key", - key_node.start_mark, - ) - - if key in seen: - raise ConstructorError( - "while constructing a mapping", - node.start_mark, - f"found duplicate key {display_key!r}", - key_node.start_mark, - ) - seen.add(key) - - # Delegate construction (including merge-key flattening) to SafeLoader. - return super().construct_mapping(node, deep=deep) + MERGE_TAG = "tag:yaml.org,2002:merge" + + def construct_document(self, node: yaml.Node): + # Validate the composed node graph before SafeConstructor touches it: + # flatten_mapping() rewrites mapping nodes in place while expanding "<<", + # so a check that runs during construction sees merged, not authored, keys. + self._check_unique_keys(node, set()) + return super().construct_document(node) + + def _check_unique_keys(self, node: yaml.Node, visited: set) -> None: + if id(node) in visited: # alias or recursive anchor: check the node once + return + visited.add(id(node)) + + if isinstance(node, yaml.MappingNode): + seen = set() + merge_key = object() + for key_node, value_node in node.value: + if key_node.tag == self.MERGE_TAG: + key, display_key = merge_key, "<<" + elif isinstance(key_node, yaml.ScalarNode): + key = display_key = self.construct_object(key_node, deep=True) + else: + # Collection keys are unhashable under SafeLoader. Reject them + # here rather than constructing them, which would run + # flatten_mapping() on the graph this walk must not disturb. + raise ConstructorError( + "while constructing a mapping", + node.start_mark, + "found an unhashable key", + key_node.start_mark, + ) + + if not isinstance(key, Hashable): + raise ConstructorError( + "while constructing a mapping", + node.start_mark, + "found an unhashable key", + key_node.start_mark, + ) + + if key in seen: + raise ConstructorError( + "while constructing a mapping", + node.start_mark, + f"found duplicate key {display_key!r}", + key_node.start_mark, + ) + seen.add(key) + + self._check_unique_keys(key_node, visited) + self._check_unique_keys(value_node, visited) + elif isinstance(node, yaml.SequenceNode): + for child in node.value: + self._check_unique_keys(child, visited) def validate_schema(data: dict, schema: dict) -> list[str]: