Skip to content

Fix JsonElementStringConverter non-generic IConverter swapped casts (#175) - #177

Merged
chullybun merged 1 commit into
mainfrom
chullybun-fix-json-element-string-converter-icover
Aug 3, 2026
Merged

Fix JsonElementStringConverter non-generic IConverter swapped casts (#175)#177
chullybun merged 1 commit into
mainfrom
chullybun-fix-json-element-string-converter-icover

Conversation

@chullybun

Copy link
Copy Markdown
Collaborator

Summary

Fixes #175. JsonElementStringConverter's non-generic IConverter.ConvertToDestination(object?)/ConvertToSource(object?) overrides cast to the wrong side's type, causing InvalidCastException or infinite recursion (StackOverflowException) for any caller going through the non-generic IConverter contract.

Root cause and audit

Audited all 4 implementers of IConverter<TSource,TDestination>. Only JsonElementStringConverter was actually broken; StringBase64Converter, EncodedStringToUInt32Converter, and TypeToJsonStringConverter<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:

  1. IConverterT.cs — added the two missing default interface implementations for the base IConverter.ConvertToDestination(object?) / ConvertToSource(object?) members on IConverter<TSource,TDestination>. These were previously not covered by any DIM anywhere in the hierarchy (the existing DIMs only satisfy the new-hidden slots on ISourceConverter<TSource>/IDestinationConverter<TDestination> — analogous to how IEnumerable<T> hides but doesn't implement IEnumerable.GetEnumerator()). Without this addition, deleting the structs' hand-rolled overrides would not compile.
  2. Deleted the redundant hand-rolled non-generic overrides from all four converters (JsonElementStringConverter, StringBase64Converter, EncodedStringToUInt32Converter, TypeToJsonStringConverter<T>), now relying solely on the interface's default implementations.
  3. Added regression tests exercising the non-generic IConverter path for all four converters (new JsonElementStringConverterTests, new EncodedStringToUInt32ConverterTests, extended StringToBase64ConverterTests), mirroring the coverage already present in TypeToJsonStringConverterTests.

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.

…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>
Copilot AI review requested due to automatic review settings August 3, 2026 18:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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> for IConverter.ConvertToDestination(object?) and IConverter.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 IConverter interface 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
chullybun merged commit a8ad59f into main Aug 3, 2026
4 checks passed
@chullybun
chullybun deleted the chullybun-fix-json-element-string-converter-icover branch August 3, 2026 18:15
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.

JsonElementStringConverter: non-generic IConverter overloads use swapped/wrong casts

2 participants