Skip to content

refactor(cache)!: collapse serialization onto ISerializerProxy<byte[]> - #145

Open
cosmin-staicu wants to merge 1 commit into
mainfrom
refactor/unify-serializer-seam
Open

refactor(cache)!: collapse serialization onto ISerializerProxy<byte[]>#145
cosmin-staicu wants to merge 1 commit into
mainfrom
refactor/unify-serializer-seam

Conversation

@cosmin-staicu

@cosmin-staicu cosmin-staicu commented Sep 2, 2026

Copy link
Copy Markdown
Member

Summary

ISerializerProxy<byte[]> — the seam #121 added for the IDistributedCache adapter — becomes the library's only serialization seam, and ISerializerProxy<RedisValue> is removed. The serialization contract no longer names a Redis type, so a custom serializer needs no StackExchange.Redis dependency and one registration covers ICache, IHashCache, ISetCache and the distributed adapter alike.

No stored cache entry changes format and nothing needs migrating. The default serializer emits byte-for-byte what SystemJsonSerializerProxy emitted; raw byte passthrough is scoped to the one call site that actually needs it.

Changes

  • One seam. RedisCache, RedisHashCache, RedisSetCache, the memory set tier and all four providers take ISerializerProxy<byte[]>; the broadcast change token is built as ChangeTokenFactory<byte[]>. SystemJsonSerializerProxy and the internal RedisValueSerializerProxy shim (which existed only to bridge the two seams) are deleted.

  • Two implementations, because raw passthrough turned out to be a property of one call site rather than of the seam:

    Byte payloads Everything else Wired at
    SystemJsonByteSerializerProxy (default) base64 in a JSON string UTF-8 JSON DI, TryAddSingleton
    RawByteSerializerProxy stored verbatim UTF-8 JSON the adapter's own construction site

    UiPathDistributedCache is byte[] end to end and encodes its own absexp/sldexp with Encoding.UTF8.GetBytes, so it never asks the serializer to handle anything but a byte[] payload. Those bytes are the caller's, already serialized by ASP.NET Core — base64-ing them costs a third again in size for nothing, and a consumer swapping in MessagePack should not have their session bytes re-encoded. AddDistributedCache therefore constructs RawByteSerializerProxy directly rather than resolving from DI, so replacing the app-wide serializer cannot reach into what the adapter stores in its data field.

  • MemorySetCache structural equality. The local snapshot was keyed on RedisValue, whose structural equality made Contains/Union/Except work; byte[] compares by reference, so the snapshot now carries a ByteArrayEqualityComparer. Without it every membership test misses — and MultilayerSetCache.ContainsItemAsync treats a populated local tier as authoritative, so the wrong answer is never corrected against the backing tier. No perf cost: RedisValue.GetHashCode already hashed the full payload.

  • Startup guard. AddCaching throws when a leftover ISerializerProxy<RedisValue> registration is present. Nothing resolves that service any more, so it would otherwise be ignored and the application would quietly serialize as JSON instead of with the custom serializer.

  • Docs (docs/how-to/extending.md): the "Custom serializer" section is rewritten around the byte seam, the two implementations are tabulated with the reason each exists, and the old "swap both registrations" caveat is gone.

Why the default keeps base64 for byte[]

Adopting raw passthrough app-wide was implemented first and rejected. It corrupts reads silently: Deserialize<byte[]> passes bytes through rather than parsing them, so a 1.x base64-in-JSON entry hands the caller the base64 ASCII — no exception, so nothing degrades it to a cache miss. Two fixes were considered and don't work:

  • Sniffing for a quoted base64 payload trades a deterministic wrong answer for a probabilistic one — arbitrary raw bytes can legitimately begin and end with 0x22 (~1 in 65536 at random, not rare at cache scale). It also reintroduces the format sniffing the design deliberately rejected.
  • A version header on the payload would break the Microsoft.Extensions.Caching.StackExchangeRedis-compatible layout the adapter's data field matches on purpose.

Scoping passthrough to the adapter removes the problem rather than detecting it. Registering RawByteSerializerProxy app-wide stays available as an opt-in, documented as a wire-format change requiring a keyspace relocation. LegacySerializerWireCompatTests pins both halves, including The_raw_proxy_would_misread_a_legacy_byte_payload_silently, so the reason for the split can't be refactored away.

On performance

The usual argument for keeping RedisValue is that it avoids materializing a byte[] on reads. Measured against StackExchange.Redis 3.1.13 before committing to the change:

RedisValue backing → ReadOnlyMemory<byte> → byte[]
byte[] (what a JSON payload read off the wire becomes) zero-alloc zero-alloc, same reference
string allocates allocates
long allocates allocates

The two conversions are allocation-identical on every backing, so SystemJsonSerializerProxy's apparent zero-copy read path costs nothing to give up.

BREAKING

Source-level, not data-level. Six shipped constructor signatures change their serializer parameter type (RedisCacheProvider, RedisSetCache, InMemoryQueueCacheProvider, InMemoryRedisQueueCacheProvider, RedisQueueCacheProvider, and the memory set tier), and SystemJsonSerializerProxy is gone. Consumers who pass these constructors a serializer directly, or who register a custom one, need a one-line type change — and the startup guard makes a missed one fail loudly.

One wire difference, deliberate: a null value now serializes to a null payload rather than the four-byte JSON null literal, following the seam's documented contract. Entries of either shape read back as default, and the caches normalize nulls before serializing, so it surfaces only for a deliberately-null value such as a null set member.

Test plan

  • Unit tests added/updated (+19: RawByteSerializerProxyTests, LegacySerializerWireCompatTests, SystemJsonByteSerializerProxyTests rewritten for JSON-everything, a set-cache test pinning that members match by serialized bytes rather than by reference, and two startup-guard tests)
  • Integration tests pass locally (dotnet test — 1464 passed, 0 failed, 10 skipped on both net8.0 and net10.0; build clean at the same 16 pre-existing CS0618 warnings as main)
  • CHANGELOG.md updated

Linked issues

N/A — follow-up to #121.

Contributor declaration

  • I signed off my commits per the DCO (git commit -s).
  • I am contributing on behalf of my employer, or in the course of employment / using employer resources. (If checked, your employer may hold IP rights in this work, which can require a signed CLA — a maintainer will follow up. See CONTRIBUTING.md.)

🤖 Generated with Claude Code

https://claude.ai/code/session_01HNxcCLCZd8sBVdSGdh5C23

@github-actions github-actions Bot added the needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md) label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🔎 Maintainer heads-up: automated triage flagged this PR as potentially material, so it may need a signed CLA in addition to the DCO sign-off.

Strong signals

  • adds public API surface (PublicAPI.Unshipped.txt in src/UiPath.Caching.Abstractions, src/UiPath.Caching.Queue, src/UiPath.Caching)

This is advisory only — the bot does not decide. Please judge against the CLA criteria (material, product-critical, patent-sensitive, corporate contributor, broad commercial use). Note that thresholds can be gamed by splitting PRs, so use your judgement.

  • If a CLA is needed → add the cla-required label (a contributor comment with signing steps is posted automatically).
  • If it is not needed → replace needs-cla-review with cla-not-required so later pushes don't re-flag it.

@cosmin-staicu
cosmin-staicu force-pushed the refactor/unify-serializer-seam branch from 8c3231f to 70eef47 Compare September 2, 2026 12:03
@cosmin-staicu cosmin-staicu added cla-not-required Maintainer reviewed: no CLA required for this contribution and removed needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md) labels Sep 2, 2026
@cosmin-staicu cosmin-staicu self-assigned this Sep 2, 2026
@cosmin-staicu
cosmin-staicu requested a balanced review from Copilot September 2, 2026 14:41
Copilot AI reviewed Sep 2, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Null Redis values and mutable byte-array set keys introduce correctness regressions, and serializer registration documentation is inconsistent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Consolidates caching serialization on ISerializerProxy<byte[]>, preserving the default wire format while introducing dedicated raw-byte handling.

Changes:

  • Migrates caches, providers, broadcasts, and DI to byte-array serialization.
  • Adds raw-byte serialization and structural set-member equality.
  • Updates compatibility tests, public APIs, documentation, and migration guards.
File summaries
File Description
tests/UiPath.Caching.Tests/SystemJsonSerializerProxyTests.cs Removes legacy serializer tests.
tests/UiPath.Caching.Tests/SystemJsonByteSerializerProxyTests.cs Tests JSON/base64 byte behavior.
tests/UiPath.Caching.Tests/SetCacheProviderTests.cs Uses the byte serializer.
tests/UiPath.Caching.Tests/Redis/RedisSetCacheTests.cs Migrates Redis set tests.
tests/UiPath.Caching.Tests/Redis/RedisHashCacheTests.cs Migrates Redis hash tests.
tests/UiPath.Caching.Tests/Redis/RedisCacheTests.cs Migrates Redis cache tests.
tests/UiPath.Caching.Tests/RawByteSerializerProxyTests.cs Tests raw-byte serialization.
tests/UiPath.Caching.Tests/MultilayerSetCacheTests.cs Migrates multilayer set tests.
tests/UiPath.Caching.Tests/LegacySerializerWireCompatTests.cs Verifies legacy wire compatibility.
tests/UiPath.Caching.Tests/InMemorySetCacheTests.cs Tests structural member equality.
tests/UiPath.Caching.Tests/InMemoryRedisCollectionExtensionsTests.cs Verifies byte change-token registration.
tests/UiPath.Caching.Tests/Distributed/RedisValueSerializerProxyTests.cs Removes obsolete adapter tests.
tests/UiPath.Caching.Tests/CachingBuilderTests.cs Tests serializer registration guards.
tests/UiPath.Caching.Tests/Broadcast/ChangeTokenTests.cs Migrates change-token serialization.
src/UiPath.Caching/SystemJsonSerializerProxy.cs Removes the Redis-specific serializer.
src/UiPath.Caching/Redis/RedisHashCache.cs Uses byte-array serialization.
src/UiPath.Caching/Redis/RedisCacheProvider.cs Changes the serializer dependency.
src/UiPath.Caching/Redis/RedisCache.cs Uses byte-array serialization.
src/UiPath.Caching/PublicAPI.Unshipped.txt Records core API changes.
src/UiPath.Caching/Distributed/RedisValueSerializerProxy.cs Removes the bridging serializer.
src/UiPath.Caching/Config/InMemoryRedisCollectionExtensions.cs Registers byte change tokens.
src/UiPath.Caching/Config/DistributedCacheCollectionExtensions.cs Wires raw-byte serialization.
src/UiPath.Caching/Config/CachingBuilder.cs Adds the legacy-registration guard.
src/UiPath.Caching.Queue/RedisSetCache.cs Migrates Redis set serialization.
src/UiPath.Caching.Queue/RedisQueueCacheProvider.cs Changes the serializer dependency.
src/UiPath.Caching.Queue/PublicAPI.Unshipped.txt Records queue API changes.
src/UiPath.Caching.Queue/MultilayerSetCache.cs Migrates the memory set tier.
src/UiPath.Caching.Queue/MemorySetCache.cs Adds structural byte-array sets.
src/UiPath.Caching.Queue/InMemoryRedisQueueCacheProvider.cs Changes the serializer dependency.
src/UiPath.Caching.Queue/InMemoryQueueCacheProvider.cs Changes the serializer dependency.
src/UiPath.Caching.Queue/GlobalUsings.cs Removes the global Redis import.
src/UiPath.Caching.Queue/Config/QueueCacheCollectionExtensions.cs Updates API documentation.
src/UiPath.Caching.Queue/ByteArrayEqualityComparer.cs Adds structural byte comparison.
src/UiPath.Caching.Azure/GlobalUsings.cs Removes the global Redis import.
src/UiPath.Caching.Azure/AzureEntraConnectionConfigurator.cs Adds its explicit Redis import.
src/UiPath.Caching.Abstractions/SystemJsonByteSerializerProxy.cs Makes JSON the default for bytes.
src/UiPath.Caching.Abstractions/RawByteSerializerProxy.cs Adds raw-byte passthrough.
src/UiPath.Caching.Abstractions/PublicAPI.Unshipped.txt Records abstraction API changes.
src/UiPath.Caching.Abstractions/Broadcast/IEventFormatterProxy.cs Removes obsolete string methods.
docs/how-to/extending.md Rewrites serializer guidance.
CHANGELOG.md Documents breaking changes.
Review details
  • Files reviewed: 41/41 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/UiPath.Caching.Abstractions/RawByteSerializerProxy.cs Outdated
