fix(backfill)!: 4.0.0 — the defects six reviews found - #29
Merged
Merged
Conversation
… API sends Porting this command to the JavaScript SDK and diffing the two outputs over the same park found nine defects. Two independent implementations reading one API disagree in exactly the places one of them is wrong. EPCOT's full archive now comes back byte for byte identical from both: 73,522 rows, 41 columns, the only differences being today's row, which grows as the day elapses. The resume checkpoint was the newest row written. The page it came from covered further, because an entity that stopped reporting has no rows for the tail days, so a rerun re-fetched a day already in the file and appended every row of it again. That breaks the (entityId, date) key the file is documented to have, and it happens on the exit-75 path, which is the ordinary path for a long back fill. The checkpoint is now the day the server's own `next` URL starts on, reported through a new `on_page` hook that fires once a page's rows are all written. The CSV was missing ten of the thirty-six fields the API sends, on every row: unknownMinutes, the whole inParkHours block, extremeWaits, and three of singleRider's five percentiles while standby had all five. 37,352 of 73,522 rows in a five-year EPCOT export were missing their in-park statistics. The vendored models were stale as well, and pydantic drops what it does not declare, so those fields were being deleted at parse time for every caller of days(), not just for the CSV. Models regenerated; the column list is now derived from the model, so regenerating is the whole fix next time. Also: UTC written as Z rather than +00:00 and CSV line endings as LF, which between them accounted for 39,201 differing lines against the JavaScript output; one park's failure no longer abandons the rest of a destination; a failed park no longer leaves a 0-byte file that reads as "no history"; a network failure or Ctrl-C is a sentence rather than a traceback; the user agent names both the command's and the SDK's real version instead of a hardcoded 1; --list reports the destination total rather than the filtered count; and an ambiguous exact name lists the parks that actually match it. Tests: 299 passing, and the fixtures are two real consecutive pages of one request where the newest row, the page's last day and the resume point are three different dates, so a test cannot confuse them by accident. Every hand-written stand-in is now checked against the real method's signature, because a stub that had drifted is how the 403 handler shipped as a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…up with Pydantic drops undeclared fields by default. This spec trails the API by days at a time, and in that window the SDK was deleting real data at parse time -- unknownMinutes, inParkHours and extremeWaits were gone before any caller saw them, with nothing failing and nothing warning. The TypeScript SDK never had the problem because its types are erased at runtime, so the same stale schema cost it nothing but autocompletion. Every generated model now inherits themeparks._models_base.ApiModel, whose only job is extra="allow". A field the API adds tomorrow survives parsing, reaches model_dump(), and reaches anyone writing rows to a file. Reading it from typed code still wants a regeneration, which the nightly drift job already opens a PR for; losing it in the meantime did not have to be the default. The generator's own invariant check caught the base-class change immediately and refused to ship the models, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…header BREAKING: the CSV header gained fifteen columns and the order is now the schema's, so a reader that takes columns by position gets the wrong ones rather than an error. Read by name. The library API is unchanged. 3.3.0 is yanked. It wrote the wrong park name into every row of a paid export: a name matching one park by substring returned the formatted display label, so parkName read "Magic Kingdom Park (Walt Disney World® Resort)" for all ~94,000 rows, and four live names reach that path. The rest, each of which loses or corrupts data: A failure on a resumed run deleted every row already downloaded, because `written == 0` means this process wrote nothing rather than the file is empty. The state file survived pointing mid-archive, so the next run appended only the tail and recorded complete. A window that closes under a resumed run -- a key rotated out of a scheduler's environment, a lapsed subscription -- did the same and exited 0, so the scheduler logged success, and then trapped: the file gone, the state surviving, every later run re-entering the branch. Resuming across versions, formats or SDKs corrupted the file. One state file served both formats, so ndjson -> csv -> ndjson doubled every row in the first; nothing recorded the header, so 3.3.0's 19-column file resumed under this build appended 41-field rows beneath it; and the two SDKs' keys differed only on the interrupted path, so the safe paths interoperated silently. The state file is now <parkId>.<format>.backfill-state.json carrying the SDK, its version, a state version and a fingerprint of the exact header, and refuses anything that does not match. A collection of nested models would have produced phantom columns and then an AttributeError on the first row; duplicate column names are now impossible at import. The NDJSON identity columns could be overwritten by the row once models kept undeclared fields. A network failure or timeout exits 75 now, so a scheduler retries, and anything the API rejected still exits 1 -- the JavaScript SDK had these the other way round. The CSV carries a UTF-8 BOM so Excel stops mangling ® and accents, and a cell a spreadsheet would execute is prefixed with an apostrophe; numeric cells are left alone, so a negative number stays a number. Both SDKs use one strict numeric pattern, because float() and Number() disagree about "\t5". On the tests, which is where this got uncomfortable. The test named for the headline fix asserted only that the run completed, and mutating the checkpoint to record the newest row -- the exact defect -- left all 301 green. The test whose docstring called it THE DRIFT GATE computed both sides of its assertion with the production helper, so dropping 30 of 36 columns still passed it. And destinations_slice.json, documented at length as the oracle for the ®, apostrophe and accent cases, was referenced by no test at all while the resolution tests used catalogues invented in the test file. All three are replaced: the checkpoint test reads the state file after a real page boundary, the drift gate flattens 108 real captured rows, and resolution runs against the real 127-park catalogue. tests/fixtures/csv_contract.json is asserted by both SDKs so the two headers cannot diverge again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cubehouse
changed the title
…0 do not CI caught this on the two oldest supported interpreters while a 3.12 venv passed. `csv.DictWriter` with QUOTE_MINIMAL only quotes characters that appear in the line terminator, and this command sets LF, so a bare CR in an entity name went through unquoted: one row parsed as two, every later column shifted. Python 3.11 changed the module to always quote CR and LF, which is why it looked fine locally and was live on 3.9 and 3.10 -- the same defect that was just fixed in the JavaScript SDK, arriving by a different door. Depending on stdlib behaviour that moved between 3.10 and 3.11 cannot produce a file that is byte-identical across Python versions, let alone identical to the JavaScript SDK's, so the writer now does its own minimal quoting: quote when the cell contains a quote, comma, CR or LF, and double an embedded quote. Ten lines, and the same rule the JavaScript SDK applies. The test also read the file with `splitlines()`, which splits on a bare CR whether or not it is inside quotes -- so it was testing a string method rather than the file. It now reads through `csv.reader` over the open file and asserts the quoted bytes. Verified: 332 tests on 3.9, 3.10 and 3.12, and EPCOT's archive is still byte-identical between the two SDKs bar today's four rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
themeparks 3.3.0should be yanked: it writes the wrong park name into every row of a paid export. This is the fix, as 4.0.0.Why a major
The CSV header gained fifteen columns and the order is now the schema's, so a reader taking columns by position gets the wrong ones rather than an error. Read by name. The library API is backward compatible.
What was wrong
Porting this command to the JavaScript SDK, diffing both outputs over the same park, and then reviewing the result from six angles found around twenty defects. Magic Kingdom's full archive now comes back byte for byte identical from both SDKs: 94,223 rows, 41 columns, the only differences being today's row, which grows as the day elapses.
The ones that lose or corrupt data:
themeparks-backfill "magic kingdom"wroteparkNameasMagic Kingdom Park (Walt Disney World® Resort)— the formatted display label — for all ~94,000 rows. Four live names reach that path. Live in 3.3.0.unknownMinutes, all ofinParkHours,extremeWaits, and three ofsingleRider's five percentiles. 72,200 of 94,223 rows were missing their in-park statistics. Live in 3.3.0.days(), not just for the CSV.written == 0means "this process wrote nothing", not "the file is empty". A closed window on a resumed run did the same and exited 0, so a scheduler logged success — then trapped permanently.Plus: a collection of nested models would have produced phantom columns and an
AttributeError; the NDJSON identity columns could be overwritten by the row; network failures exited 1 where the JS SDK exited 75;--listmisreported a destination's total; an ambiguous name listed the wrong candidates.New
State file is
<parkId>.<format>.backfill-state.jsoncarrying the SDK, its version, a state version and a header fingerprint — refusing anything that does not match. UTF-8 BOM so Excel stops mangling®. Formula-leading cells prefixed, numeric cells untouched.on_pageondays()/days_with_entities().--version.EntityRefandHistoryPageexported.The tests, which is where this got uncomfortable
THE DRIFT GATEcomputed both sides of its assertion with the production helper. Dropping 30 of 36 columns still passed it.destinations_slice.json, documented at length as the oracle for the®, apostrophe and accent cases, was referenced by no test; the resolution tests used catalogues invented in the test file.All three replaced. 332 tests (from 301), and 12 of 12 targeted mutations killed, including every one a reviewer found surviving.
tests/fixtures/csv_contract.jsonis asserted by both SDKs so the two headers cannot diverge again.Companion: ThemeParks_JavaScript#51.
🤖 Generated with Claude Code