Repository navigation
FIX: query and remove seeds by literal metadata keys - #3125
Chen Yufeiyang (feiiiiii5) wants to merge 3 commits into
Conversation
`prompt_metadata` is documented as a free-form dictionary, but the SQLite
and Azure SQL backends spelled each key into the JSON path and reused it
as the bind-parameter name:
JSON_EXTRACT(prompt_metadata, '$.{key}') = :{key}
Only `[A-Za-z0-9_]` survives that. Anything else raises before the query
runs, so the filter cannot be expressed at all:
ArgumentError: This text() construct doesn't define a bound
parameter named 'attack.id'
`get_message_pieces_async(prompt_metadata={"attack.id": "v"})` against
SQLiteMemory raises that today. A dot was also read as a path step, so a
key that happened to survive would have looked up a nested object.
Both backends now bind the path with the key quoted into it (`'$."{key}"'`)
and give each condition positional parameter names, matching how microsoft#3063
fixed the same interpolation in the label conditions:
JSON_EXTRACT(prompt_metadata, :pm_path_0) = :pm_value_0
Binding the path also keeps the value out of the statement text entirely.
Seed metadata filtering goes through the same two code paths
(`_get_seed_metadata_conditions`), so it is fixed as well.
Regression tests query four keys holding a dot, hyphen, space and colon,
plus one query naming three keys at once. Against the parent commit they
fail with the ArgumentError above; the plain-key cases the suite already
covered still pass.
Not addressed: a key containing a double quote cannot be expressed in a
SQLite or T-SQL JSON path, so it matches nothing rather than raising.
That is the same residual microsoft#3063 left in the label path.
Validation at this head: tests/unit/memory/memory_interface/,
tests/unit/memory/test_sqlite_memory.py, tests/unit/memory/test_azure_sql_memory.py
-- 648 passed, 1 skipped; ruff check and format clean.
Chen Yufeiyang (feiiiiii5)
changed the title
|
Thanks for reproducing the deletion case. Fixed in Added quote and backslash cases for prompt/seed retrieval, seed removal and group removal, each asserting that unrelated records remain. All eight fail on The PR description now reflects the literal-key implementation and removes the incorrect claim that quoted keys would simply match nothing. |
| alias = f"metadata_{index}" | ||
| json_conditions.append( | ||
| f"EXISTS (SELECT 1 FROM OPENJSON(prompt_metadata) AS [{alias}] " | ||
| f"WHERE [{alias}].[key] = :{key_param} AND [{alias}].[value] = :{value_param})" |
There was a problem hiding this comment.
🔴 Must Fix: SQL Server still treats different metadata keys as equal.
OPENJSON gives [key] a BIN2 collation, but SQL Server's = comparison still pads strings with spaces. That means "plain" and "plain " compare equal here. Two seeds with these different dictionaries would both match metadata={"plain": "v"}:
{"plain": "v"}
{"plain ": "v"}The same predicate backs prompt retrieval, seed retrieval, and seed/group removal, so this can remove the seed or group with the wrong key. Previously, JSON_VALUE(..., '$.plain') only selected the actual plain property. SQLite's new literal-key comparison does distinguish these keys.
Please keep the equality check and also compare the key lengths, using the same Unicode type on both sides. For example:
AND DATALENGTH([metadata_0].[key])
= DATALENGTH(CAST(:md_key_0 AS nvarchar(max)))Add coverage that distinguishes plain, plain , and an empty key from a space-only key, including removal. The current main branch's _get_scenario_role_match_condition uses the same equality-plus-length approach for exact strings.
I checked the actual generated MSSQL predicate and SQL Server's documented padding behavior, but did not run a live SQL Server. Am I missing an exact-length check elsewhere?
|
Thanks Roman Lutz (@romanlutz) — the shared Azure predicate was missing that length guard. Fixed in Added generated-MSSQL predicate checks for trailing spaces, empty versus space-only keys, Unicode and multiple filters; all three fail on The memory suites pass: 683 passed, 1 skipped. Ruff check/format passed. Live SQL Server was not run; the Azure regression checks inspect generated SQL, and the PR description now makes that limit explicit. |
This branch has not been deployed
2 participants
Description
Prompt and seed metadata accept free-form keys, but keys with dots, spaces or punctuation were used as bind-parameter names and JSON paths. Quoting paths is insufficient: quotes or backslashes inside a key can select a different record, including when deleting seeds.
Changes
SQLite compares literal keys through
json_each(...).key; Azure SQL comparesOPENJSONkeys. Azure also checks Unicode byte lengths, since SQL Server equality pads trailing spaces. Keys and values remain bound parameters; value comparison semantics are unchanged. Prompt retrieval, seed retrieval and seed/group removal share these filters. Related: #3063, which addressed label filters.Tests
Eight quote/backslash retrieval and removal regressions fail on
fab86809with unrelated records selected or removed. Three generated-MSSQL predicate tests fail on2efea640because exact-key length guards are absent. These inspect generated SQL, rather than executing SQL Server.SQLite database tests distinguish
plainfromplain, empty from space-only keys, and Unicode keys with trailing spaces, including removal. Those cases already pass on2efea640and pin the behavior Azure should preserve.Tests and Documentation
Memory-interface, SQLite and Azure unit suites: 683 passed, 1 skipped. Ruff check and format check passed on the three newly changed files. No docs changed; JupyText was not run.
Not addressed
Live Azure SQL integration was not run. Azure checks cover generated MSSQL predicates and literal bound keys; SQLite checks execute against a local database.