fix(ebcdic): require record names to be unique - #4126
Open
aniketwaghh wants to merge 1 commit into
Open
Conversation
EbcdicLayout checks that record selectors are unique but not that names are, and EbcdicRecordLayout.name defaults to "record". A copybook with two schemas, unique selectors and no names was therefore accepted, and _RecordParser.parse buckets decoded rows by name, so both schemas shared one bucket. Every table then held every record, with each schema's own header written over the combined list, and the conversion still reported success. Add the missing check next to the selector one it mirrors. This also resolves the two identical "## record" headings such a layout produced, since name is what add_heading prints. Signed-off-by: aniketwaghh <aniketwaghh@users.noreply.github.com>
Contributor
|
✅ DCO Check Passed Thanks @aniketwaghh, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
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
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.
EbcdicLayoutrequires record selectors to be unique but says nothing about record names, andEbcdicRecordLayout.namedefaults to"record". A copybook with two schemas, unique selectors andno names is therefore accepted, and
_RecordParser.parsebuckets the decoded rows by name:Records are selected by
selectorbut stored byname, so both schemas share one bucket.convert()then hands each schema that combined list while
_build_tablewrites the schema's own header overit, and every table ends up holding every record. The conversion reports
ConversionStatus.SUCCESS.This adds the missing check to
_validate_records, next to the selector one it mirrors. It alsoresolves the two identical
## recordheadings such a layout produced, sincenameis whatadd_headingprints.Before, on
mainat 97bdf30, converting an 84-byte cp037 file holding two customer records taggedCand two transaction records taggedT:A transaction amount of
1250.50is presented as a customer'saccount_no, and the account number0041as a monetaryamount. After, the same layout raises at construction:Adding
name="customer"andname="transaction", changing nothing else, converts to the twocorrect tables both before and after this change.
I considered keying
rowsby the schema's index instead, which would fix the data without rejectingany layout. I went with the validator because
nameis also the table heading, so two schemassharing one is ambiguous output even when the rows are right — but say the word if you would rather
not reject layouts that are currently accepted, and I will switch it.
Three tests.
test_record_names_must_be_uniqueandtest_record_names_must_be_unique_from_a_layout_filecover the two ways a copybook arrives, inline and as JSON; both fail on
mainand pass here.test_a_single_record_keeps_the_default_nameguards the case the check must not break — one schemakeeping the
"record"default — and passes either way, so it is a regression guard rather than abug test.
Suite on this branch and on
mainat 97bdf30, same machine: 12 failed / 1777 passed here against12 failed / 1774 passed there, the three extra passes being the tests above. The twelve failures are
the same twelve on both sides — asr_pipeline, webp, the two e2e conversions and the two extraction
template tests, all model- or network-dependent.
make validateis clean.Issue resolved by this Pull Request:
Resolves #4125
Checklist: