fix(gql): collapse carriage returns when sanitizing GraphQL strings

[feiiiiii5]

Problem

_sanitize_str() can emit a GraphQL string literal containing a raw carriage return, which makes the whole query fail to parse server-side.

strip_newlines() only replaces U+000A:

def strip_newlines(s: str) -> str:
    return s.replace("\n", " ")

A "foo\r\nbar" value therefore becomes "foo bar" with a stray \r left in front of the space, and a lone "foo\rbar" is untouched. In the GraphQL grammar LineTerminator is LF or CR, and an unescaped LineTerminator is not a legal character inside a string literal — which is exactly why this function replaces newlines at all. CR simply never got the same treatment as LF.

Reachable through the ordinary filter path: any Where operand whose valueText / valueTextList / valueString value carries a carriage return goes through _sanitize_str (weaviate/gql/filter.py:692,695), as does an aggregate query (weaviate/gql/aggregate.py:46). Text that passed through a Windows newline is the common source.

Change

Collapse both line terminators, and CRLF to a single space rather than to "space + stray CR":

def strip_newlines(s: str) -> str:
    # GraphQL treats CR as a line terminator inside string literals just like LF,
    # so a CRLF value must collapse to one space instead of leaving a bare CR.
    return s.replace("\r\n", "\n").replace("\r", "\n").replace("\n", " ")

No new dependency, no change to any other character, and existing LF behaviour is byte-identical.

How this was verified

Regression cases added to the repo's own parametrized test_sanitize_str in test/test_util.py (that test previously pinned only the backslash/quote cases, nothing about line terminators).

Base control — weaviate/util.py restored from 142d798a93177215e68d44601c9653a856b3c373 with git diff --stat -- weaviate/util.py printing nothing, so the run measures upstream code while the new cases are kept:

$ python -m pytest test/test_util.py -q -p no:opik -k sanitize_str
2 failed, 8 passed, 34 deselected in 0.80s
FAILED test/test_util.py::test_sanitize_str[foo\r\nbar-foo bar]
FAILED test/test_util.py::test_sanitize_str[foo\rbar-foo bar]

With the patch applied:

$ python -m pytest test/test_util.py -q -p no:opik -k sanitize_str
10 passed, 34 deselected in 0.73s

Whole unit suite, base and head, to show the diff changes only what it claims:

base:  5 failed, 488 passed, 1 skipped     # 3 in test/test_timeout.py (pre-existing here) + my 2 new cases
head:  3 failed, 490 passed, 1 skipped     # only the 3 pre-existing test_timeout.py failures remain

The three test/test_timeout.py failures are present at base 142d798a unchanged, so they are not caused by this diff; they look environment-related (they spawn interpreters to test the timeout decorator). The mock suite passes: python -m pytest mock_tests -q → 65 passed.

Consequence, measured rather than asserted from the spec — building a real Weaviate-shaped query and lexing it with graphql-core (a spec-faithful parser, since I have no Weaviate server in this environment):

plain (control) -> parses OK
LF (control)    -> parses OK
CRLF            -> PARSE ERROR: Syntax Error: Unterminated string.
bare CR         -> PARSE ERROR: Syntax Error: Unterminated string.

I did not run the integration/compose suite, so the server-side rejection is demonstrated through the grammar and a spec parser rather than against a live Weaviate instance.

CLA: CONTRIBUTING.md requires a Contributor License Agreement for this repo; that is an account-owner action outside this diff.

[orca-security-eu]

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0   0   0   0	View in Orca
Passed	SAST	0   0   0   0	View in Orca
Passed	Secrets	0   0   0   0	View in Orca
Passed	Vulnerabilities	0   0   0   0	View in Orca

[weaviate-git-bot]

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Forum?

[feiiiiii5]

I have read and I agree with the Contributor License Agreement.

[feiiiiii5]

Bumping two at once rather than making you read them separately.

#2166 — client-side batching under-reports failures and misattributes them. The wholesale-failure handler in _BatchBase.__send_batch keys its error map by position within the chunk, but BatchObjectReturn.__add__ merges chunk results with a plain dict.update and never re-keys. With batch_size smaller than the object count, every chunk contributes keys 0..len(chunk)-1, so errors from later chunks overwrite earlier ones and the reported row indices are the wrong rows.

#2168 — \\r survives GraphQL string sanitization. sanitize_str strips newlines but leaves a bare carriage return in place, so a string containing one produces a query the server rejects.

The question on #2166 is the one I am least sure of: I fixed the reader by re-keying each chunk's errors against its offset in the batch. The alternative is to fix it at the point of construction, so an error map never contains chunk-local indices in the first place. The second is arguably less code and less prone to the same mistake appearing in the other batch helpers — there are a few more that build the same shape.

For #2168 there is nothing outstanding from me; it is a small, self-contained fix with the reproduction in the description.

If either is unwanted I would rather know and close it than leave it hanging.

[feiiiiii5]

Hi @weaviate maintainers — #2168 has been open since 26 September with CI green, so this is the one nudge I planned.

Recap: _sanitize_str() can emit a GraphQL string literal containing a raw carriage return. strip_newlines() replaces U+000A only, so "foo\r\nbar" becomes "foo " + "\r" + "bar" — a stray \r in front of the space — and a lone "foo\rbar" is untouched. In the GraphQL grammar a LineTerminator is LF or CR and an unescaped one is not legal inside a string literal, which is exactly why the function replaces newlines at all; CR just never got the same treatment. The fix collapses CR the same way LF already is.

One thing I checked rather than assumed, so you do not have to: the neighbour that escapes backslashes and quotes runs on the same string, so the only path to a raw CR reaching the server is this one — I grepped the package for other writers into the same literal. If you would rather CR be escaped rather than collapsed (\r as two characters), that changes what a round-tripped value looks like, which is why I matched the existing LF behaviour instead.

[g-despot]

Thanks for the fix and the tests, @feiiiiii5!

This sanitizer only runs on the GraphQL aggregate path, which the client uses only for Weaviate 1.27 and 1.28. From 1.29 on, aggregations go over gRPC and never touch this code. That path is also slated for removal once 1.29 becomes the minimum supported version which is happening with the next release.

Because of that, we're closing this one. If you hit this on a setup we're missing, or have another reason to merge it, please explain here and we'll reopen.

[feiiiiii5]

Understood — I won't push for a reopen.

One thing worth knowing when you remove the aggregate path: strip_newlines() lives in weaviate/util.py, and searching the repository turned up no caller outside _sanitize_str(), so the bare-CR behaviour doesn't travel with it elsewhere. That helper is the one to drop along with the path.