Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-nullable struct entity property to be DB optional, if conversion to/from nullable type defined. #27227

Closed
joschmo80 opened this issue Jan 20, 2022 · 10 comments

Comments

@joschmo80
Copy link

joschmo80 commented Jan 20, 2022

Consider the following example model containing a non-nullable struct:

public void FooEntity {
   public PersonNameStruct Name { get; set; }
}

where PersonNameStruct is an example struct that allows conversion to different formats (Smith, John for display, SMITH^JOHN for storage in medical DICOM files, etc.).

and a conversion defined for PersonNameStruct to/from string (in either OnModelCreating or ConfigureConventions).

It would be nice if the Name property did not have to have a nullable operator (?). This way all code that references that property can avoid having to check against null (Name.HasValue, Name?.ToString(), (Name ?? new PersonNameStruct()).ForDisplay, etc.) Aside from cleaner code, this affects any reflection code that operates on entities.

Yes, this can also be solved by making the Name prop NonMapped, and adding a helper prop of type string that IS mapped to the database.

[NotMapped]
public PersonNameStruct Name { get => NameHelper; set => NameHelper = value.ForDb() }
public string NameHelper { get; set; }

However, this will cause helper props to be littered throughout the Entity classes, which is less than desired.

  • Allow IsRequired(false) on non-nullable properties that have a known conversion to/from nullable properties.
  • Ideally this would be allowed in both fluent API and data annotations.
  • Similarly, this would ideally be available in ConfigureConventions too
    • Ex: builder.Properties<PersonNameStruct>().HaveConversion<PersonNameStructConverter>().IsRequired(false);
@joschmo80
Copy link
Author

joschmo80 commented Jan 20, 2022

For reference, here is the error in EFCore 6.0 that appears when I attempt to specify IsRequired(false) for the aforementioned struct w/ defined conversion to/from string:
The property 'FooEntity .Name' cannot be marked as nullable/optional because the type of the property is 'PersonNameStruct ' which is not a nullable type. Any property can be marked as non-nullable/required, but only properties of nullable types can be marked as nullable/optional.

@roji
Copy link
Member

roji commented Jan 20, 2022

@joschmo80 I'm confused - if the Name property is a non-nullable struct, why do you currently need to have a nullable operator on it? Is the intent for the database column to be nullable or not?

@joschmo80
Copy link
Author

joschmo80 commented Jan 20, 2022

Thanks for answering this so quickly. The intent is for the database column to be nullable, but the property itself to be non-nullable. I can provide a more specific use case if you desire.

@roji
Copy link
Member

roji commented Jan 21, 2022

We have #24685 which is about allowing value converters to change nullability (e.g. non-nullable property with nullable column) - that seems to be what you're looking for (/cc @ajcvickers).

Simply allowing IsRequired(false) on non-nullable properties would cause a runtime exception to occur whenever reading a null value, so that's problematic.

@joschmo80
Copy link
Author

#24685 might be what I am looking for.

To be clear, #24685 will allow a non-nullable property (say public FooStruct { get; set; }) to be column nullable, if that property has a conversion defined with, say, a string?

@roji
Copy link
Member

roji commented Jan 21, 2022

@joschmo80 yes, that's the idea.

@joschmo80
Copy link
Author

Perfect, thank you very much! And, thank you to the entire EFCore team - your product is amazing!

@roji
Copy link
Member

roji commented Jan 21, 2022

Duplicate of #24685

@roji roji marked this as a duplicate of #24685 Jan 21, 2022
@roji
Copy link
Member

roji commented Jan 21, 2022

Thanks for the kind words @joschmo80!

@nettashamir-allocate
Copy link

nettashamir-allocate commented Feb 8, 2022

@ajcvickers, @roji I have the exact same issue and have tried the solution in #24685.

I have an entity, MyEntity with a property defined as follows:

NullableTime NonNullableButNullInDB { get; set; }

NullableTime is a special type class which expects to be always populated in the model but may have a null DateTime associated with it in the DB. The existence and usage of this class is legacy and I can't get rid of it at the moment so have to be able to support it.

In ConfigureMappings, I am setting

protected override void ConfigureMappings(EntityTypeBuilder<MyEntity> entity) {
   entity.Property(e => e.NonNullableButNullInDB).Metadata.IsNullable = true;

but I get the following exception:

The property 'MyEntity.NonNullableButNullInDB' cannot be marked as nullable/optional because the type of the property is 'NullableTime' which is not a nullable type. Any property can be marked as non-nullable/required, but only properties of nullable types can be marked as nullable/optional.

at

   at Microsoft.EntityFrameworkCore.Metadata.Internal.Property.SetIsNullable(Nullable`1 nullable, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.Property.set_IsNullable(Boolean value)
   at MyEntityConfiguration.ConfigureMappings(EntityTypeBuilder`1 entity)

Should I expect this to work now or has the support for this been reverted?

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants