Skip to content

feat: protect final deserialization in PatchToDate with skip conditions#151

Open
hf-kklein wants to merge 2 commits intomainfrom
fix/protect-final-deserialization-with-skip-conditions
Open

feat: protect final deserialization in PatchToDate with skip conditions#151
hf-kklein wants to merge 2 commits intomainfrom
fix/protect-final-deserialization-with-skip-conditions

Conversation

@hf-kklein
Copy link
Contributor

Problem:
PatchToDate has two steps: (1) ApplyPatchesToDate applies patches one-by-one to a JToken — each protected by skip conditions, and (2) _deserialize() converts the accumulated JToken back to TEntity. Step 2 was NOT protected by skip conditions.

Symptom:
When the initial entity has a null navigation property (e.g. due to a missing EF Core Include), jsondiffpatch.net's Unpatch silently produces a garbled JToken — no error is thrown during patching. The configured skip conditions (e.g. IgnoreEverythingSkipCondition) are never invoked because no individual patch fails. The unprotected _deserialize() then crashes with a JsonException because the garbled JToken cannot be deserialized back into the target type (e.g. List at a path that became a non-array). This was discovered in production (DEV-107694).

Fix:

  • Wrap _deserialize() and _populateEntity() calls in both PatchToDate overloads with try-catch that consults skip conditions
  • When skip conditions allow skipping: return initialEntity as-is (the unpatched entity at +/- infinity) and set FinalDeserializationFailed = true
  • When no skip conditions match: rethrow as before (no behavior change)

Consequences for servers using this library:

  • Without this fix: _deserialize() throws → unhandled exception → HTTP 500.
  • With this fix: _deserialize() is caught → initialEntity is returned. The server no longer crashes, but the returned entity is the CURRENT state (at +/- infinity), NOT the state at the requested keyDate. This is fachlich incorrect — the caller gets the wrong time slice.
  • The entity cannot be partially rescued: System.Text.Json deserializes the entire object or not at all, even though only one property path (e.g. konfigurationsprodukte) is garbled while the rest was correctly unpatched.
  • Servers SHOULD check chain.FinalDeserializationFailed after PatchToDate and log a warning, because the returned data is silently wrong.
  • The correct fix remains on the consumer side: ensure all navigation properties are properly loaded (e.g. via EF Core Include) so that the Unpatch operates on valid arrays instead of null.

New API:

  • FinalDeserializationFailed (bool): true when the final deserialization was skipped. The returned entity is NOT the state at keyDate but the unpatched initial entity.
  • PatchesHaveBeenSkipped now also returns true when FinalDeserializationFailed is true.

BREAKING CHANGE: ISkipCondition.ShouldSkipPatch parameter failedPatch changed from TimeRangePatch to TimeRangePatch? (nullable). The parameter is null when the error occurred during final deserialization (no single patch failed). If your skip condition only inspects the exception type, just update the method signature.

Problem:
PatchToDate has two steps: (1) ApplyPatchesToDate applies patches
one-by-one to a JToken — each protected by skip conditions, and
(2) _deserialize() converts the accumulated JToken back to TEntity.
Step 2 was NOT protected by skip conditions.

Symptom:
When the initial entity has a null navigation property (e.g. due to a
missing EF Core Include), jsondiffpatch.net's Unpatch silently produces
a garbled JToken — no error is thrown during patching. The configured
skip conditions (e.g. IgnoreEverythingSkipCondition) are never invoked
because no individual patch fails. The unprotected _deserialize() then
crashes with a JsonException because the garbled JToken cannot be
deserialized back into the target type (e.g. List<T> at a path that
became a non-array). This was discovered in production (DEV-107694).

Fix:
- Wrap _deserialize() and _populateEntity() calls in both PatchToDate
  overloads with try-catch that consults skip conditions
- When skip conditions allow skipping: return initialEntity as-is
  (the unpatched entity at +/- infinity) and set
  FinalDeserializationFailed = true
- When no skip conditions match: rethrow as before (no behavior change)

Consequences for servers using this library:
- Without this fix: _deserialize() throws → unhandled exception → HTTP 500.
- With this fix: _deserialize() is caught → initialEntity is returned.
  The server no longer crashes, but the returned entity is the CURRENT
  state (at +/- infinity), NOT the state at the requested keyDate.
  This is fachlich incorrect — the caller gets the wrong time slice.
- The entity cannot be partially rescued: System.Text.Json deserializes
  the entire object or not at all, even though only one property path
  (e.g. konfigurationsprodukte) is garbled while the rest was correctly
  unpatched.
- Servers SHOULD check chain.FinalDeserializationFailed after PatchToDate
  and log a warning, because the returned data is silently wrong.
- The correct fix remains on the consumer side: ensure all navigation
  properties are properly loaded (e.g. via EF Core Include) so that
  the Unpatch operates on valid arrays instead of null.

New API:
- FinalDeserializationFailed (bool): true when the final deserialization
  was skipped. The returned entity is NOT the state at keyDate but the
  unpatched initial entity.
- PatchesHaveBeenSkipped now also returns true when
  FinalDeserializationFailed is true.

BREAKING CHANGE: ISkipCondition<TEntity>.ShouldSkipPatch parameter
failedPatch changed from TimeRangePatch to TimeRangePatch? (nullable).
The parameter is null when the error occurred during final
deserialization (no single patch failed). If your skip condition only
inspects the exception type, just update the method signature.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hf-kklein hf-kklein marked this pull request as ready for review February 20, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants