Skip to content

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

Description

@chullybun

Summary

JsonElementStringConverter (src/CoreEx/Mapping/Converters/JsonElementStringConverter.cs) implements IConverter<JsonElement?, string?>. Its two non-generic IConverter overloads cast to the wrong side's type:

public readonly object? ConvertToDestination(object? source) => ConvertToDestination((string?)source);
public readonly object? ConvertToSource(object? destination) => ConvertToSource((JsonElement?)destination);
  • ConvertToDestination(object? source) should cast source to TSource (JsonElement?), not string?.
  • ConvertToSource(object? destination) should cast destination to TDestination (string?), not JsonElement?.

Impact

Because the cast's static type doesn't match either TSource/TDestination, C# overload resolution can't bind to the strongly-typed sibling overload (ConvertToDestination(JsonElement? source) / ConvertToSource(string? destination)) — there's no implicit conversion between string and JsonElement. It ends up either:

  • Throwing InvalidCastException at the cast itself (typical real-world case), or
  • If the runtime value happens to satisfy the wrong cast, recursively calling itself (object? self-overload) — infinite recursion / StackOverflowException.

This was found while auditing all IConverter<TSource, TDestination> implementations in src/CoreEx/Mapping/Converters/ during a separate change (see TypeToJsonStringConverter<T>, which had — and has now had fixed — the exact same defect class). Confirmed via git diff main that JsonElementStringConverter is unmodified/pre-existing, so it's tracked here as its own issue rather than folded into that unrelated PR.

Suggested fix

Simplest, most consistent fix across all four converters that implement IConverter<TSource, TDestination> (StringBase64Converter, EncodedStringToUInt32Converter, TypeToJsonStringConverter<T>, JsonElementStringConverter): delete the redundant hand-rolled ConvertToDestination(object?) / ConvertToSource(object?) overrides entirely. IConverter<TSource, TDestination> (IConverterT.cs) already supplies correct default interface implementations for these exact methods via ISourceConverter<TSource> / IDestinationConverter<TDestination>:

object? ISourceConverter<TSource>.ConvertToDestination(TSource source) => ConvertToDestination((TSource)source!);
TSource ISourceConverter<TSource>.ConvertToSource(object? destination) => ConvertToSource((TDestination)destination!);
TDestination IDestinationConverter<TDestination>.ConvertToDestination(object? source) => ConvertToDestination((TSource)source!);
object? IDestinationConverter<TDestination>.ConvertToSource(TDestination destination) => ConvertToSource((TDestination)destination!);

Removing the hand-rolled copies and relying on these defaults removes the duplication that caused the copy/paste bug in the first place, and fixes JsonElementStringConverter (and prevents recurrence in future converters) in one pass.

Note

This bug is currently dead code on the EF Core value-conversion pipeline — ValueConverterBridge<TModel, TProvider> calls the generic, strongly-typed ConvertToDestination(TModel) / ConvertToSource(TProvider) overloads directly, never the non-generic object?-based ones. So it won't manifest via EF Core column conversion. It's a real, live bug for any other consumer that goes through the non-generic IConverter contract (e.g. reflection-based/dynamic mapping paths).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions