Fix JsonElementStringConverter non-generic IConverter swapped casts (#175) - #177
Merged
chullybun merged 1 commit intoAug 3, 2026
Merged
Conversation
…175) - Add the two missing IConverter.ConvertToDestination(object?)/ConvertToSource(object?) default interface implementations to IConverter<TSource,TDestination>, making the interface hierarchy actually self-sufficient (previously only satisfied by each struct's own hand-rolled, copy/pasted overrides). - Delete the redundant hand-rolled non-generic overrides from JsonElementStringConverter (the actual bug - casts were swapped, causing InvalidCastException/StackOverflowException), StringBase64Converter, EncodedStringToUInt32Converter, and TypeToJsonStringConverter<T> (all already correct, but duplicated the same bug-prone pattern). - Add regression tests exercising the non-generic IConverter path for all four converters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a defect in the non-generic IConverter object-based conversion path by moving the object-cast logic into IConverter<TSource, TDestination> default interface implementations and removing the duplicated hand-rolled overrides from the converter structs. This prevents swapped-cast regressions (like the one in JsonElementStringConverter) and adds regression coverage for the non-generic IConverter route.
Changes:
- Added missing default interface implementations on
IConverter<TSource, TDestination>forIConverter.ConvertToDestination(object?)andIConverter.ConvertToSource(object?). - Removed redundant non-generic
object?overload implementations from affected converter structs, relying on the interface defaults instead. - Added/extended unit tests to explicitly exercise conversions via the non-generic
IConverterinterface for the affected converters.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreEx.Test.Unit/Mapping/Converters/StringToBase64ConverterTests.cs | Adds regression tests that call conversion through the non-generic IConverter interface. |
| tests/CoreEx.Test.Unit/Mapping/Converters/JsonElementStringConverterTests.cs | New test suite including explicit coverage for non-generic IConverter conversions (regression for #175). |
| tests/CoreEx.Test.Unit/Mapping/Converters/EncodedStringToUInt32ConverterTests.cs | New test suite including non-generic IConverter path coverage. |
| src/CoreEx/Mapping/Converters/TypeToJsonStringConverter.cs | Removes redundant non-generic object? conversion overrides in favor of interface DIMs. |
| src/CoreEx/Mapping/Converters/StringBase64Converter.cs | Removes redundant non-generic object? conversion overrides in favor of interface DIMs. |
| src/CoreEx/Mapping/Converters/JsonElementStringConverter.cs | Removes previously-buggy non-generic object? conversion overrides; relies on corrected interface DIMs. |
| src/CoreEx/Mapping/Converters/IConverterT.cs | Adds the missing DIM implementations for the base IConverter object-based conversion members. |
| src/CoreEx/Mapping/Converters/EncodedStringToUInt32Converter.cs | Removes redundant non-generic object? conversion overrides in favor of interface DIMs. |
chullybun
deleted the
chullybun-fix-json-element-string-converter-icover
branch
August 3, 2026 18:15
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
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.
Summary
Fixes #175.
JsonElementStringConverter's non-genericIConverter.ConvertToDestination(object?)/ConvertToSource(object?)overrides cast to the wrong side's type, causingInvalidCastExceptionor infinite recursion (StackOverflowException) for any caller going through the non-genericIConvertercontract.Root cause and audit
Audited all 4 implementers of
IConverter<TSource,TDestination>. OnlyJsonElementStringConverterwas actually broken;StringBase64Converter,EncodedStringToUInt32Converter, andTypeToJsonStringConverter<T>already had correct casts, just the same duplicated hand-rolled pattern.Fix
Rather than just patching the one bad cast, this removes the whole duplicated pattern (as the issue suggests) so the bug class can't recur:
IConverterT.cs— added the two missing default interface implementations for the baseIConverter.ConvertToDestination(object?)/ConvertToSource(object?)members onIConverter<TSource,TDestination>. These were previously not covered by any DIM anywhere in the hierarchy (the existing DIMs only satisfy thenew-hidden slots onISourceConverter<TSource>/IDestinationConverter<TDestination>— analogous to howIEnumerable<T>hides but doesn't implementIEnumerable.GetEnumerator()). Without this addition, deleting the structs' hand-rolled overrides would not compile.JsonElementStringConverter,StringBase64Converter,EncodedStringToUInt32Converter,TypeToJsonStringConverter<T>), now relying solely on the interface's default implementations.IConverterpath for all four converters (newJsonElementStringConverterTests, newEncodedStringToUInt32ConverterTests, extendedStringToBase64ConverterTests), mirroring the coverage already present inTypeToJsonStringConverterTests.Testing
dotnet build CoreEx.sln— succeeds, 0 errors.dotnet test tests\CoreEx.Test.Unit— 735/735 passing on net8.0, net9.0, and net10.0.No behavioral change for any existing strongly-typed call site; verified no call site in
src/depends on the non-generic overload being directly callable on a concretely-typed struct variable.