test: OWASP mapping fixtures for GSoC ETL validation - #950
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThis PR adds OWASP mapping JSON fixtures and a unittest module that verifies fixture directory contents and validates each fixture’s schema, identifier format, uniqueness rules, and fallback section references. ChangesOWASP Mapping Fixtures
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
8c971c7 to
0e97a81
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
application/tests/owasp_mapping_fixtures_test.py (1)
25-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting
section_iduniqueness per fixture.The shape test validates individual field types/formats but doesn't check that
section_idvalues are unique within a fixture, which would catch copy-paste errors in future fixture additions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/owasp_mapping_fixtures_test.py` around lines 25 - 50, The fixture shape test in test_fixtures_have_expected_mapping_shape should also verify that section_id values are unique within each fixture. Add per-fixture tracking in the existing loop over payload entries so duplicate section_id values are detected while still preserving the current type/format checks for section, hyperlink, and cre_ids.application/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2025.json (1)
1-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fallback_section_idsfield isn't validated by the fixture test.This is the only fixture with a
fallback_section_idsarray, butapplication/tests/owasp_mapping_fixtures_test.pydoesn't assert its shape (list of strings, non-empty, referencing valid section IDs). Not a blocker since the field is optional/fixture-specific, but worth a light assertion if this field becomes load-bearing for the ETL logic being validated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2025.json` around lines 1 - 73, The fixture test currently ignores the shape of fallback_section_ids in the OWASP Kubernetes mapping fixture, so add a lightweight assertion in owasp_mapping_fixtures_test.py alongside the existing section validation. Use the fixture’s section_id set to verify each fallback_section_ids entry is a non-empty list of strings and that every referenced ID matches a known section_id. Keep the check optional/fixture-aware so it only applies when the field is present, and anchor the change near the existing OWASP mapping fixture validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2025.json`:
- Around line 1-73: The fixture test currently ignores the shape of
fallback_section_ids in the OWASP Kubernetes mapping fixture, so add a
lightweight assertion in owasp_mapping_fixtures_test.py alongside the existing
section validation. Use the fixture’s section_id set to verify each
fallback_section_ids entry is a non-empty list of strings and that every
referenced ID matches a known section_id. Keep the check optional/fixture-aware
so it only applies when the field is present, and anchor the change near the
existing OWASP mapping fixture validation logic.
In `@application/tests/owasp_mapping_fixtures_test.py`:
- Around line 25-50: The fixture shape test in
test_fixtures_have_expected_mapping_shape should also verify that section_id
values are unique within each fixture. Add per-fixture tracking in the existing
loop over payload entries so duplicate section_id values are detected while
still preserving the current type/format checks for section, hyperlink, and
cre_ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c1fc8608-6f0b-4fd6-9e8f-d06311dd0deb
📒 Files selected for processing (8)
application/tests/fixtures/owasp_mappings/owasp_aisvs_1_0.jsonapplication/tests/fixtures/owasp_mappings/owasp_api_top10_2023.jsonapplication/tests/fixtures/owasp_mappings/owasp_cheatsheets_supplement.jsonapplication/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2022.jsonapplication/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2025.jsonapplication/tests/fixtures/owasp_mappings/owasp_llm_top10_2025.jsonapplication/tests/fixtures/owasp_mappings/owasp_top10_2025.jsonapplication/tests/owasp_mapping_fixtures_test.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
7841f89 to
8c2e34c
Compare
2268801 to
805357e
Compare
805357e to
415e91f
Compare
northdpole
left a comment
There was a problem hiding this comment.
Review — OWASP mapping fixtures (#950)
Test-only fixtures + structural validation — good split from #900. Scope is clear and CI is green.
What looks good
- Fixture location under
application/tests/fixtures/owasp_mappings/keeps this out of production importer paths - Shape checks cover required fields (
section,hyperlink,cre_ids), CRE id format\d{3}-\d{3}, optionalsection_iduniqueness, andfallback_section_idsreferential integrity - Fixture-set completeness assertion avoids silent drift
- Matches unittest style used elsewhere in
application/tests/
Non-blocking (follow up in importer PRs #953/#960/#954 if needed)
- K8s Top Ten 2025 hyperlinks — every entry points at the project root (
https://owasp.org/www-project-kubernetes-top-ten/) rather than per-section URLs. Fine for a provisional fixture; tighten when the 2025 importer lands. - Self-referential fallbacks — e.g.
K01 → ["K01"],K04 → ["K04"]pass the “known section_id” check but don’t add information. Prefer omit or point at a different section. - Cheat sheets omit
section_id— OK given the test treats it as optional; #954 may want stable ids when normalizing.
Approving as the fixture base for the #471 split queue. PR body mentions pytest but the file is unittest — no change needed.
northdpole
left a comment
There was a problem hiding this comment.
Review — OWASP mapping fixtures (#950)
Test-only fixtures + structural validation — good split from #900. Scope is clear and CI is green.
What looks good
- Fixture location under
application/tests/fixtures/owasp_mappings/keeps this out of production importer paths - Shape checks cover required fields (
section,hyperlink,cre_ids), CRE id format\d{3}-\d{3}, optionalsection_iduniqueness, andfallback_section_idsreferential integrity - Fixture-set completeness assertion avoids silent drift
- Matches unittest style used elsewhere in
application/tests/
Non-blocking (follow up in importer PRs #953/#960/#954 if needed)
- K8s Top Ten 2025 hyperlinks — every entry points at the project root (
https://owasp.org/www-project-kubernetes-top-ten/) rather than per-section URLs. Fine for a provisional fixture; tighten when the 2025 importer lands. - Self-referential fallbacks — e.g.
K01 → ["K01"],K04 → ["K04"]pass the “known section_id” check but don’t add information. Prefer omit or point at a different section. - Cheat sheets omit
section_id— OK given the test treats it as optional; #954 may want stable ids when normalizing.
Approving as the fixture base for the #471 split queue. PR body mentions pytest but the file is unittest — no change needed.
- Update default path to `tests/fixtures/owasp_mappings/` - Add env override `OWASP_CHEATSHEETS_SUPPLEMENT_PATH` - Gracefully handle missing/malformed JSON - Validate required keys in entries Fixes CI failure due to moved file (PR OWASP#950).
The supplement file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the hardcoded path to match, resolving the `FileNotFoundError` seen in CI. Fixes: https://github.com/OWASP/OpenCRE/actions/runs/31112703672
The file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the parser to use the new location (parents[3] from parser file). This resolves the FileNotFoundError seen in CI.
…file path - Update `supplement_data_file` path to use `parents[3]` to resolve the fixture location at `tests/fixtures/owasp_mappings/` (as per PR OWASP#950) - Add existence check before attempting to load the supplemental JSON file - Wrap JSON loading in try/except to gracefully handle malformed or missing files, logging warnings/errors instead of crashing the parser - Validate required keys (`section`, `hyperlink`) in each supplemental entry and skip malformed entries with a warning - Maintain fallback behavior: return empty list if file is not found or fails to load, allowing parser to continue with repo-based cheatsheets This resolves the `FileNotFoundError` seen in CI and makes the parser more resilient to missing or corrupt supplemental data.
- Update default path to `tests/fixtures/owasp_mappings/` - Add env override `OWASP_CHEATSHEETS_SUPPLEMENT_PATH` - Gracefully handle missing/malformed JSON - Validate required keys in entries Fixes CI failure due to moved file (PR #950).
The supplement file was moved to `tests/fixtures/owasp_mappings/` in PR #950. Update the hardcoded path to match, resolving the `FileNotFoundError` seen in CI. Fixes: https://github.com/OWASP/OpenCRE/actions/runs/31112703672
The file was moved to `tests/fixtures/owasp_mappings/` in PR #950. Update the parser to use the new location (parents[3] from parser file). This resolves the FileNotFoundError seen in CI.
…file path - Update `supplement_data_file` path to use `parents[3]` to resolve the fixture location at `tests/fixtures/owasp_mappings/` (as per PR #950) - Add existence check before attempting to load the supplemental JSON file - Wrap JSON loading in try/except to gracefully handle malformed or missing files, logging warnings/errors instead of crashing the parser - Validate required keys (`section`, `hyperlink`) in each supplemental entry and skip malformed entries with a warning - Maintain fallback behavior: return empty list if file is not found or fails to load, allowing parser to continue with repo-based cheatsheets This resolves the `FileNotFoundError` seen in CI and makes the parser more resilient to missing or corrupt supplemental data.
- Update default path to `tests/fixtures/owasp_mappings/` - Add env override `OWASP_CHEATSHEETS_SUPPLEMENT_PATH` - Gracefully handle missing/malformed JSON - Validate required keys in entries Fixes CI failure due to moved file (PR OWASP#950).
The supplement file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the hardcoded path to match, resolving the `FileNotFoundError` seen in CI. Fixes: https://github.com/OWASP/OpenCRE/actions/runs/31112703672
The file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the parser to use the new location (parents[3] from parser file). This resolves the FileNotFoundError seen in CI.
…file path - Update `supplement_data_file` path to use `parents[3]` to resolve the fixture location at `tests/fixtures/owasp_mappings/` (as per PR OWASP#950) - Add existence check before attempting to load the supplemental JSON file - Wrap JSON loading in try/except to gracefully handle malformed or missing files, logging warnings/errors instead of crashing the parser - Validate required keys (`section`, `hyperlink`) in each supplemental entry and skip malformed entries with a warning - Maintain fallback behavior: return empty list if file is not found or fails to load, allowing parser to continue with repo-based cheatsheets This resolves the `FileNotFoundError` seen in CI and makes the parser more resilient to missing or corrupt supplemental data.
Summary
This PR is split out from #900 to make review smaller and more focused.
It adds test-only OWASP mapping fixtures for GSoC ETL validation by extracting the OWASP mapping JSON files into a dedicated test fixture location and adding focused validation coverage. The intent is to let reviewers assess the fixture data independently from production importer/runtime changes.
Issue reference:
Problem Fixed
PR #900 grouped multiple different concerns into a single larger review, including:
That made it harder to review the mapping fixture portion on its own.
For this part of the work, the useful standalone contribution is:
Solution
This PR:
application/tests/fixtures/owasp_mappings/section,hyperlink, andcre_idsare presentcre_idsvalues are non-empty and structurally validFiles in scope:
application/tests/fixtures/owasp_mappings/owasp_aisvs_1_0.jsonapplication/tests/fixtures/owasp_mappings/owasp_api_top10_2023.jsonapplication/tests/fixtures/owasp_mappings/owasp_cheatsheets_supplement.jsonapplication/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2022.jsonapplication/tests/fixtures/owasp_mappings/owasp_kubernetes_top10_2025.jsonapplication/tests/fixtures/owasp_mappings/owasp_llm_top10_2025.jsonapplication/tests/fixtures/owasp_mappings/owasp_top10_2025.jsonapplication/tests/owasp_mapping_fixtures_test.pyTests