fix: prevent CitationMixin crash when validation_context is missing 'context' key (#2459) - #2492
Conversation
ErenAta16
left a comment
There was a problem hiding this comment.
The fix itself is right, text_chunks comes back None when the validation context exists but has no context key, and both downstream uses assume a string:
regex.search('(quote){e<=0}', None) -> TypeError: expected string or buffer
with text_chunks[span[0]:span[1]] failing on None as well. Returning early rather than patching either site is the correct shape, and it matches the info.context is None guard already two lines above.
Flagging an overlap though: #2488 landed the byte-identical change (if text_chunks is None: return self, same location) a day earlier, and additionally ships a regression test plus a CHANGELOG entry. Since this bug is a hard crash rather than a silent wrong result, a test is worth having so it can't come back, and #2488 already carries one.
Nothing wrong with this PR, it's just the strictly smaller subset of the earlier one. Probably worth consolidating on #2488 unless a maintainer prefers this one, in which case lifting the test across would be the thing to do.
|
Correction to my review above: I wrote "#2488 is the more complete of the two", but there are four PRs for #2459, not two. #2460 (Jul 18, by the issue author) predates both #2488 and this one by eight days and also ships a test, and #2471 was a fifth attempt that's since been closed. My duplicate check only looked at recently-created PRs, which missed the earlier ones. Apologies for the inaccurate framing. The technical assessment of this PR stands, the fix is correct and the only gap is the missing test. But on merge order #2460 has priority rather than #2488. Full comparison on #2460. |
|
Closed as a duplicate implementation. The citation-context fix and regression coverage shipped in #2495 via the consolidated canonical patch. |
Description
This PR addresses an issue where
CitationMixin.validate_sourcescrashes with aTypeErrorif a user provides avalidation_contextdictionary but omits the expected"context"key.Previously, when the
"context"key was missing,info.context.get("context", None)returnedNone. ThisNonevalue was then passed directly intoself.get_spans(), which caused the underlyingregex.searchengine to crash because it expected a string or bytes-like object.Changes Proposed
if text_chunks is None: return self) directly after the.get()lookup ininstructor/v2/dsl/citation.py.Related Issues
Closes #2459