Repository navigation
fix(python-sdk): do not rewrite an empty namespace to the default (WALM-701) - #1265
harrymove-ctrl wants to merge 2 commits into
Conversation
Every call resolved its namespace with namespace or self._namespace, so
namespace="" was written to the default partition, while the TypeScript
SDK sends "" and the relayer rejects it. Use the client default only for
None and raise ValueError("namespace cannot be empty") for an empty or
blank string, before anything is signed. MemWalMock.recall gets the same
check; its store path already had it.
Fixes #1012
WALM-701
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@duccn could you review this one when you get a chance? Thanks! |
|
@ducnmm when you have time, could you review this? Tracked in WALM-701, CI is green. |
ducnmm
left a comment
ducnmm
left a comment
There was a problem hiding this comment.
Summary
Python no longer rewrites an explicit empty namespace to the client default. That closes the wrong-partition write in #1012 for "". The new check is wider than the relayer: str.strip() also rejects whitespace namespaces the server stores, and "" raises here instead of the TypeScript SDK's HTTP 400.
Issue counts by severity
- bugs: 1
- suggestions: 1
- nits: 0
| """ | ||
| if namespace is None: | ||
| return self._namespace | ||
| if not namespace.strip(): |
There was a problem hiding this comment.
[bug] not namespace.strip() rejects " ", "\t", "\n", and "\r". Those strings are truthy, so the old namespace or self._namespace sent them. validate_namespace on this base rejects only "" and a NUL byte, and it keeps tab, newline, and CR because those namespaces are already valid Postgres text. Rejecting them on the client blocks partitions the TypeScript SDK can still read and write (namespace ?? this.namespace). The same .strip() check is copied in memwal/mock.py (recall and _store), and tests/test_empty_namespace.py locks " " in as a rejection.
"" itself now raises ValueError and never hits the wire. The TypeScript SDK still sends "" and gets HTTP 400. WALM-701 asks for one empty-namespace semantic in both SDKs.
Suggestion: Treat only None as the default and send every other string, including "" and whitespace, so validate_namespace returns 400 for "". If "" must fail in the client, compare with == "" (do not strip) and apply that same error in the TypeScript SDK, with a short note next to the server rule. Drop " " from the rejection cases, and add a case that a whitespace namespace is sent unchanged. Point the mock at the same rule.
| def _resolve_namespace(self, namespace: Optional[str]) -> str: | ||
| """``None`` means the client default; "" is an error, not the default. | ||
|
|
||
| ``namespace or self._namespace`` silently wrote ``namespace=""`` to |
There was a problem hiding this comment.
[suggestion] This docstring retells the old namespace or self._namespace bug, the TypeScript wire shape, and #1012. It also says the relayer rejects a value this function never sends.
Suggestion: One line is enough: None uses the client default; do not treat "" as missing.
The relayer's validate_namespace refuses only "", a NUL byte and over-long names, and keeps " ", tab, newline and CR namespaces readable because they are valid Postgres text. The .strip() check in _resolve_namespace and the mock blocked partitions the TypeScript SDK (namespace ?? default) can still read and write. Only "" is rejected client-side now, with the same message the relayer and the TypeScript mock use; None still means the client default. Test doubles now accept the full _signed_request signature, the capture double answers remember and bulk-remember so every call path is covered, and new tests assert whitespace namespaces are sent unchanged. WALM-701
|
@ducnmm only |
This branch has not been deployed
2 participants
WALM-701
Fixes #1012
Only
Nonenow means "use the client namespace". An empty or blanknamespaceraisesValueError("namespace cannot be empty")before signing, instead of being rewritten to the default partition. This covers remember, remember_async, remember_bulk_async, recall (positional and params), analyze, ask, remember_manual and recall_manual.MemWalMock.recallgets the same check; its store path already had it.Tests: tests/test_empty_namespace.py, 25 cases (9 call paths x {'', ' '}, 6 None/explicit cases, 1 mock case). Before the fix 19 failed; after, all 25 pass. Full suite without integration tests: 279 passed. ruff clean.
Behaviour change: callers passing "" now get an error instead of a silent write to the default partition. The TS SDK sends "" and gets a 400 from the server, so both now fail closed.
SP: 2