Comment thread src/UiPath.Caching.Abstractions/SystemJsonByteSerializerProxy.cs Outdated
Comment thread src/UiPath.Caching.Queue/MemorySetCache.cs
Comment thread docs/how-to/extending.md Outdated
Comment thread docs/how-to/extending.md Outdated
@cosmin-staicu
cosmin-staicu force-pushed the refactor/unify-serializer-seam branch from 740503d to bde40a4 Compare September 2, 2026 14:55
cosmin-staicu added a commit that referenced this pull request Sep 2, 2026
@cosmin-staicu @claude
fix(cache): keep null payloads off the wire, own set snapshot members
cd5f8f5 
Addresses the Copilot review on #145. Two real defects, both verified.

Serialize(null) returned an actual null, which reaches StackExchange.Redis as
RedisValue.Null. Tested against Redis 7 with SE.Redis 3.1.13: SADD and the
multi-field HSET throw ArgumentException ("A null value is not valid in this
context"), SetContains throws, and the single-value HSET/SET paths silently
store nothing. So a null set member or a null hash field with CacheNullValues
off went from storing the legacy JSON null literal to throwing. Null now goes
through JSON like every other value, which is also byte-identical to 1.x.

MemorySetCache stored whatever the serializer returned, and
RawByteSerializerProxy returns the caller's own array. A caller mutating it
after the add would change an element's hash from inside the snapshot,
corrupting membership and removal. The store paths now clone; the lookup paths
still compare against the caller's array.

Two documentation claims were wrong and are corrected: replacing the app-wide
serializer does not reach the IDistributedCache adapter, which constructs its
own RawByteSerializerProxy; and the empty-buffer contract has an exception,
since RawByteSerializerProxy.Deserialize<byte[]> returns the empty array
because an empty payload is a value there rather than an absence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNxcCLCZd8sBVdSGdh5C23
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
@cosmin-staicu
cosmin-staicu force-pushed the refactor/unify-serializer-seam branch from cd5f8f5 to 7906193 Compare September 2, 2026 17:03
@cosmin-staicu @claude
refactor(cache)!: collapse serialization onto ISerializerProxy<byte[]>
1ea6b5b 
Every cache now serializes through the byte seam #121 introduced, and the
RedisValue one is gone. The serialization contract no longer names a Redis
type, so a custom serializer needs no StackExchange.Redis dependency and one
registration covers ICache, IHashCache and ISetCache. Deletes
SystemJsonSerializerProxy and the internal RedisValueSerializerProxy shim the
distributed cache needed to bridge the two.

Measured before committing to it: RedisValue -> ReadOnlyMemory<byte> and
RedisValue -> byte[] allocate identically on every RedisValue backing
(byte[]-backed is zero-copy and returns the same reference; string- and
integer-backed allocate either way), so the zero-copy read path
SystemJsonSerializerProxy appeared to buy costs nothing to give up.

Two implementations ship, because raw byte passthrough is a requirement of one
call site rather than of the seam:

  SystemJsonByteSerializerProxy (the DI default) - UTF-8 JSON for every value,
  byte-for-byte what SystemJsonSerializerProxy wrote. No stored entry changes
  format and nothing needs migrating, byte payloads and nulls included.

  RawByteSerializerProxy - byte payloads verbatim, JSON otherwise, and derives
  from the default so the shared TryDeserialize bodies live once and dispatch
  through the virtual Deserialize. AddDistributedCache constructs it for the
  provider it builds rather than resolving from DI, so replacing the app-wide
  serializer cannot change what the adapter stores: UiPathDistributedCache is
  byte[] end to end, encodes its own absexp/sldexp, and its payload is caller
  bytes ASP.NET Core has already serialized. Available as an app-wide opt-in,
  documented as a wire-format change needing a keyspace relocation.

Adopting raw passthrough as the default was tried and rejected: it silently
corrupted reads, because Deserialize<byte[]> passes bytes through rather than
parsing them, so a 1.x base64-in-JSON entry handed the caller base64 ASCII with
no exception and therefore no degradation to a cache miss. Sniffing for a
quoted base64 payload cannot fix that - arbitrary raw bytes can begin and end
with 0x22 - and a version header would break the MS-compatible layout the
adapter's data field matches.

Serialize(null) must not return null. Verified against Redis 7 with SE.Redis
3.1.13: RedisValue.Null makes SADD, the multi-field HSET and SetContains throw
ArgumentException, and the single-value HSET/SET paths silently store nothing.
Null therefore goes through JSON like every other value, which is also what 1.x
wrote.

MemorySetCache keyed its local snapshot on RedisValue, whose structural
equality made Contains/Union/Except work. byte[] compares by reference, so the
snapshot carries ByteArrayEqualityComparer -- without it every membership test
misses, and MultilayerSetCache.ContainsItemAsync treats a populated local tier
as authoritative and never corrects itself against the backing tier. Its store
paths also clone, since a passthrough serializer returns the caller's own array
and a later mutation would change an element's hash from inside the set.

AddCaching throws when a leftover ISerializerProxy<RedisValue> registration is
present. Nothing resolves that service any more, so it would otherwise be
ignored and the application would quietly serialize as JSON instead of with the
custom serializer.

Also drops IEventFormatterProxy<T>.Decode(string) and EncodeAsString(T),
obsolete since the broadcast path moved to ReadOnlyMemory<byte> and both dead,
and narrows the StackExchange.Redis global using in the Queue and Azure
projects to the one file in each that needs it.

Rebased onto main after #144: its TryAddAsync serializes through the same seam
now, and the RedisValue local it wrote has to be typed explicitly, because
EmptyString and the byte[] the serializer returns convert to one another, so a
var-typed conditional cannot pick between them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QeZZPMo8bFYz98AuPZU2V
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
@cosmin-staicu
cosmin-staicu force-pushed the refactor/unify-serializer-seam branch from 7906193 to 1ea6b5b Compare September 3, 2026 14:48
@sonarqubecloud

sonarqubecloud Bot commented Sep 3, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@litheon litheon Awaiting requested review from litheon litheon is a code owner
@cosminvlad cosminvlad Awaiting requested review from cosminvlad cosminvlad is a code owner
@alinahornet alinahornet Awaiting requested review from alinahornet alinahornet is a code owner
@lucianaparaschivei lucianaparaschivei Awaiting requested review from lucianaparaschivei lucianaparaschivei is a code owner
@razvalex razvalex Awaiting requested review from razvalex razvalex is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@cosmin-staicu cosmin-staicu

Labels

cla-not-required Maintainer reviewed: no CLA required for this contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cosmin-staicu