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

Add support for check constraints. #270

Merged
merged 6 commits into from
Mar 28, 2018
Merged

Add support for check constraints. #270

merged 6 commits into from
Mar 28, 2018

Conversation

SackCastellon
Copy link
Contributor

This is a first implementation of the support for check constraints, based on the Issue #268

A test has been created to test this new functionality: DDLTests.testCheckConstraint(). It has been tested to work with PostgreSQL, SQLite and H2.

@SackCastellon SackCastellon changed the title #268 Add support for check constraints. Add support for check constraints. Mar 19, 2018
@Tapac
Copy link
Contributor

Tapac commented Mar 20, 2018

Check constraint can be used with multiple columns (https://www.w3schools.com/sql/sql_check.asp), do you mind to support it too?

@SackCastellon
Copy link
Contributor Author

SackCastellon commented Mar 20, 2018

Check constraint with multiple columns are already supported.
You can write each check in its corresponding column:

val positive = integer("positive").check { it greaterEq 0 }
val negative = integer("negative").check("subZero") { it less 0 }

Or just one check in the last column involving all the previous columns:

val positive = integer("positive")
val negative = integer("negative").check("multiColumn") { (it less 0) and (positive greaterEq 0) }

Both are equivalent.

Is this what you meant?

@Tapac
Copy link
Contributor

Tapac commented Mar 20, 2018

Not exactly, I mean that you may want to define complex check which doesn't belong to a single column and then such DSL (based on Column<>.check extension function) looks a bit ugly.

Maybe there should be separate check function available within Table class, in addition, to check on a column ?

@SackCastellon
Copy link
Contributor Author

Okay, I'll try to implement it.

@SackCastellon
Copy link
Contributor Author

That the best way that I found to implement it

@Tapac
Copy link
Contributor

Tapac commented Mar 22, 2018

Sorry, but I'll be able to verify PR only on weekends.

}

override fun createStatement(): List<String> {
if (currentDialect is MysqlDialect) throw UnsupportedOperationException("Check constraints are not currently supported by MySQL")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's better to log warn or error, because MySQL parses CHECK expression, but ignore it.
If you work with different databases and use same Exposed classes then you will have problem running it on MySQL.

* @param op The expression that the value of this column must satisfy
*/
fun <T> Column<T>.check(name: String = "", op: SqlExpressionBuilder.(Column<T>) -> Op<Boolean>) = apply {
table.checkConstraints.add(name to SqlExpressionBuilder.op(this))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth to check that constraint with the same name already in checkConstraints list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I just checked and SQLite is the only database, among MySQL, H2 and Postgres, that supports multiple constraints with the same name.

I'll add the necessary code to check for repeated names.

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 am also considering to .trim() the name before it is added to the list. What do you think @Tapac?

Also, empty names will be added even if there are constraints with empty names already, so an automatic name is generated by the database.

- Warning shown when trying to create a CHECK constraint in MySQL
- Constraint with names that were already added are ignored and a warning is shown
- White spaces are now trimmed from constraint names
- Improved tests
@@ -76,17 +76,17 @@ data class CheckConstraint(val tableName: String, val checkName: String, val che
}

override fun createStatement(): List<String> {
if (currentDialect is MysqlDialect) throw UnsupportedOperationException("Check constraints are not currently supported by MySQL")
if (currentDialect is MysqlDialect) exposedLogger.warn("CHECK constraints are not currently supported by MySQL")
Copy link
Contributor

Choose a reason for hiding this comment

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

One more "idea": why not to return an empty list in a case when the database doesn't support constraints? With warn logging ofc.
Here and in dropStatements(). And you can remove any checks in modifyStatements().

@Tapac Tapac merged commit 41d5b30 into JetBrains:master Mar 28, 2018
Tapac added a commit that referenced this pull request Mar 30, 2018
…ame if it wasn't provided b/c Oracle doesn't support CHECK without name
Tapac added a commit that referenced this pull request May 23, 2018
Tapac added a commit that referenced this pull request Jun 5, 2018
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.

2 participants