Skip to content

Conversation

roji
Copy link
Member

@roji roji commented Sep 16, 2020

Closes #5

This hardcodes support for specific providers. Once dotnet/efcore#15409 is done, we can modify this to generate expression trees instead, and have the provide do the proper translation.

Notes:

  • Both SQL Server and Sqlite need special setup in order to have regex, the docs will point that out.
  • The validation in the .NET validation attributes is sometimes really odd - I don't consider them very good. I currently implemented to reproduce the same behavior in regex, but how important is exact compatibility with the .NET attribute, maybe we should just do better? Note that users can configure the regexes they want in any case.
  • [Compare]: is there any real use-case for this in a database? Sounds like it should be a computed column instead, no?
  • [DataType]: some of the values duplicate dedicated attributes (phone number, credit card, but not postal code (?)), others seem to be more about actual types (DateTime, Duration) which seem more appropriate for database types. So the value of this seems somewhat limited ()
  • [CreditCard]: we do not do checksum
  • Do a special [LikeValidation] attribute? Becomes pretty close to just having a [CheckConstraint] in EFCore.Abstractions (which we should probably do)

/cc @bricelam @ajcvickers @AndriySvyryd @smitpatel @maumar

@roji roji force-pushed the ValidationConstraints branch from 1ca4de6 to 39dc0cf Compare September 16, 2020 11:06
continue;
}

var columnName = property.GetColumnName();

Choose a reason for hiding this comment

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

Should get the name for the specific table

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, still learning the new world of the relational model. Will also modify the other conventions to do the same.

Should the parameterless GetColumnName continue to exist, as it's rarely what you want to call? Maybe rename to GetDefaultColumnName (the breaking change here would probably have been a good thing)?

Choose a reason for hiding this comment

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

Neither should be used outside of model building and I think there's not enough benefit to do this just for conventions.

Copy link
Member Author

@roji roji Sep 17, 2020

Choose a reason for hiding this comment

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

FWIW in EFCore.PG there are calls to parameterless GetColumnName in NpgsqlMigrationsSqlGenerator and in NpgsqlAnnotationProvider, which I will now go and fix.

I'm pretty sure that any provider writer out there (like me) will call this wrong method 100%, it seems dangerous.

Choose a reason for hiding this comment

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

It's too late for 5.0 anyway. I would recommend provider writers to add TPT tests for provider-specific functionality.

@roji roji merged commit 07a9535 into efcore:main Sep 18, 2020
@roji roji deleted the ValidationConstraints branch December 18, 2023 22:08
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.

Add check constraints for validation attributes
2 participants