Skip to content

fix: Allow nullable Exclude properties#198

Open
Dreamescaper wants to merge 1 commit intoartiomchi:mainfrom
Dreamescaper:allow-nullable-exclude
Open

fix: Allow nullable Exclude properties#198
Dreamescaper wants to merge 1 commit intoartiomchi:mainfrom
Dreamescaper:allow-nullable-exclude

Conversation

@Dreamescaper
Copy link

@Dreamescaper Dreamescaper commented Mar 5, 2026

Currently the following code produces nullable warning:

public class MyModel
{
      public int Id {get;set;}
      public required string Name {get;set;}
      public string? Description {get;set;}
}

await dbContext.MyModels
     .Upsert(new() { Id = 42, Name = "Answer to Life"  })
     .Exclude(m => m.Description) // Nullable warning here
     .RunAsync();

It works perfectly fine if you use .Exclude(m => m.Description!).

All I did was to change Expression<Func<TEntity, object>> to Expression<Func<TEntity, object?>> for Exclude expressions.

{
var matchProperties = _matchExpression != null
? ProcessPropertiesExpression(_entityType, _matchExpression, true)
? ProcessPropertiesExpression(_entityType, _matchExpression!, true)
Copy link
Owner

Choose a reason for hiding this comment

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

Wait, is this line required here?

Copy link
Author

@Dreamescaper Dreamescaper Mar 5, 2026

Choose a reason for hiding this comment

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

Yeah, it complains about conversion without !:

Argument of type 'Expression<Func<TEntity, object>>' cannot be used for parameter 'propertiesExpression' of type 'Expression<Func<TEntity, object?>>' in 'IProperty[] UpsertCommandBuilder.ProcessPropertiesExpression(IEntityType entityType, Expression<Func<TEntity, object?>> propertiesExpression, bool match)' due to differences in the nullability of reference types.

I thought about making _matchExpression object? as well, but probably it shouldn't ever point to nullable property.

Copy link
Owner

Choose a reason for hiding this comment

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

It might be worth validating that through tests, as the expression might be inflated on in memory EF

Copy link
Author

Choose a reason for hiding this comment

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

Could you clarify what to validate exactly?

I thought about adding integration test with nullable Exclude property, but currently Nullable=false for integration tests project.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm.. good point - in which case it's probably safe to just merge it :D

I'll probably run a test (or check if we have one that won't have issues excluding columns that have null values). It shouldn't, but I want to try sanity check just to cover all bases

Copy link
Author

@Dreamescaper Dreamescaper Mar 5, 2026

Choose a reason for hiding this comment

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

I thought about making _matchExpression object? as well, but probably it shouldn't ever point to nullable property.

Ah, I assume you mean this sentence. It's not that it wouldn't work or fail if matchExpression leads to nullable property - ProcessPropertiesExpression doesn't care if it's nullable or not, it simply inspects expression, that's it.

It's just semantically matchExpression leads to PK or unique index, so the column shouldn't be nullable.

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.

2 participants