Skip to content

Honor HasConversion<T>() for non-key reference properties in source-generated mappers - #146

Merged
mrdevrobot merged 3 commits into
mainfrom
copilot/fix-valueconverter-issue
Sep 12, 2026
Merged

mrdevrobot merged 3 commits into
mainfrom
copilot/fix-valueconverter-issue

Conversation

Copilot AI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

HasConversion<T>() was not reliably applied to non-ID reference-type properties (e.g., SmartEnum-style types), so values could be serialized/deserialized through nested-object logic instead of the configured converter. This caused incorrect round-trips for properties like Gender.

  • Generator fix: converter precedence

    • Updated source-generator mapper emission so non-key properties with ConverterTypeName are handled before nested-object/primitive branches.
    • Applied this precedence in both write and read paths:
      • GenerateWriteProperty(...)
      • GenerateReadPropertyToLocal(...)
    • Result: explicit fluent conversion now consistently controls persistence for supported provider types.
  • Regression coverage for SmartEnum-like scenario

    • Added a focused non-ID reference-type converter case:
      • DeviceWithSmartStatus
      • SmartStatus + SmartStatusConverter : ValueConverter<SmartStatus, string>
    • Wired model config through fluent API:
      • .Property(x => x.Status).HasConversion<SmartStatusConverter>()
    • Added round-trip assertion ensuring canonical value restoration (SmartStatus.Active) rather than nested-object materialization behavior.
modelBuilder.Entity<DeviceWithSmartStatus>()
    .ToCollection("devices_with_smart_status")
    .Property(x => x.Status)
    .HasConversion<SmartStatusConverter>();

Copilot AI linked an issue Sep 11, 2026 that may be closed by this pull request
Co-authored-by: mrdevrobot <12503462+mrdevrobot@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix ValueConverter not working in BLite SDK Honor HasConversion<T>() for non-key reference properties in source-generated mappers Sep 11, 2026
Copilot AI requested a review from mrdevrobot September 11, 2026 08:45
@mrdevrobot

Copy link
Copy Markdown
Contributor

@copilot I'd rather update the unit test with a SmartEnum more similar to the one presented by the issue opener.

Co-authored-by: mrdevrobot <12503462+mrdevrobot@users.noreply.github.com>

Copilot AI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot I'd rather update the unit test with a SmartEnum more similar to the one presented by the issue opener.

Updated in commit b160aa4. The regression now uses Ardalis.SmartEnum with a Gender : SmartEnum<Gender> model (Male=3, Female=5) and GenderValueConverter, matching the issue opener’s pattern more closely.

@mrdevrobot
mrdevrobot marked this pull request as ready for review September 12, 2026 22:37
Copilot AI lite review requested due to automatic review settings September 12, 2026 22:37
Copilot AI deployed to Production September 12, 2026 22:37 Active

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.

🟡 Changes recommended

Unresolved converter ordering, null-handling, and numeric-cast issues remain in generated mapper code.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR updates source-generated mappers so explicit HasConversion<T>() takes precedence for non-key reference properties, with SmartEnum-style regression coverage.

Changes:

  • Reorders converter handling in write/read generation.
  • Adds SmartEnum fixtures, configuration, and round-trip testing.
  • Adds the supporting test dependency.
File summaries
File Description
tests/BLite.Tests/SourceGeneratorFeaturesTests.cs Adds SmartEnum round-trip coverage.
tests/BLite.Shared/TestDbContext.cs Registers the test collection and converter.
tests/BLite.Shared/MockEntities.cs Adds SmartEnum test models and converter.
tests/BLite.Shared/BLite.Shared.csproj Adds the SmartEnum package dependency.
src/BLite.SourceGenerators/CodeGenerator.cs Updates converter precedence and generated mapper emission.
Review details

Suppressed comments (1)

src/BLite.SourceGenerators/CodeGenerator.cs:860

  • The read path has the same nullability mismatch for nonnullable-declared reference properties. If the stored value is BSON null, it calls ReadString()/the provider reader without a null check; ReadString expects a string length and throws. Mirror the write-side reference-type guard here.
                     if (prop.IsNullable)
  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

}
else
{
sb.AppendLine($" {localVar} = _converter_{prop.Name}.ConvertFromProvider(reader.{converterReadMethod}{converterReadArgs});");
var converterWriteMethod = GetPrimitiveWriteMethod(providerProp, allowKey: false);
if (converterWriteMethod != null)
{
if (prop.IsNullable)
}
sb.AppendLine($" }}");
}
else if (prop.ConverterTypeName != null)
@mrdevrobot
mrdevrobot merged commit 345ac63 into main Sep 12, 2026
7 checks passed
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.

the ValueConverter isn't work

3 participants