Make OpaqueChange fallback opt-in and EntityType nullable - #91
Merged
Conversation
Unknown IChange $type handling is now configurable via HarmonyConfig.UnknownChangeHandling, defaulting to Throw. Callers opt into OpaqueChange fallback with UnknownChangeHandling.Fallback. OpaqueChange.EntityType now returns null instead of throwing, and IChange.EntityType is nullable to reflect that. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds configurable handling for unknown change ChangesUnknown change handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JsonSerializer
participant HarmonyConfig
participant PeekThenConcreteChangeConverter
participant OpaqueChange
JsonSerializer->>HarmonyConfig: configure JSON options
HarmonyConfig->>PeekThenConcreteChangeConverter: register handling policy
JsonSerializer->>PeekThenConcreteChangeConverter: deserialize unknown $type
alt Throw
PeekThenConcreteChangeConverter-->>JsonSerializer: JsonException
else Fallback
PeekThenConcreteChangeConverter->>OpaqueChange: preserve raw JSON
OpaqueChange-->>JsonSerializer: opaque IChange
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SIL.Harmony.Tests/ChangeConverterTests.cs`:
- Around line 12-17: Update SampleOptions to support creating
JsonSerializerOptions without explicitly setting
HarmonyConfig.UnknownChangeHandling, and use that unset-options path in the
default-policy test so it verifies the actual configured default. Keep passing
Fallback explicitly in tests that target fallback behavior, including the cases
around lines 53–63.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07ed0b66-e788-4c06-8534-bd70800148bd
📒 Files selected for processing (6)
src/SIL.Harmony.Tests/ChangeConverterTests.cssrc/SIL.Harmony/Changes/Change.cssrc/SIL.Harmony/Changes/OpaqueChange.cssrc/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cssrc/SIL.Harmony/Config/HarmonyConfig.cssrc/SIL.Harmony/Config/UnknownChangeHandling.cs
myieye
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
per our discussion we can now opt out of this new behavior as it requires work on the fwlite side before we can adopt it and we don't want to block upgrading.
There's also a new
HarmonyConfig.ConfigureExternalJsonOptionsto setup an external options object with whatever it needs to handle changes and objects.AI summary
Follow-up to #80 (OpaqueChange for unknown
IChange$type). Two refinements:1. Opaque fallback is now opt-in. New
UnknownChangeHandlingenum (Throw,Fallback) exposed asHarmonyConfig.UnknownChangeHandling. Default isThrow— an unknown$typenow raises aJsonExceptionduring deserialization unless the caller setsUnknownChangeHandling.Fallback, which restores the #80 behavior of preserving the payload as anOpaqueChange.UnknownChangeHandling.Fallback.2.
OpaqueChange.EntityTypeno longer throws. It returnsnull, since an opaque change genuinely has no known entity type.IChange.EntityTypeis nowType?to advertise this;Change<T>continues to returntypeof(T).Wiring: the handling mode flows from
HarmonyConfigintoPeekThenConcreteChangeConverter, which throws or falls back accordingly.SnapshotWorkeronly encountersOpaqueChangeinFallbackmode, so no other production code changed.Test plan
ChangeConverterTestsupdated so itsSampleOptionshelper opts intoFallbackfor the existing opaque round-trip tests.$typethrowsJsonExceptionwhen handling isThrow.OpaqueChange.EntityTypeisnull.dotnet test src/SIL.Harmony.Tests— all pass (the one failure,DataModelPerformanceTests.AddingChangePerformance, is a pre-existing debug-onlyAssert.Fail).Summary by CodeRabbit
UnknownChangeHandlingsetting to control what happens when an unrecognized change$typeis encountered during deserialization (Throwby default, orFallbackto preserve unknown changes as opaque data for round-tripping).JsonSerializerOptions.