Skip to content

Fixed #11964 -- Added support for check constraints in model Meta. #7615

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 10, 2018
Merged

Fixed #11964 -- Added support for check constraints in model Meta. #7615

merged 1 commit into from
Jul 10, 2018

Conversation

LilyFirefly
Copy link
Contributor

@LilyFirefly LilyFirefly commented Nov 24, 2016 •
edited by timgraham
Loading

LilyFirefly
LilyFirefly commented Nov 24, 2016
View reviewed changes
django/db/models/constraints.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work because I don't have access to a suitable compiler. I will also need to add support for as_sql to Q objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From #8056, it looks like I can do:

compiler = connection.ops.compiler('SQLCompiler')(None, connection, 'default')

@LilyFirefly LilyFirefly force-pushed the ticket_11964 branch 2 times, most recently from f7f256c to 89b2092 Compare February 19, 2017 23:46
LilyFirefly
LilyFirefly commented Feb 19, 2017
View reviewed changes
django/db/models/constraints.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets me further. Unfortunately, I appear to need a more complete query instead of None, so Col.as_sql can be evaluated.

LilyFirefly
LilyFirefly commented Feb 19, 2017
View reviewed changes
tests/migrations/test_operations.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long term, I want to be able to use Q(pink__gt=2) here, but for now using this avoids the need to implement Q.as_sql.

LilyFirefly
LilyFirefly commented Feb 19, 2017
View reviewed changes
tests/migrations/test_state.py Outdated
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 haven't really thought whether this is the best way to fix these tests, but it is sufficient for now.

@LilyFirefly LilyFirefly force-pushed the ticket_11964 branch 4 times, most recently from fc2f707 to 835f04e Compare February 26, 2017 16:40
@LilyFirefly
Copy link
Contributor Author

LilyFirefly commented Feb 26, 2017 •
edited
Loading

I'm not sure where is best to put MySQL error handling. It supports the syntax, but doesn't actually do anything with it, so I think we should fail loud somewhere. Perhaps in a check?

@LilyFirefly
Copy link
Contributor Author

I'm also not sure how best to handle SQLite. It appears to support the feature, but I get a syntax error from SQLite with the syntax that works on postgres. I think this is because that uses ALTER TABLE, which SQLite only supports a limited form of, but I'm not clear what to do instead.

Django does set supports_column_check_constraints to False for SQLite's db features, so perhaps working around this limitation would be difficult.

@LilyFirefly
Copy link
Contributor Author

LilyFirefly commented Feb 26, 2017 •
edited
Loading

@jarshwah @MarkusH I'd appreciate your feedback on this. There's definitely some work left to do - docs, MySQL, SQLite, naming consistency, any edge-cases I've missed - but I think the core is mostly there.

I'd also like to add a test to check an IntegrityError is actually raised if a Check constraint is violated, but I'm not sure how to set up the model to test so the Check is applied in the database.

@LilyFirefly LilyFirefly force-pushed the ticket_11964 branch 4 times, most recently from 50f4ae0 to 5e97e81 Compare February 27, 2017 20:56
auvipy
auvipy reviewed Mar 18, 2017
View reviewed changes
docs/ref/models/options.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

should be 2.0

@LilyFirefly LilyFirefly force-pushed the ticket_11964 branch 3 times, most recently from 00ee601 to 505beef Compare April 8, 2017 09:43
@LilyFirefly LilyFirefly mentioned this pull request Apr 10, 2017
Merged
@LilyFirefly LilyFirefly force-pushed the ticket_11964 branch 2 times, most recently from 44e6a30 to 76217f8 Compare April 17, 2017 15:14
@LilyFirefly
Copy link
Contributor Author

This now just needs to do something sensible for MySQL and the rest is cleanup and edge-cases.

LilyFirefly
LilyFirefly commented Apr 17, 2017
View reviewed changes
django/db/models/sql/query.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be possible to simplify this further. I'm also not 100% certain I've thought of every edge-case, so I may have accidentally deleted something important.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels odd why so much code needs to be duplicated. Have you considered letting Query subclass this?

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 hadn't considered that. My main focus has been trying to get something that works well for this use case. Refactoring to allow code re-use is a secondary concern.

Copy link
Member

Choose a reason for hiding this comment

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

This is a lot of stuff to duplicate, especially as you note below that you don't understand everything. I'm concerned that future expression refactors/features will be missed in constraints and things will asplode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - that's a good point. I'll take another pass and see if I can minimise the amount of duplicated code.

Copy link
Member

Choose a reason for hiding this comment

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