Skip to content

testdrive: escape quotes and backslashes in rewrite_result#36488

Merged
def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-testdrive-quote
May 21, 2026
Merged

testdrive: escape quotes and backslashes in rewrite_result#36488
def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-testdrive-quote

Conversation

@def-
Copy link
Copy Markdown
Contributor

@def- def- commented May 10, 2026

Values containing whitespace combined with " or \ were wrapped in quotes without escaping, causing the parser to either fail with "unterminated quote" or silently corrupt expected values (e.g. \n inside a path becomes a newline). Use Debug formatting like TestdriveRow::fmt does, which escapes exactly what split_line expects.

@def- def- requested a review from bosconi May 10, 2026 15:17
@def- def- requested a review from a team as a code owner May 10, 2026 15:17
Values containing whitespace combined with `"` or `\` were wrapped in
quotes without escaping, causing the parser to either fail with
"unterminated quote" or silently corrupt expected values (e.g. `\n`
inside a path becomes a newline).

Introduce a small `escape_for_parser` helper that emits only the escape
sequences `parser::split_line` recognises (`\\`, `\"`, `\n`, `\t`, `\r`,
`\0`) and use it from both `rewrite_result` and `TestdriveRow::fmt`.
This avoids `format!("{:?}", …)`, whose Debug impl also emits `\u{N}`
for non-printable Unicode — escapes the parser does not understand and
would silently corrupt on the next read.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@def- def- force-pushed the pr-testdrive-quote branch from 9119d9f to bacbbc6 Compare May 11, 2026 05:38
Copy link
Copy Markdown
Member

@bosconi bosconi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@def- def- merged commit 226fb0c into MaterializeInc:main May 21, 2026
105 checks passed
@def- def- deleted the pr-testdrive-quote branch May 21, 2026 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants