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

Properties should be "required" by default, if not declared as option or nullable #79

Closed
zelenij opened this issue Apr 2, 2021 · 5 comments · Fixed by #115
Closed
Assignees
Labels
Milestone

Comments

@zelenij
Copy link

zelenij commented Apr 2, 2021

I was under impression that by default fields like string, byte[] etc would be automatically flagged as IsRequired. However, a simple type like this:


    [<CLIMutable>]
    type CoreEventAttachment = 
    {
        Id: int64
        CoreEventInfoId: int64
        CoreEventInfo: CoreEventInfo
        CreatedAt: DateTime
        LastUpdatedAt: DateTime
        [<Required>]
        MimeType: string
        [<Required>]
        Attachment: byte[]
    }

Has Attachment and MimeType fields migration code without IsRequired - when no "Required" attribute is stamped, of course. Interestingly, all DateTime fields are flagged as IsRequired automatically.

I looked at the code, I can see that the decision to flag is made here, but I'm not entirely sure what the logic is exactly.

@simon-reynolds simon-reynolds changed the title Question: shouldn't most properties be "required" by default, if not declared as option? Properties should be "required" by default, if not declared as option or nullable Apr 2, 2021
@simon-reynolds simon-reynolds self-assigned this Apr 2, 2021
@simon-reynolds
Copy link
Collaborator

Original implementation for handling option types is broken
Attempting to work around it with usage of ValueConverterSelector but ValueConverters has issues at the moment around handling null values
Will keep an eye on dotnet/efcore#13850

@zelenij
Copy link
Author

zelenij commented Apr 5, 2021

Fun, looks like they have gone through a few iterations with this one already! :)

@zelenij
Copy link
Author

zelenij commented Apr 17, 2021

@simon-reynolds any thoughts on the issue? Supporting option feels like rather fundamental. I see there have been no updates on the issue in the efcore project you linked - is it an absolute blocker?

@simon-reynolds
Copy link
Collaborator

Hi @zelenij
At the moment ValueConverters are not called for null values in the database so it is a blocker
I'm trying to see what other ways there may be to get around this

@simon-reynolds
Copy link
Collaborator

@zelenij, dotnet/efcore#13850 has been closed but the functionality that allows conversion of null values will be available from EFCore 6 onwards

I'm afraid full option support will only really be feasible from then.
I'll use this ticket to track development against the EF Core 6 preview builds for handling option values

@simon-reynolds simon-reynolds added this to the efcore6 milestone Aug 17, 2021
@simon-reynolds simon-reynolds mentioned this issue Nov 4, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants