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

Update handling of non-nullable store-generated properties #15070

Closed
JakePorter05 opened this issue Mar 18, 2019 · 4 comments · Fixed by #30843
Closed

Update handling of non-nullable store-generated properties #15070

JakePorter05 opened this issue Mar 18, 2019 · 4 comments · Fixed by #30843
Assignees
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@JakePorter05
Copy link

If you make the default value of an enum that is mapped to the database anything other then 0 it always defaults every value saved to the default value.

Example with code: If I do default sleeping and try to set it to active and save. It will change it back to sleeping in the savechanges. But if you leave the default at 0 aka active then you can change and save as normal.

EfcoreEnumTester.zip

public partial class Person
{
   [Key, Column(TypeName = "uniqueidentifier")]
   public Guid Id { get; set; }
   [Column(TypeName = "nvarchar(20)")]
   public string FirstName { get; set; }
   [Column(TypeName = "nvarchar(20)")]
   public string LastName { get; set; }
   [Column(TypeName = "int")]
   public State State { get; set; }
 }

public enum State
{
    Active = 0,
    Sleeping = 1,
}

modelBuilder.Entity<Person>(entity =>
{
    entity.HasIndex(e => e.Id);
    entity.Property(e => e.State)
              .HasConversion<int>()
              .HasDefaultValue(State.Sleeping);
 });
@ajcvickers
Copy link
Member

Note for triage: related to #13613

@ajcvickers
Copy link
Member

Note for triage: should our guidance here be to do this:

public partial class Person
{
   public State State { get; set; } = State.Sleeping;
}

This is basically what we decided to scaffold in #13613--although we don't reverse engineer to enums by default its still the same pattern. This works because a value will always be inserted, but if no value was explicitly set, then Sleeping will be inserted.

The property above should not have a call to HasDefaultValue, or if this is needed for Migrations, then the following would be needed:

modelBuilder.Entity<Person>(entity =>
{
    entity.Property(e => e.State)
        .ValueGeneratedNever()
        .HasDefaultValue(State.Sleeping);
});

Another option is to break to HasDefaultValue (and HasDefaultValueSql) such that it doesn't also set store-generated. But that would be a pretty impactful break.

Let's discuss. (Again. 🚎)

@ajcvickers
Copy link
Member

Notes for the design meeting

Current behavior

Calling HasDefaultValue or HasDefaultValueSql does the following:

  • Marks the property as ValueGenerated.OnAdd
  • Sets the default value for use by migrations
  • Setting both of these makes the property store-generated (as opposed to client-generated)

Making something a store-generated property means:

  • If the property value is the CLR default, then don't include the value in the insert, and instead propagate back the store-generated value.
  • If the property value is anything else, then include it in the insert.

This mechanism works well except when the CLR default is a valid value that needs to be inserted. For example, if the default for a bool column should be true, then setting the CLR default (false) will result in using the default of true, and so there is no way to end up with a value of false in the database.

In addition, the call to HasDefaultValue or HasDefaultValueSql might only be intended to set the value for Migrations.

Options

Patterns that make this better, using as an example a bool with default true.

A: Use client-side defaults

We would scaffold:

public class Blog
{
    public bool IsValid { get; set; } = true;
}

and

    .HasDefaultValue(true)
    .ValueGeneratedNever();

(If we introduce a scaffolding mode which does not include all database artifacts, then we can skip this.)

When writing code manually, we would recommend just:

public class Blog
{
    public bool IsValid { get; set; } = true;
}

and no store-generated config unless you need Migrations to create the default in the database.

B: Use regular properties backed by nullable field

This approach would preserve all store-generated behavior at the expense of more code. Also, we need to make changes to EF to make this work, but they should not be complex.

Code is the same for both scaffolding and writing manually:

public class Blog
{
    private bool? _isValid;

    public bool IsValid
    {
        get => _isValid ?? false;
        set => _isValid = value;
    }
}

and

    .HasDefaultValue(true);

(There should be no need to set the property access mode since we read/write fields by default in 3.0.)

C: Use nullable properties

This preserves store generated behavior, but forces the type in the model to be nullable.

public class Blog
{
    public bool? IsValid { get; set; };
}

and

    .HasDefaultValue(true);

Proposal

For reverse engineering:

  • If the default value can be parsed into a CLR object, then use pattern A
  • Otherwise use pattern B (with HasDefaultValueSql)

To document/recommend for "Code First":

  • Show how to do pattern A for when client-side defaults are the best option
  • Show how to do pattern B for when store defaults must be used
  • Mention pattern C to simplify code where the model property can be nullable

@ajcvickers ajcvickers changed the title Enum Default Value Doesn't work. Enhance warning for non-nullable store-generated bool properties to also cover enums Mar 27, 2019
@ajcvickers
Copy link
Member

Notes from design meeting:

  • Make B (nullable field) work (see Enable nullable backing field pattern for store-generated defaults #15182)
  • Consider expanding warning for bools to also be used for enums (this issue)
  • For hand-written code document: (see Update guidance on using default values EntityFramework.Docs#1398)
    • Use client side default values. This ensures the defaults are consistent in the domain model.
    • Show how store-generated values work in the "normal" case--e.g. with integers
    • Show how to use option B for when both the normal way and client-side defaults are not appropriate, also mentioning C
    • Show how to get the default value into the migration if it needs to be there
  • For scaffolding: (see Update pattern for scaffolding column default constraints #13613)
    • We currently only do C for bools with a default of true. We could consider doing B here, but this could be punted to post 3.0.
    • For other cases, just keep doing what we are doing.
  • Use of a different sentinel value could be useful here if it is set to the same as the default value. (See Allow default value check in value generation to be customized #701)
    • This would mean that if the value is set to default, then it would not be inserted, since this value is the same as the sentinel. But it would get the default value from the database as expected.
    • However, this would be a breaking change since it would insert the CLR default rather than using the default from the database
    • We could even consider automatically making the default value the same as the sentinel value if it has been set.

@ajcvickers ajcvickers added this to the Backlog milestone Mar 29, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@ajcvickers ajcvickers changed the title Enhance warning for non-nullable store-generated bool properties to also cover enums Update handling of non-nullable store-generated properties Mar 13, 2023
@ajcvickers ajcvickers self-assigned this Mar 18, 2023
ajcvickers added a commit that referenced this issue Apr 25, 2023
…as not been set

Fixes #701

Part of #13224, #15070, #13613

This PR contains the underlying model and change tracking changes needed to support sentinel values. Future PRs will add model building API surface and setting the sentinel automatically based on the database default.

There is a breaking change here: explicitly setting the value of a property with a temporary value no longer automatically makes the value non temporary.
ajcvickers added a commit that referenced this issue Apr 28, 2023
…as not been set

Fixes #701

Part of #13224, #15070, #13613

This PR contains the underlying model and change tracking changes needed to support sentinel values. Future PRs will add model building API surface and setting the sentinel automatically based on the database default.

There is a breaking change here: explicitly setting the value of a property with a temporary value no longer automatically makes the value non temporary.
ajcvickers added a commit that referenced this issue May 8, 2023
Fixes #15070

- Warn for enums like we do for bools
- Don't warn if a sentinel value has been configured
- Update warning message to mention sentinel value
- Set the sentinel by convention for non-nullable bools with a default value
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label May 9, 2023
@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0 May 9, 2023
ajcvickers added a commit that referenced this issue May 9, 2023
Fixes #15070

- Warn for enums like we do for bools
- Don't warn if a sentinel value has been configured
- Update warning message to mention sentinel value
- Set the sentinel by convention for non-nullable bools with a default value
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview5, 8.0.0 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants