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

Support for CHECK constraints #14673

Merged
merged 1 commit into from
Apr 18, 2019
Merged

Conversation

Muppets
Copy link
Contributor

@Muppets Muppets commented Feb 11, 2019

Thought I would pick this up as it sounded easy enough. Turns out it was probably more of a fundamental change than I was expecting and I feel I'm slightly out of my depth.

I feel like I've missed a ton of unit tests along the way. Feel free to rip it apart or send me in a different direction if it's not implemented correctly. Designed by example on this one.

Summary of the changes

  • Implemented CHECK constraint in a similar fashion to unique constraints.
  • I moved the name property from being an annotation property to be required when specifying the constraint
  • Added HasCheckConstraint to EntityTypeBuilder (unsure on this name, open to better ones)
  • Added Add and Drop operations for check constraint. Decided to not implement a rename operation so it just calls Drop and Add for changes in name or logical expression.
  • Not added any scaffolding code for finding existing check constraints in the database, wasn't sure if this was needed/wanted, or how to implement that.
  • Not added anything out of the box for auto adding check constraints to Enum type columns. Felt like this could be added as another issue with this support in the bag.

Fixes #14425

@bricelam bricelam self-assigned this Feb 15, 2019
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Direction is good, but we should settle on the IModel changes before moving forward.

src/EFCore/Metadata/ICheckConstraint.cs Outdated Show resolved Hide resolved
/// <returns> An object that can be used to configure the check constraint. </returns>
public virtual EntityTypeBuilder HasCheckConstraint(
[NotNull] string name,
[NotNull] string logicalExpression)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be cool to take a C# expression and use the query pipeline to generate the SQL. We have similar ideas for ToView().

If we did that, these could also be implemented for the in-memory provider (it would just evaluate the expression)

Copy link
Contributor

@bricelam bricelam Feb 28, 2019

Choose a reason for hiding this comment

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

If this becomes specific to relational (and we don't take C# expressions), we should call this sql instead of expression.

Copy link
Member

Choose a reason for hiding this comment

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

I think this was also discussed at some point for expressing global filters, maybe we should track all places which could benefit from this in a new issue.

In any case, it will always be desirable to provide a raw SQL overload (for expressions which for any reason can't be expressed otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm liking the idea for the C# expression overload. Any pointers on where I could find examples of using the query pipeline to generate SQL like that already?

Copy link
Member

Choose a reason for hiding this comment

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

I think finalizing the raw SQL version is a good first goal - an expression overload isn't going to be a simple task... In addition the query pipeline is being redone at the moment, so it may not be the best timing. Finally, expression make sense in other places, not just this one, so it may be better to tackle them together later. So I'd recommend concentrating only on SQL for now - we know we want it anyway - but as @bricelam wrote make sure everything is compatible with adding expressions later.

@bricelam
Copy link
Contributor

bricelam commented Feb 28, 2019

Another crazy idea: We could create these by convention based on validation attributes:

Attribute Expression
[Compare("Y")] X == Y
[MinLength(8)] X.Length >= 8
[Range(0, 10)] X >= 0 && X <= 10
[RegularExpression("[0-9A-F]+") Regexp.IsMatch(X, "[0-9A-F]+")
[DataType(...)] (predefined regular expressions)

@phatcher
Copy link

phatcher commented Mar 2, 2019

Couple of things to consider if you want to support OOB

  1. Convention based naming e.g. "CH_{TableName}_{ColumnName}", 99% of the time there is at most one check for a column
  2. Expressions based on comparing two columns e.g Start < FInish, i.e. can you generate the code from a C# lamda.

@Muppets
Copy link
Contributor Author

Muppets commented Apr 1, 2019

@bricelam Right, I think I've completed the review changes.

One bit I'm not happy with is that CheckConstraints collection hangs off RelationalModelAnnotations which feels wrong as it relates to entity types so maybe it should live on RelationalEntityTypeBuilderAnnotations?

@AndriySvyryd
Copy link
Member

It's fine for CheckConstraints collection to hang off RelationalModelAnnotations for now. The reason it feels wrong is because it should be associated with a table, but to properly represent that we need a store model.

@Muppets Muppets changed the title [WIP] Support for CHECK constraints Support for CHECK constraints Apr 16, 2019
@AndriySvyryd
Copy link
Member

@Muppets Generally it's looking good, so it's time to rebase and squash so we can have a final review.

@Muppets
Copy link
Contributor Author

Muppets commented Apr 17, 2019

@AndriySvyryd I know this might not be the right place to ask this but after a few touch and go moments rebasing and squash some other branches, do you guys have any tips on how you generally rebase and squash changes on a branch to master?

@AndriySvyryd
Copy link
Member

@Muppets There's nothing specific to our repo that I can think of. It helps to rebase frequently, you can do it retroactively by rebasing to a specific older commit.

@Muppets Muppets requested review from dougbu and a team as code owners April 18, 2019 20:54
@natemcmaster natemcmaster removed the request for review from a team April 18, 2019 21:07
@dougbu dougbu removed their request for review April 18, 2019 21:52
Improved encapsulation for CheckConstraint annotations
@AndriySvyryd
Copy link
Member

@Muppets Thanks for your contribution!

@Muppets Muppets deleted the issue14425 branch April 18, 2019 23:03
roji added a commit that referenced this pull request Aug 11, 2019
roji added a commit that referenced this pull request Aug 11, 2019
@roji
Copy link
Member

roji commented Aug 11, 2019

Note follow-up in #17085

@Muppets
Copy link
Contributor Author

Muppets commented Aug 11, 2019

Thanks @roji. Definitely felt light on the tests for this.

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.

5 participants