Repository navigation
Fix validation of out-of-order tables split across fragments - #572
satriadhikara wants to merge 6 commits into
Conversation
davidpavlovschi
left a comment
There was a problem hiding this comment.
Reviewed the validation path against the reported nested out-of-order case and the existing duplicate/redefinition checks. Iterating the raw fragments avoids treating the proxy as a non-table while still validating every fragment against the candidate. I also ran the complete suite locally on this head: 1,051 passed, 1 skipped. The focused regression preserves both the unwrapped structure and exact serialization.
Add regression coverage for a valid multi-fragment extension and rejection of duplicate parent/child tables and scalar-to-table redefinitions.
|
Hi, following up on this PR after reading the discussion in #596. I’ve updated the branch against current master and added regression tests for collisions across nested out-of-order fragments. All 1,058 tests pass, and lint and formatting checks pass. Compared with #592, this approach applies the same type-conflict, concrete-table redefinition, and recursive checks to every stored fragment. This keeps validation in one path, avoiding proxy construction and a separate proxy-specific branch that would need to stay consistent with those checks. Both implementations pass the focused tests; I prefer this approach because it keeps the validation logic simpler. Would you be able to take another look? Happy to address any concerns with this approach. |
append() compared a new table only with the last fragment of an out-of-order table, so a table declared twice across fragments was accepted, e.g. [a.b.c] [a.b] [a] [a.b.d] [a.b]. Master rejected these only because the proxy was wrongly treated as a non-table (python-poetry#571). Also reject a [table] header redefining a table created by dotted keys, e.g. [a] b.y=1 [a.b.c] [q] [a.b]. Differential fuzzing against tomllib (~250k generated documents): no new disagreements vs master, and invalid documents wrongly accepted drop from 2813 to 14.
|
Updated against current master. While fuzzing against tomllib I found that tables declared twice across out-of-order fragments slipped through, e.g. |
Merging a super table in place into the last fragment of an out-of-order table mutated a fragment the validation cache had already copied, so later fragments were validated against a stale copy and e.g. a.x=1 [a.b.c] [a.b.d] [q] [a.b.d] was accepted. Merge the new part into the cached container as well. With the cache correct, earlier fragments only need the cheap check that a concrete [table] does not redefine a concrete or dotted-key fragment; drop the recursive per-fragment validation, which was quadratic for tables split into many fragments. Differential fuzzing against tomllib (~250k generated documents): no invalid document accepted, no new disagreements vs master.
…dation # Conflicts: # tests/test_toml_document.py # tomlkit/container.py
Closes #571.
Problem
Validation of out-of-order tables (a table whose parts are split by other tables) had one false rejection and three false acceptances:
_validate_table_candidatelooked up the existing value withitem(k), which returns anOutOfOrderTableProxyfor out-of-order keys. The proxy is not aTable, so the type check raisedKeyAlreadyPresentfor valid documents such as[a]\n[a.b.c]\n[a.b]\n[[zz]]\n[a.b.d].append()compared a concrete[table]only with the last fragment of an out-of-order key, e.g.[a]\n[q]\n[a.b.c.e]\n[a.b]\n[a]\n[a.b.d]([a]twice)._validation_cachehad already copied, so later fragments were validated against an outdated copy:a.x=1\n[a.b.c]\n[a.b.d]\n[q]\n[a.b.d]([a.b.d]twice).[a]\nb.y=1\n[a.b.c]\n[q]\n[a.b].Master rejected some invalid documents from cases 2 and 3 only by accident, through bug 1. Fixing bug 1 alone would have made them parse.
Fix
_validate_table_candidateiterates the raw stored fragments instead of materializing the proxy, applying the existing checks to each one, and rejects a concrete[table]over a dotted-key fragment._validate_earlier_fragments: a concrete[table]may not redefine an earlier concrete or dotted-key fragment. This is a cheap, non-recursive scan; nested collisions are left to the incremental validation.Verification
Differential fuzzing against
tomllib, about 250k generated documents mixing headers, arrays of tables and dotted keys:Parse time stays linear: 0.40s vs master's 0.36s on 2000 split tables. An intermediate version of this branch was quadratic; that is fixed.
12 new tests: valid nested out-of-order layouts and each rejection case above.
pytest: 1109 passed.ruff checkandruff format --checkare clean.The one failing integration job (Ubuntu / 3.12 / poetry) fails on Poetry's
test_extractall_sdist_no_symlink_path_traversal_via_directory_symlink(tarfile behavior of the runner's Python). It passes on macOS and Windows and is unrelated to this change.Agent Drafting Metadata
tomllib, found and fixed cases 2–4, and drafted this description.