Skip to content

fix: reject malformed JSON feed objects without a listings key - #2

Open
TrueFurina wants to merge 2 commits into
quangshuynh:mainfrom
TrueFurina:fix/reject-malformed-json-feed
Open

TrueFurina wants to merge 2 commits into
quangshuynh:mainfrom
TrueFurina:fix/reject-malformed-json-feed

Conversation

@TrueFurina

Copy link
Copy Markdown

Problem

fetch_listings() silently treated a top-level object without a listings key as an empty feed (raw.get("listings", [])), making malformed input look like a legitimate empty result.

Fix

  • A top-level object missing the listings key now raises ValueError describing the expected shape (including the keys actually present).
  • A top-level object whose listings value is not a list is rejected with a clear error.
  • {"listings": []} remains a valid empty feed.
  • Top-level lists remain supported unchanged.

Tests

Added 4 cases covering: missing-key, wrong-type, valid-empty-object, top-level-list. Verified locally with the acceptance criteria:

  • missing key -> ValueError "must contain a 'listings' key"
  • listings as dict -> ValueError "must be a list, got dict"
  • empty object -> 0 listings
  • top-level list -> parsed unchanged

@quangshuynh quangshuynh left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! The validation behavior and test coverage are headed in the right direction.

There are two things to address before this is ready to merge:

  1. CI currently fails at ruff format --check . because collectors/json_feed_collector.py would be reformatted. Please run the repository's formatter and push the result
  2. The PR currently shows much more file churn than this change requires (+270/-201 across two files), with many apparently unchanged lines appearing in the diff. Please avoid unrelated formatting or line-ending changes and keep the patch focused on the feed-shape validation and its tests

Once the diff is focused and CI is green, I’m happy to take another look

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants