fix: crash/leak fixes in reader, source, writer and CLI arg parsing - #73
Open
maan-iitd2 wants to merge 1 commit into
Open
fix: crash/leak fixes in reader, source, writer and CLI arg parsing#73maan-iitd2 wants to merge 1 commit into
maan-iitd2 wants to merge 1 commit into
Conversation
- xml_file_reader: stop double-parsing the file (xmltodict + a second ElementTree pass) and leaking the file handle on exceptions; derive the parent tag from the already-parsed dict instead. - json_source: raise the documented ValueError when the configured id key is absent from the payload, instead of an unguarded KeyError. - writer/util: raise a clear ValueError for an unknown header/footer prop type instead of a bare "NoneType not callable"; make get_new_line return '' (not None) when the flag is false, so string concatenation in get_custom_value doesn't blow up. - outer_join: raise a clear KeyError when the config omits 'source' or names a source that was never fetched, instead of an AttributeError deep in pandas. - utils.KeyValue: split CLI key=value args on the first '=' only, so values containing '=' (tokens, URLs, SQL fragments) survive. Each fix ships with a regression test; none change output for already-valid configs. Verified against the full test suite (no regressions vs main) and an end-to-end `python -m ingen` run. Signed-off-by: maan-iitd2 <maan.iitd.ac.in@gmail.com>
shpiyu
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Five small, independent crash/leak fixes in existing
ingenlibrary code. No behavior change for any config that was already working — each fix only changes what happens on a previously-unhandled error path.ingen/reader/xml_file_reader.pyxmltodict+ a secondElementTree.parse) and never closed the file handle if parsing raisedxmltodict, file handle closed viawith, parent tag derived from the parsed dict (also fixes a namespace-prefix mismatch between the two parsers)ingen/data_source/json_source.pyjson_dict[self.id]raised a rawKeyErrorif the id was absent from the payloadValueError("JSON source with ID {} does not exist")ingen/writer/util.pyTypeError: 'NoneType' object is not callable;get_new_line(False)returnedNone, which breaks thecustom_string + resultconcatenation inget_custom_valueValueError;get_new_linereturns''for the false caseingen/pre_processor/outer_join.pysourcein config surfaced asAttributeError: 'NoneType' object has no attribute 'columns'KeyErrornaming the problemingen/utils/utils.py(KeyValueCLI action)value.split('=')on--query_params/--override_paramsbroke for any value containing its own=(tokens, URLs with query strings,col=valfragments)=only, matchingKeyValueOrString's existing behaviourWhy these and not others
This PR intentionally excludes two other fixes from the same working branch (
not_equals_filter.pyraising on a missing source, and areplace_in_listNaN-matching change) because those alter existing behavior rather than just fixing a crash. Happy to open those separately with more discussion if maintainers want them.Testing
pytest test/— 399 passed / 33 failed, identical failure set and count on unmodifiedmain(confirmed by running the suite before and after this diff). The 33 failures are pre-existing environment issues (missing GPG binary, Windows file locks, live network calls) unrelated to this change.python -m ingenagainst a real config with an XML source and--query_paramscontaining an embedded=, and manually exercisedget_custom_value,get_new_line,OuterJoin.execute, andJsonSource.fetchagainst real (non-mocked) objects to confirm each fix behaves as described.git diff --ignore-all-space mainconfirms these are the only 5 real line changes — no line-ending noise.🤖 Generated with Claude Code