fix: raise RuntimeError on invalid JSON instead of crashing with NameError

[sourabhgithubcode]

Summary

Fixes #10

When a descriptor file contains invalid JSON, _create_descriptor caught the ValueError and logged it — but never assigned descriptor_dict. The function then fell through to return Descriptor(descriptor_dict, file_name), raising a NameError that escaped DescriptorsFromDirectory._get_from_dir's except clause, crashing the data model manager.

Root cause: except ValueError silently swallowed the error instead of propagating it.

Fix: Re-raise as RuntimeError so the caller's existing except (Exception, ValueError, RuntimeError) catches it and continues to the next file — which was the intended behavior.

Changes

• data_resource_api/app/utils/descriptor.py — raise RuntimeError on invalid JSON; add except RuntimeError: raise guard so the outer except Exception doesn't swallow it
• tests/descriptor_util_classes/test_descriptor_from_file.py — replace skipped stub tests with three regression tests:
  • invalid JSON file raises RuntimeError
  • directory path raises RuntimeError
  • invalid file is skipped while valid files still load

Test plan

• pytest tests/descriptor_util_classes/test_descriptor_from_file.py
• Place an invalid JSON file in the schema directory and confirm the service starts without crashing

---

Note

Low Risk
Low risk: changes are limited to error propagation when parsing descriptor JSON and adds tests to prevent regressions. Main risk is slightly different exception behavior/message for invalid descriptor files.

Overview
Fixes descriptor loading to fail fast on invalid JSON by raising a RuntimeError (instead of logging and later crashing due to an uninitialized descriptor_dict), and ensures the raised RuntimeError isn’t swallowed by the generic exception handler.

Replaces skipped placeholder tests with regression coverage verifying DescriptorFromFile raises on invalid JSON and directory paths, and that DescriptorsFromDirectory skips bad JSON while still loading valid descriptors.

^{Written by Cursor Bugbot for commit 21a72ce. This will update automatically on new commits. Configure here.}

Summary by CodeRabbit

• Bug Fixes

  • Invalid JSON input now raises explicit error messages instead of silent logging, improving error visibility and debugging experience.
  • Enhanced error handling to provide clearer feedback when schema validation fails.

• Tests

  • Added comprehensive test coverage for error scenarios and path validation.

[coderabbitai]

📝 Walkthrough

Walkthrough

Error handling in the descriptor loading system is improved by converting JSON decode failures from info logging to explicit RuntimeError exceptions, with explicit error propagation to prevent outer exception handlers from masking these errors.

Changes

Cohort / File(s)	Summary
Descriptor Error Handling data_resource_api/app/utils/descriptor.py	Modified DescriptorFromFile._create_descriptor to raise RuntimeError on JSON decoding failures instead of logging info messages; added explicit except RuntimeError: raise clause to ensure newly raised errors propagate outward without being caught by the outer generic exception handler.
Descriptor Tests tests/descriptor_util_classes/test_descriptor_from_file.py	Replaced placeholder tests with three active test cases verifying DescriptorFromFile raises RuntimeError for invalid JSON and directory paths, and that DescriptorsFromDirectory skips invalid JSON files while loading valid descriptors. Updated imports to include json, os, shutil, and DescriptorsFromDirectory.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A JSON error caught with proper care,
No more silent logs floating in air!
When parsing fails, we raise and declare,
Invalid schemas now get called out fair!
Directory walkers skip the bad, take the good—
Error handling works just as it should! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: changing from crashing with NameError to raising RuntimeError on invalid JSON.
Description check	✅ Passed	The description covers the root cause, fix, changes made, and test plan. It includes issue link, code walk-through, and context for the reviewer.
Linked Issues check	✅ Passed	The PR directly addresses issue #10 by fixing the crash when descriptor files contain invalid JSON and ensuring proper error propagation.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing error handling for invalid JSON parsing and adding regression tests; no unrelated modifications present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.

Add a Ruff configuration file to your project to customize how CodeRabbit runs ruff.

[coderabbitai]

🧹 Nitpick comments (2)

data_resource_api/app/utils/descriptor.py (2)

82-85: Use exception chaining to preserve the original traceback.

The static analysis hint is valid. When re-raising as a different exception type, use from to chain exceptions and preserve the original ValueError traceback for debugging.

♻️ Proposed fix

                 except ValueError:
-                    raise RuntimeError(
+                except ValueError as e:
+                    raise RuntimeError(
                         f"Failed to load JSON file. JSON is probably invalid in file '{os.path.join(schema_dir, file_name)}'"
-                    )
+                    ) from e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data_resource_api/app/utils/descriptor.py` around lines 82 - 85, The except
block that catches ValueError and re-raises a RuntimeError loses the original
traceback; capture the original exception (e.g., except ValueError as e:) and
re-raise using exception chaining (raise RuntimeError(f"Failed to load JSON
file. JSON is probably invalid in file '{os.path.join(schema_dir, file_name)}'")
from e) so the original ValueError traceback is preserved; update the except
clause in descriptor.py where ValueError is handled to use the as e pattern and
the from e chaining.

---

48-48: Redundant exception types in the except clause.

ValueError and RuntimeError are both subclasses of Exception, so listing them explicitly alongside Exception has no effect. This may have been intentional to document expected exception types, but it could confuse readers.

♻️ Proposed simplification

-                except (Exception, ValueError, RuntimeError) as e:
+                except Exception as e:

If documenting expected types is desired, a comment would be clearer:

                # Catches RuntimeError (invalid JSON, directory as file) and other errors
                except Exception as e:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data_resource_api/app/utils/descriptor.py` at line 48, The except clause in
descriptor.py currently lists redundant exception types ("except (Exception,
ValueError, RuntimeError) as e"); simplify it to just "except Exception as e"
and, if you want to document expected errors, add a brief comment above (e.g.,
"# catches RuntimeError/ValueError and other exceptions") to make intent clear
without repeating subclasses.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@data_resource_api/app/utils/descriptor.py`:
- Around line 82-85: The except block that catches ValueError and re-raises a
RuntimeError loses the original traceback; capture the original exception (e.g.,
except ValueError as e:) and re-raise using exception chaining (raise
RuntimeError(f"Failed to load JSON file. JSON is probably invalid in file
'{os.path.join(schema_dir, file_name)}'") from e) so the original ValueError
traceback is preserved; update the except clause in descriptor.py where
ValueError is handled to use the as e pattern and the from e chaining.
- Line 48: The except clause in descriptor.py currently lists redundant
exception types ("except (Exception, ValueError, RuntimeError) as e"); simplify
it to just "except Exception as e" and, if you want to document expected errors,
add a brief comment above (e.g., "# catches RuntimeError/ValueError and other
exceptions") to make intent clear without repeating subclasses.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 68e5df7a-c17e-4e33-897a-8d37fd519490

📥 Commits

Reviewing files that changed from the base of the PR and between 2ab054e and 21a72ce.

📒 Files selected for processing (2)

• data_resource_api/app/utils/descriptor.py
• tests/descriptor_util_classes/test_descriptor_from_file.py