From 55ba1c16528a63f790eaf7b4bbad567bae6125b3 Mon Sep 17 00:00:00 2001 From: Alaa-Taieb <93670187+Alaa-Taieb@users.noreply.github.com> Date: Wed, 15 Jul 2026 06:53:01 +0100 Subject: [PATCH] fix(salary): validate category shape and add --validate preflight (#156) validate_data() accepted category values that are not {count?, index?} objects. They slipped through to format_entry(), which then raised AttributeError on a normal table lookup (or serialized a malformed shape under --json). It also accepted duplicate company names silently. - collect_validation_issues() now also flags a non-object category value (and non-numeric count / non number-or-string index) as a hard error, and duplicate company names as a warning. - validate_data() keeps its eager-fail behavior (same messages), so existing tests and load_data() are unchanged. - --validate runs the checks standalone and prints an actionable report (exit 1 on errors, 0 on warnings-only/clean), letting users pre-flight their BYO salary_data.json. Reproduced on master: validate_data({'companies':[{'company':'Acme', 'categories':{'eng':'not_a_dict'}}]}) returns without error, but format_entry then raises AttributeError. Co-authored-by: Tunic Assistant --- salary_lookup.py | 113 ++++++++++++++++++++++++++++++++---- tests/test_salary_lookup.py | 86 ++++++++++++++++++++++++++- tools/README_SALARY_TOOL.md | 2 + 3 files changed, 189 insertions(+), 12 deletions(-) diff --git a/salary_lookup.py b/salary_lookup.py index 0295e29..8020935 100644 --- a/salary_lookup.py +++ b/salary_lookup.py @@ -51,39 +51,94 @@ def fail_data_error(message): sys.exit(1) -def validate_data(data): - """Validate the salary data shape before lookups use it.""" +def collect_validation_issues(data): + """Return (errors, warnings) for the salary data shape. + + errors -> hard problems that make lookups crash or emit wrong output + (these cause validate_data() to exit(1)). + warnings -> usability concerns that still work (e.g. duplicate company + names); --validate reports them but exits 0. + """ + errors = [] + warnings = [] + if not isinstance(data, dict): - fail_data_error("top-level JSON value must be an object") + errors.append("top-level JSON value must be an object") + return errors, warnings metadata = data.get("metadata", {}) if metadata is not None and not isinstance(metadata, dict): - fail_data_error("'metadata' must be an object when provided") + errors.append("'metadata' must be an object when provided") companies = data.get("companies") if not isinstance(companies, list): - fail_data_error("'companies' must be a list") + errors.append("'companies' must be a list") + return errors, warnings + seen_companies = {} for index, entry in enumerate(companies, start=1): if not isinstance(entry, dict): - fail_data_error(f"companies[{index}] must be an object") + errors.append(f"companies[{index}] must be an object") + continue company = entry.get("company") if not isinstance(company, str) or not company.strip(): - fail_data_error(f"companies[{index}].company must be a non-empty string") + errors.append(f"companies[{index}].company must be a non-empty string") + else: + key = company.lower() + if key in seen_companies: + warnings.append( + f"Duplicate company name '{company}' " + f"(companies[{seen_companies[key]}] and companies[{index}])" + ) + else: + seen_companies[key] = index city = entry.get("city") if city is not None and not isinstance(city, str): - fail_data_error(f"companies[{index}].city must be a string when provided") + errors.append(f"companies[{index}].city must be a string when provided") categories = entry.get("categories", {}) if categories is not None and not isinstance(categories, dict): - fail_data_error(f"companies[{index}].categories must be an object when provided") + errors.append(f"companies[{index}].categories must be an object when provided") + elif categories: + for cat_label, cat_data in categories.items(): + if not isinstance(cat_data, dict): + errors.append( + f"companies[{index}].categories.{cat_label} must be an object " + f"with 'count' and/or 'index' (got {type(cat_data).__name__})" + ) + continue + count = cat_data.get("count") + if count is not None and not isinstance(count, (int, float)): + errors.append( + f"companies[{index}].categories.{cat_label}.count must be a " + f"number (got {type(count).__name__})" + ) + index_val = cat_data.get("index") + if index_val is not None and not isinstance(index_val, (int, float, str)): + errors.append( + f"companies[{index}].categories.{cat_label}.index must be a " + f"number or string (got {type(index_val).__name__})" + ) + return errors, warnings + + +def validate_data(data): + """Validate the salary data shape before lookups use it. + + Preserves historical behavior: exits(1) on the first hard error with the + same user-facing message, and returns data unchanged when valid. + """ + errors, _ = collect_validation_issues(data) + if errors: + fail_data_error(errors[0]) return data -def load_data(): +def read_raw_data(): + """Load and JSON-parse salary_data.json; exit with a helpful message if missing/invalid.""" if not DATA_FILE.exists(): print("Error: salary_data.json not found.", file=sys.stderr) print("", file=sys.stderr) @@ -98,7 +153,12 @@ def load_data(): data = json.load(f) except json.JSONDecodeError as exc: fail_data_error(f"invalid JSON at line {exc.lineno}, column {exc.colno}: {exc.msg}") - return validate_data(data) + return data + + +def load_data(): + """Load, parse, and validate salary_data.json for lookups.""" + return validate_data(read_raw_data()) def normalize(s): @@ -292,14 +352,45 @@ def format_entry(entry, metadata): return "\n".join(lines) +def print_validation_report(errors, warnings): + """Print an actionable validation report. Returns the process exit code.""" + if not errors and not warnings: + print("OK - no issues found.") + return 0 + print(f"Found {len(errors) + len(warnings)} issue(s):") + if errors: + print(" Errors:") + for i, msg in enumerate(errors, start=1): + print(f" [{i}] {msg}") + if warnings: + print(" Warnings:") + for i, msg in enumerate(warnings, start=1): + print(f" [{i}] {msg}") + if errors: + print("") + print("Fix the errors above, then re-run. See tools/README_SALARY_TOOL.md " + "for the expected format.") + return 1 + return 0 + + def main(): parser = argparse.ArgumentParser(description="Salary Benchmark Lookup") parser.add_argument("company", nargs="?", help="Company name to search for") parser.add_argument("--city", help="Filter by city name") parser.add_argument("--json", action="store_true", help="Output as JSON") parser.add_argument("--list-all", action="store_true", help="List all companies") + parser.add_argument("--validate", action="store_true", + help="Validate salary_data.json and print a report, then exit") args = parser.parse_args() + if args.validate: + data = read_raw_data() + errors, warnings = collect_validation_issues(data) + print(f"Validating {DATA_FILE.name} ...") + print("") + sys.exit(print_validation_report(errors, warnings)) + data = load_data() metadata = data.get("metadata", {}) companies = data.get("companies", []) diff --git a/tests/test_salary_lookup.py b/tests/test_salary_lookup.py index dd5e46f..0a4dd06 100644 --- a/tests/test_salary_lookup.py +++ b/tests/test_salary_lookup.py @@ -3,8 +3,9 @@ import io import tempfile import unittest -from contextlib import redirect_stderr +from contextlib import redirect_stderr, redirect_stdout from pathlib import Path +from unittest import mock import salary_lookup from salary_lookup import ( @@ -15,6 +16,7 @@ from salary_lookup import ( match_score, search_company, validate_data, + collect_validation_issues, ) @@ -241,6 +243,88 @@ class ValidateDataTests(unittest.TestCase): ) +class ValidateDataShapeTests(ValidateDataTests): + """Category-shape and duplicate-name checks (reuses assert_invalid_data).""" + + def test_malformed_category_value_rejected(self): + data = {"companies": [{"company": "Acme", "categories": {"eng": "not_a_dict"}}]} + self.assert_invalid_data(data, "must be an object with 'count' and/or 'index'") + + def test_non_numeric_count_rejected(self): + data = { + "companies": [ + {"company": "Acme", "categories": {"eng": {"count": "many"}}} + ] + } + self.assert_invalid_data(data, "count must be a number") + + def test_duplicate_company_name_is_warning(self): + data = { + "companies": [ + {"company": "Acme"}, + {"company": "Other Corp"}, + {"company": "Acme"}, + ] + } + errors, warnings = collect_validation_issues(data) + self.assertEqual(errors, []) + self.assertEqual(len(warnings), 1) + self.assertIn("Duplicate company name 'Acme'", warnings[0]) + + def test_valid_categories_have_no_issues(self): + data = { + "companies": [ + {"company": "Acme", "categories": {"eng": {"count": 5, "index": 108.5}}} + ] + } + errors, warnings = collect_validation_issues(data) + self.assertEqual(errors, []) + self.assertEqual(warnings, []) + + +class ValidateFlagTests(unittest.TestCase): + """End-to-end checks for the --validate pre-flight flow.""" + + def _run_validate(self, payload): + with tempfile.TemporaryDirectory() as tmpdir: + data_file = Path(tmpdir) / "salary_data.json" + data_file.write_text(payload, encoding="utf-8") + original_data_file = salary_lookup.DATA_FILE + salary_lookup.DATA_FILE = data_file + argv_patch = mock.patch("sys.argv", ["salary_lookup.py", "--validate"]) + argv_patch.start() + try: + stdout = io.StringIO() + with self.assertRaises(SystemExit) as raised: + with redirect_stdout(stdout): + salary_lookup.main() + return raised.exception.code, stdout.getvalue() + finally: + argv_patch.stop() + salary_lookup.DATA_FILE = original_data_file + + def test_validate_flag_exits_1_on_errors(self): + code, out = self._run_validate( + '{"companies": [{"company": "Acme", "categories": {"eng": "not_a_dict"}}]}' + ) + self.assertEqual(code, 1) + self.assertIn("must be an object with 'count' and/or 'index'", out) + + def test_validate_flag_exits_0_on_clean(self): + code, out = self._run_validate( + '{"companies": [{"company": "Acme", "categories": {"eng": {"count": 5}}}]}' + ) + self.assertEqual(code, 0) + self.assertIn("OK", out) + + def test_validate_flag_exits_0_on_duplicates_only(self): + code, out = self._run_validate( + '{"companies": [{"company": "Acme"}, {"company": "Acme"}]}' + ) + self.assertEqual(code, 0) + self.assertIn("Duplicate company name", out) + + class UtilityTests(unittest.TestCase): def test_normalize_strips_suffix_and_noise(self): self.assertEqual(normalize("Novo Nordisk A/S"), "novonordisk") diff --git a/tools/README_SALARY_TOOL.md b/tools/README_SALARY_TOOL.md index 835bea4..6882638 100644 --- a/tools/README_SALARY_TOOL.md +++ b/tools/README_SALARY_TOOL.md @@ -113,6 +113,7 @@ python3 salary_lookup.py "Novo Nordisk" python3 salary_lookup.py "Ørsted" --city "Fredericia" python3 salary_lookup.py "COWI" --json python3 salary_lookup.py --list-all +python3 salary_lookup.py --validate # pre-flight check your salary_data.json ``` ## Important notes @@ -120,3 +121,4 @@ python3 salary_lookup.py --list-all - The data file (`salary_data.json`) is **excluded from git** (see `.gitignore`). Your salary data may be proprietary or confidential. - If the data file is missing, `salary_lookup.py` exits with a helpful error message and the `/apply` workflow skips the salary benchmark step. - The fuzzy matcher handles Danish company name variations: legal suffixes, Nordic characters, anglicized spellings, and partial matches. +- `--validate` checks your data file for malformed category values and duplicate company names and prints a report, without performing a lookup.