Skip to content

fix(import): check the numbers in each ImportTable row as the service does - #804

Open
LeeroyHannigan wants to merge 1 commit into
fix/618-number-validation-all-opsfrom
fix/import-table-numbers
Open

LeeroyHannigan wants to merge 1 commit into
fix/618-number-validation-all-opsfrom
fix/import-table-numbers

Conversation

@LeeroyHannigan

Copy link
Copy Markdown
Collaborator

Stacked on #620, so it merges after #620 and #619.

ImportTable stored the numbers in each row as written, so a row could store 1E126, abc or 1E999999999 as an N value. Arithmetic on such a value is unbounded until #803 merges, and the SQLite sort-key encoder assumes every number is in range.

This change checks every number in a row before the other row checks, as the service does:

  • A row holding a value that is not a number ends the import of the file. The row counts as one error, the rows before it stay imported, and the rows after it are not processed.
  • A row holding a number out of range, or with more than 38 digits, is skipped as one error, and the rows after it are imported.
  • Numbers are read as BatchWriteItem reads them, with a 32-bit exponent and scale, so 1E2147483648 is not a number and 1E2147483647 is a Number underflow.
  • An import that refused any row answers ImportStatus FAILED with FailureCode ItemValidationError and keeps the rows it imported. Before, it answered COMPLETED with an error count. The service's FailureMessage sends the reader to its error log in CloudWatch; ExtendDB's names the server log.
  • Each refused row is logged with the import ARN, the table name and its item number in the file, so the lines of two imports running at once can be told apart. The ARN is now created before the rows are read. These lines write the ARN, the table name and the error text escaped, since ImportTable does not check the table name; the access log line and the SQLite table worker still write it as given (fix(engine): escape the table name and path in the import/export access log #772 escapes the access log, and ImportTable creates a table under a name CreateTable refuses #771 tracks checking the name as CreateTable does).

Measured on DynamoDB in us-east-1 on 2026-11-03, one import from S3 per case, each file holding a valid row, the row under test and another valid row. The mixed cases (a number out of range and a value that is not a number in one row, a row with no key, a five-row file) were measured the same way.

Two values are still read differently, and both are recorded in docs/differences-from-dynamodb.md. A zero whose exponent fits in 32 bits but is out of range, such as 0E126, is imported as 0, where the service skips the row. A Unicode decimal digit (\uff15), which the service imports as 5, ends the import of the file, since no number check in ExtendDB reads Unicode digits. The comment and test in number.rs that hold back range checking those zeros now name only Query and Scan (#544, #562).

The file is still parsed before any row is imported, so a JSON error after the row that ends the import fails the request, where the service would stop before reading it. That case was not measured.

Tests: unit tests for import_row in operations/tests.rs, and TestImportTableNumbers in tests/test_import_export.py, which asserts the measured counts, rows and status. Both pass on PostgreSQL and SQLite. With the row check removed, 21 of the 22 import tests fail; the one that passes imports only valid numbers.

… does

ImportTable stored a row's numbers as written. Each row's numbers are now
read as BatchWriteItem reads them, before the other row checks: a value
that is not a number ends the import of the file, and a number out of
range skips the row. Either counts as one error, and an import with any
error answers FAILED with ItemValidationError and keeps the rows it
imported, as measured on DynamoDB on 2026-11-03.

Each refused row is logged with the import ARN, the table name and its
item number, with the error text escaped.

A zero with an out-of-range exponent and a Unicode decimal digit are still
read differently from the service; both are recorded in
docs/differences-from-dynamodb.md.
@LeeroyHannigan LeeroyHannigan added the v1 Needed for the 1.0 release label Nov 3, 2026
@navareamol navareamol added security release-blocker Must ship for its milestone's release labels Nov 3, 2026
@navareamol navareamol added this to the 1.0 milestone Nov 3, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-blocker Must ship for its milestone's release security v1 Needed for the 1.0 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants