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

Migrations: CHECK enum columns #15411

Closed
AndriySvyryd opened this issue Apr 18, 2019 · 7 comments
Closed

Migrations: CHECK enum columns #15411

AndriySvyryd opened this issue Apr 18, 2019 · 7 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

No description provided.

@ajcvickers ajcvickers added this to the Backlog milestone Apr 19, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@SARAVANA1501
Copy link
Contributor

hi @AndriySvyryd, Could you tell me which file i can add this CHECK?

@AndriySvyryd
Copy link
Member Author

This is about adding a CHECK constraint to enum columns e.g.:

mycol VARCHAR(10) NOT NULL CHECK (mycol IN('Useful', 'Useless', 'Unknown'))

The easiest way of doing this is by adding a SqlServer convention (IModelFinalizedConvention) that calls AddCheckConstraint for all enum properties.

@SARAVANA1501
Copy link
Contributor

SARAVANA1501 commented Sep 8, 2019

Hi @AndriySvyryd

Find below my solution in the form of test case. Will it serve the purpose? Please add your comments.

 [ConditionalFact]
public void Can_create_check_constraint_with_enum()
{
       var modelBuilder = CreateConventionModelBuilder();
       var entityType = modelBuilder.Entity<Customer>().Metadata;`

       modelBuilder
                .Entity<Customer>()
                .Property(t => t.EnumValue)
                .WithEnumConstraint();

       var checkConstraint = entityType.FindCheckConstraint("CK_Customer_EnumValue");

       Assert.NotNull(checkConstraint);
       Assert.Equal(entityType, checkConstraint.EntityType);
       Assert.Equal("CK_Customer_EnumValue", checkConstraint.Name);
       Assert.Equal("CHECK (EnumValue IN('Sun', 'Mon', 'Tue'))", checkConstraint.Sql);
}

@AndriySvyryd
Copy link
Member Author

@SARAVANA1501 Not quite, this should happen without needing to call WithEnumConstraint()

@SARAVANA1501
Copy link
Contributor

@AndriySvyryd ,

SqlServerEnumConstraintConvention

Please review the convention, and guide me to register it with other conventions,

Thanks.

@AndriySvyryd
Copy link
Member Author

You are heading in the right direction.

  • Call entityType.GetDeclaredProperties() instead of entityType.GetProperties() to avoid going over the same properties.
  • Don't call property.SetIsNullable(false)
  • The convention can be added in SqlServerConventionSetBuilder:
    ConventionSet.AddBefore(
        conventionSet.ModelFinalizedConventions,
        new SqlServerEnumConvention(),
        typeof(ValidatingConvention));
  • Add tests to EFCore.SqlServer.Tests

I'll give further feedback on the PR

@AndriySvyryd
Copy link
Member Author

Fixed in e17584c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. 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.

4 participants