From 9833a5dcb75dcbeefb053c09f77639356de834a3 Mon Sep 17 00:00:00 2001 From: soumyadip sarkar Date: Wed, 2 Sep 2026 01:05:59 +0530 Subject: [PATCH] fix(salary): treat null metadata/categories as absent instead of crashing (#413) --validate treats an explicit "metadata": null / "categories": null the same as an omitted key ("...must be an object when provided", None is skipped), but format_entry read both through dict.get(key, {}), which only substitutes the default for an *absent* key - a present-but-null value passed through. The renderer then hit None.get("index_label", ...) (AttributeError) or, via the numeric-field fallback, None[key] = value (TypeError), so a hand-maintained salary_data.json using null for "no value" died with an uncaught traceback right after printing "Found 1 match(es)". format_entry now coerces both to {} up front, honouring the validator's existing "when provided" contract at the single consumer that broke it. Tests (all verified to fail on the unfixed renderer): - two unit cases calling format_entry with null metadata / null categories - two end-to-end cases running main() --validate (blesses the file) then the lookup path (renders it), one per null shape Plus an [Unreleased] CHANGELOG entry. Co-authored-by: Claude Sonnet 5 --- CHANGELOG.md | 13 ++++++ salary_lookup.py | 9 +++- tests/test_salary_lookup.py | 92 +++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c26fedd..abfb62d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,19 @@ per-file diff commands. Pinned by three new cases in `test_scrape_contract.py`, each verified to fail on the unfixed spec. Reported and diagnosed from a real run by @sandunwijerathne. +- **`salary_lookup.py` no longer crashes on a `null` `metadata` or `categories`** - `--validate` + treats an explicit `"metadata": null` / `"categories": null` the same as an omitted key (the + shape checks are "...must be an object *when provided*" and skip `None`), but the renderer read + both through `dict.get(key, {})`, which only substitutes the default for an *absent* key - a + present-but-null value passed straight through. `format_entry` then hit `None.get("index_label", + ...)` (`AttributeError`) or, via the numeric-field fallback, `None[key] = value` (`TypeError`), + so a hand-maintained `salary_data.json` using `null` for "no value here" died with an uncaught + traceback right after printing `Found 1 match(es)`. `format_entry` now coerces both to `{}` up + front, so `null`, absent, and `{}` behave identically. Pinned by four cases in + `test_salary_lookup.py` - two unit calls into `format_entry` and two end-to-end (`main() + --validate` blesses the file, then the lookup path renders it), one per null shape, all verified + to fail on the unfixed renderer. + ## [1.7.0] - 2026-08-29 ### Fixed diff --git a/salary_lookup.py b/salary_lookup.py index 4d1bbcb..a49fa6e 100644 --- a/salary_lookup.py +++ b/salary_lookup.py @@ -291,6 +291,11 @@ def search_company(data, query, city=None): def format_entry(entry, metadata): """Format a single company entry for display.""" + # `metadata` and `entry["categories"]` may be an explicit null: --validate + # treats a null the same as an omitted key ("...must be an object when + # provided"), but dict.get(key, default) only substitutes the default for an + # absent key, so a null reached `.get()`/`[]` here and crashed the lookup. + metadata = metadata or {} lines = [] lines.append(f"\n{'='*60}") lines.append(f" {entry['company']}") @@ -298,8 +303,8 @@ def format_entry(entry, metadata): lines.append(f" Location: {entry['city']}") lines.append(f"{'='*60}") - # Get category data (everything except company/city fields) - categories = entry.get("categories", {}) + # Get category data (everything except company/city fields). + categories = entry.get("categories") or {} if not categories: # Fallback: treat any numeric fields as categories skip_keys = {"company", "city", "categories"} diff --git a/tests/test_salary_lookup.py b/tests/test_salary_lookup.py index 77f4de9..d5ac424 100644 --- a/tests/test_salary_lookup.py +++ b/tests/test_salary_lookup.py @@ -87,6 +87,34 @@ class FormatEntryTests(unittest.TestCase): self.assertIn("45000.0", rendered) self.assertIn("+12.5%", rendered) + def test_null_categories_with_sibling_dict_does_not_crash(self): + # --validate accepts "categories": null, so format_entry must not crash + # on it. entry.get("categories", {}) returns None (not {}) for an + # explicit null, and the numeric-field fallback then did None[key] = .... + entry = { + "company": "Example Corp", + "city": "", + "categories": None, + "engineering": {"count": 10, "index": 105.0}, + } + + rendered = format_entry(entry, {"index_baseline": 100, "index_label": "Index"}) + + self.assertRegex(rendered, r"Engineering\s+10\s+105\.0") + + def test_null_metadata_does_not_crash(self): + # --validate accepts "metadata": null the same way; format_entry then did + # None.get("index_label", ...) -> AttributeError. + entry = { + "company": "Example Corp", + "city": "", + "categories": {"eng": {"count": 5, "index": 108.0}}, + } + + rendered = format_entry(entry, None) + + self.assertRegex(rendered, r"Eng\s+5\s+108\.0") + # --------------------------------------------------------------------------- # match_score tests (from #106) @@ -331,6 +359,70 @@ class ValidateFlagTests(unittest.TestCase): self.assertIn("Duplicate company name", out) +class NullShapeEndToEndTests(unittest.TestCase): + """The disagreement in full: --validate blesses a file with a null + metadata/categories, then the lookup path must render it, not crash. + + Both payloads pass --validate on master; the second command then dies + (TypeError in the categories fallback, AttributeError on metadata.get). + """ + + def _run_main(self, payload, *argv_tail): + """Run main() against `payload` with the given argv. Returns + (exit_code_or_None, stdout). main() returns normally on a successful + render, so a missing SystemExit is success, not an error.""" + 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", *argv_tail]) + argv_patch.start() + try: + stdout = io.StringIO() + try: + with redirect_stdout(stdout): + salary_lookup.main() + return None, stdout.getvalue() + except SystemExit as exc: + return exc.code, stdout.getvalue() + finally: + argv_patch.stop() + salary_lookup.DATA_FILE = original_data_file + + def test_null_categories_passes_validate_then_renders(self): + payload = ( + '{"metadata": {"index_label": "Index", "index_baseline": 100},' + ' "companies": [{"company": "Foo A/S", "city": "Aarhus",' + ' "categories": null,' + ' "engineering": {"count": 10, "index": 105}}]}' + ) + + code, out = self._run_main(payload, "--validate") + self.assertEqual(code, 0) + self.assertIn("OK", out) + + code, out = self._run_main(payload, "Foo") + self.assertIsNone(code) + self.assertIn("Foo A/S", out) + self.assertRegex(out, r"Engineering\s+10\s+105") + + def test_null_metadata_passes_validate_then_renders(self): + payload = ( + '{"metadata": null,' + ' "companies": [{"company": "Foo A/S", "city": "Aarhus",' + ' "categories": {"engineering": {"count": 10, "index": 105}}}]}' + ) + + code, out = self._run_main(payload, "--validate") + self.assertEqual(code, 0) + self.assertIn("OK", out) + + code, out = self._run_main(payload, "Foo") + self.assertIsNone(code) + self.assertRegex(out, r"Engineering\s+10\s+105") + + class UtilityTests(unittest.TestCase): def test_normalize_strips_suffix_and_noise(self): self.assertEqual(normalize("Novo Nordisk A/S"), "novonordisk")