Skip to content

Commit 0ccf783

Browse files
ausfeldtREDMOND\gaausfelCopilotFabianMeiswinkelNaluTripician
authored
Fix KeyError: 'version' in SessionContainer.get_session_token (#47798)
* [Cosmos] Fix KeyError: 'version' in SessionContainer.get_session_token The sync and async `SessionContainer.get_session_token` accessed `collection_pk_definition['version']` with bracket notation. When the service-returned `partitionKey` payload omits the optional `version` field (common for containers created without an explicit V2 partition key), this raised `KeyError`, which was silently swallowed by the surrounding `except (KeyError, AttributeError)`. The net effect was that `get_session_token` returned an empty string, so the client sent no `x-ms-session-token` header on the next read. Against the Dedicated Gateway / Integrated Cache, every Session-consistency read was rejected as a cache miss (verified at the server with `ComputeRequest5M.CacheHitLevel`: 695 reads / 0 CacheFullHit pre-fix vs 678 reads / 677 CacheFullHit post-fix on the same item). Treating `version` as optional and passing `None` to `PartitionKey` when it is missing matches the existing `PartitionKey` constructor contract and restores Session-consistency cache hits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [Cosmos] Add regression tests for partitionKey without 'version' Covers SessionContainer.get_session_token in tests/test_session_token_unit.py: test_partitionkey_definition_without_version_returns_token Mirrors the live failure: container_properties_cache holds a partitionKey definition without 'version' (as the service returns it for containers created without explicit V2 partition-key versioning). Asserts a real per-partition token is returned (was '' before the fix, because the KeyError was silently swallowed). test_partitionkey_definition_with_version_returns_same_token Pins the no-regression case: when 'version' IS present, behavior is unchanged. Verified by reverting the fix locally and observing that the without_version test fails with AssertionError: '' != '0:1#100'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Nalu Tripician <27316859+NaluTripician@users.noreply.github.com> * Fix KeyError in SessionContainer for optional version Updated the default value of `partitionKey.version` to 1 in `SessionContainer.get_session_token` to match `PartitionKey` behavior. * Strengthen regression tests to assert V1 EPK The prior tests used a routing-map stub that discarded the ranges argument and returned a fixed partition range regardless of the computed effective partition key, so a wrong-EPK regression (e.g. version=None falling through to the raw-binary encoding) would still have passed the assertions on the returned session token. Replace the stub with a capturing variant that records what get_overlapping_ranges received, and add two tests: test_missing_version_produces_v1_hash_epk_not_raw_binary Computes the V1-hash EPK independently and asserts the captured EPK matches. Fails if the fix ever regresses to version=None (the actual EPK would be the raw-binary encoding). test_v1_and_v2_produce_different_epks Sanity check: V1 and V2 must produce distinct EPKs, otherwise the V1-EPK assertion above becomes vacuous. Verified by temporarily reverting to .get('version') (no default) and observing the new EPK test fails with AssertionError comparing the raw-binary encoding against the expected V1 hash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update sdk/cosmos/azure-cosmos/CHANGELOG.md Co-authored-by: Simon Moreno <30335873+simorenoh@users.noreply.github.com> * Rename test to spell out effective partition keys (fix cspell) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Re-trigger CI (emulator collection-count flake) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: REDMOND\gaausfel <gaausfel@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Fabian Meiswinkel <fabianm@microsoft.com> Co-authored-by: Nalu Tripician <27316859+NaluTripician@users.noreply.github.com> Co-authored-by: Simon Moreno <30335873+simorenoh@users.noreply.github.com>
1 parent 7ed6bad commit 0ccf783

3 files changed

Lines changed: 149 additions & 2 deletions

File tree

sdk/cosmos/azure-cosmos/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#### Breaking Changes
99

1010
#### Bugs Fixed
11+
* Fixed `KeyError: 'version'` in `SessionContainer.get_session_token` (sync and async) when the container's `partitionKey` definition returned by the service does not include the optional `version` field. The error was silently swallowed by a broad `except`, causing the client to send no `x-ms-session-token` header on subsequent reads. Against the Dedicated Gateway, this turned every Session-consistency read into an Integrated Cache miss. `partitionKey.version` is now treated as optional and defaults to `1`, matching how `PartitionKey` handles a missing version. See [PR 47143](https://github.com/Azure/azure-sdk-for-python/pull/47143)
1112

1213
#### Other Changes
1314

sdk/cosmos/azure-cosmos/azure/cosmos/_session.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ def get_session_token(
119119
collection_pk_definition = container_properties_cache[collection_name]["partitionKey"]
120120
partition_key = PartitionKey(path=collection_pk_definition['paths'],
121121
kind=collection_pk_definition['kind'],
122-
version=collection_pk_definition['version'])
122+
version=collection_pk_definition.get('version', 1))
123123
epk_range = partition_key._get_epk_range_for_partition_key(pk_value=pk_value)
124124
pk_range = routing_map_provider.get_overlapping_ranges(collection_name,
125125
[epk_range],
@@ -213,7 +213,7 @@ async def get_session_token_async(
213213
collection_pk_definition = container_properties_cache[collection_name]["partitionKey"]
214214
partition_key = PartitionKey(path=collection_pk_definition['paths'],
215215
kind=collection_pk_definition['kind'],
216-
version=collection_pk_definition['version'])
216+
version=collection_pk_definition.get('version', 1))
217217
epk_range = partition_key._get_epk_range_for_partition_key(pk_value=pk_value)
218218
pk_range = await routing_map_provider.get_overlapping_ranges(collection_name,
219219
[epk_range],

sdk/cosmos/azure-cosmos/tests/test_session_token_unit.py

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,152 @@ def __init__(self, t):
321321
self.assertEqual(result, token.session_token)
322322

323323

324+
class TestGetSessionTokenWithoutPartitionKeyVersion(unittest.TestCase):
325+
326+
COLLECTION_LINK = "dbs/db1/colls/c1"
327+
COLLECTION_RID = "abc123=="
328+
PK_RANGE_ID = "0"
329+
PK_VALUE = "some-pk-value"
330+
RAW_TOKEN = "1#100"
331+
332+
class _CapturingRoutingMapProvider:
333+
"""Records the ranges argument passed to get_overlapping_ranges so tests
334+
can assert that get_session_token computed the correct effective partition
335+
key. Returns a single fixed partition range."""
336+
337+
def __init__(self, pk_range_id):
338+
self._pk_range_id = pk_range_id
339+
self.captured_ranges = None
340+
341+
def get_overlapping_ranges(self, collection_link, ranges, options):
342+
del collection_link, options
343+
self.captured_ranges = list(ranges)
344+
return [{
345+
"id": self._pk_range_id,
346+
"minInclusive": "",
347+
"maxExclusive": "FF",
348+
"parents": [],
349+
}]
350+
351+
class _TokenWrap:
352+
"""Mimics what ``SessionContainer.rid_to_session_token`` stores per
353+
partition: an object exposing ``.session_token`` as the string form."""
354+
355+
def __init__(self, vector_session_token):
356+
self.session_token = vector_session_token.session_token
357+
358+
def _build_container_with_seeded_token(self):
359+
container = _session.SessionContainer()
360+
container.collection_name_to_rid[self.COLLECTION_LINK] = self.COLLECTION_RID
361+
container.rid_to_session_token[self.COLLECTION_RID] = {
362+
self.PK_RANGE_ID: self._TokenWrap(VectorSessionToken.create(self.RAW_TOKEN)),
363+
}
364+
return container
365+
366+
def _build_cache(self, partition_key_definition):
367+
return {
368+
self.COLLECTION_LINK: {
369+
"_rid": self.COLLECTION_RID,
370+
"partitionKey": partition_key_definition,
371+
},
372+
}
373+
374+
def test_partitionkey_definition_without_version_returns_token(self):
375+
"""Service responses without a 'version' key must not silently nuke the token."""
376+
container = self._build_container_with_seeded_token()
377+
cache = self._build_cache({"paths": ["/pk"], "kind": "Hash"}) # no 'version'
378+
379+
token = container.get_session_token(
380+
resource_path=self.COLLECTION_LINK,
381+
pk_value=self.PK_VALUE,
382+
container_properties_cache=cache,
383+
routing_map_provider=self._CapturingRoutingMapProvider(self.PK_RANGE_ID),
384+
partition_key_range_id=None,
385+
options={},
386+
)
387+
388+
self.assertEqual(
389+
token, "{0}:{1}".format(self.PK_RANGE_ID, self.RAW_TOKEN),
390+
"Expected a non-empty session token when partitionKey lacks 'version'."
391+
)
392+
393+
def test_partitionkey_definition_with_version_returns_same_token(self):
394+
"""When 'version' IS present, behavior must be unchanged."""
395+
container = self._build_container_with_seeded_token()
396+
cache = self._build_cache({"paths": ["/pk"], "kind": "Hash", "version": 2})
397+
398+
token = container.get_session_token(
399+
resource_path=self.COLLECTION_LINK,
400+
pk_value=self.PK_VALUE,
401+
container_properties_cache=cache,
402+
routing_map_provider=self._CapturingRoutingMapProvider(self.PK_RANGE_ID),
403+
partition_key_range_id=None,
404+
options={},
405+
)
406+
407+
self.assertEqual(
408+
token, "{0}:{1}".format(self.PK_RANGE_ID, self.RAW_TOKEN),
409+
"Expected an unchanged session token when partitionKey includes 'version'."
410+
)
411+
412+
def test_missing_version_produces_v1_hash_epk_not_raw_binary(self):
413+
"""Missing 'version' must default to V1 hashing (matching the SDK's
414+
_get_partition_key_from_partition_key_definition convention). If
415+
version resolves to None, PartitionKey.__init__ stores None
416+
(kwargs.get('version', V2) returns None when the key is present) and
417+
_get_hashed_partition_key_string matches neither V1 nor V2 — the EPK
418+
becomes the raw-binary encoding and targets the wrong physical
419+
partition on multi-partition Hash containers."""
420+
from azure.cosmos.partition_key import PartitionKey
421+
expected_epk = PartitionKey(
422+
path=["/pk"], kind="Hash", version=1
423+
)._get_epk_range_for_partition_key(pk_value=self.PK_VALUE)
424+
425+
container = self._build_container_with_seeded_token()
426+
cache = self._build_cache({"paths": ["/pk"], "kind": "Hash"}) # no 'version'
427+
capturing = self._CapturingRoutingMapProvider(self.PK_RANGE_ID)
428+
429+
container.get_session_token(
430+
resource_path=self.COLLECTION_LINK,
431+
pk_value=self.PK_VALUE,
432+
container_properties_cache=cache,
433+
routing_map_provider=capturing,
434+
partition_key_range_id=None,
435+
options={},
436+
)
437+
438+
self.assertIsNotNone(
439+
capturing.captured_ranges,
440+
"get_overlapping_ranges was never called by get_session_token.",
441+
)
442+
self.assertEqual(len(capturing.captured_ranges), 1)
443+
actual_epk = capturing.captured_ranges[0]
444+
self.assertEqual(
445+
actual_epk.min, expected_epk.min,
446+
"Missing 'version' must default to V1 hashing. If the actual EPK "
447+
"differs from the expected V1 hash, the fix regressed to "
448+
"version=None and produced the raw-binary encoding instead.",
449+
)
450+
self.assertEqual(actual_epk.max, expected_epk.max)
451+
452+
def test_v1_and_v2_produce_different_effective_partition_keys(self):
453+
"""Sanity check: V1 and V2 hashing produce distinct EPKs for the same
454+
pk_value. If this ever regresses to equal, the V1-EPK assertion in
455+
test_missing_version_produces_v1_hash_epk_not_raw_binary becomes vacuous."""
456+
from azure.cosmos.partition_key import PartitionKey
457+
v1_epk = PartitionKey(
458+
path=["/pk"], kind="Hash", version=1
459+
)._get_epk_range_for_partition_key(pk_value=self.PK_VALUE)
460+
v2_epk = PartitionKey(
461+
path=["/pk"], kind="Hash", version=2
462+
)._get_epk_range_for_partition_key(pk_value=self.PK_VALUE)
463+
self.assertNotEqual(
464+
v1_epk.min, v2_epk.min,
465+
"V1 and V2 must produce distinct EPKs — if identical, the "
466+
"'version' parameter no longer influences hashing.",
467+
)
468+
469+
324470
# Unit tests for set_session_token_header. When a request targets a single
325471
# partition, the helper must send only that partition's token, not a
326472
# comma-joined token covering every cached partition.

0 commit comments

Comments
 (0)