Skip to content

fix(sitemap): always return a list from extract_urls_from_sitemap#743

Open
valter-silva-au wants to merge 1 commit into
aws-samples:mainfrom
valter-silva-au:task/TASK-00028
Open

fix(sitemap): always return a list from extract_urls_from_sitemap#743
valter-silva-au wants to merge 1 commit into
aws-samples:mainfrom
valter-silva-au:task/TASK-00028

Conversation

@valter-silva-au

Copy link
Copy Markdown

Issue

extract_urls_from_sitemap() in lib/shared/layers/python-sdk/python/genai_core/websites/sitemap.py places return urls inside the else: clause of a try/except/else. The else block only runs when the try body raises nothing, so on any error the except swallows the exception, the function falls off the end, and it implicitly returns None instead of the partial/empty list.

Real sitemaps hit this regularly — unreachable child sitemaps, malformed/rejected XML, gzip failures. Two concrete blast radii:

  • Recursive self-call (sitemap.py): a sitemapindex recurses into each child via urls.extend(extract_urls_from_sitemap(elem.text)). If a child errors and returns None, the parent does urls.extend(None)TypeError: 'NoneType' object is not iterable, which its own except then swallows — so the whole tree collapses to None.
  • External caller (documents.py): limit = min(limit, len(urls_to_crawl))len(None)TypeError, masked by a generic except that discards the partial crawl results.

Fix

Dedent return urls out of the else: to function-body level so the function returns the (possibly partial) accumulated list on every path — success, non-200, and exception — and never None. The except branch already leaves urls as the list gathered so far, so a single trailing return urls covers all cases.

    except Exception as e:
        print(f"Error while processing sitemaps for {sitemap_url}", e)
    return urls

Tests

Added to the existing tests/shared/layers/python-sdk/genai_core/websites/sitemap_test.py, using the repo's established mocker fixture (pytest-mock, same style as the sibling crawler_test.py):

  • test_extract_urls_returns_list_when_request_failsrequests.get raises → asserts the return is [] (a list), not None.
  • test_extract_urls_returns_list_when_child_sitemap_fails — a sitemapindex whose child fetch raises → asserts the parent still returns a list rather than crashing on urls.extend(None).

Both tests fail on the current code (RED: None / 'NoneType' object is not iterable) and pass after the dedent (GREEN). The full pytest tests/ suite stays green (269 passed); flake8, black --check, and bandit are clean on the changed files. Pure logic + a requests.get mock — no AWS deploy required.

Risk

Low. Single-function control-flow fix touching only the error/return path; the success path is byte-for-byte identical; backward-compatible; no API/CDK-synth surface.

- Dedent `return urls` out of the try/else so the function returns the
  (possibly partial) list on every path instead of falling through to
  None when the try body raises (unreachable child sitemap, malformed
  XML, gzip failure).
- This fixes the recursion blast-radius where a single failing child
  sitemap caused urls.extend(None) -> TypeError on the parent, and the
  documents.py len(None) crash that discarded partial crawl results.
- Add tests (pytest-mock): requests.get raising returns [], and a
  sitemapindex whose child fetch fails still returns a list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant