Refs #29641 -- Refactored database schema constraint creation code.

[LilyFoote]

Add a test to ensure we set the constraint name correctly in the
database.

Update sqlite introspection to use sqlparse. This allows us to read the
actual constraint name for table check constraints and unique constraints.

[timgraham]

I pushed a few edits, including removing the unused attributes like sql_create_fk -- that created a test failure.

Also, here's a failure if sqparse isn't installed:

======================================================================
FAIL: test_name (constraints.tests.CheckConstraintTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 615, in run
    testMethod()
  File "/home/tim/code/django/django/test/testcases.py", line 1117, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/home/tim/code/django/tests/constraints/tests.py", line 40, in test_name
    self.assertIn('price_gt_discounted_price', constraints)
  File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 1106, in assertIn
    self.fail(self._formatMessage(msg, standardMsg))
  File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 680, in fail
    raise self.failureException(msg)
AssertionError: 'price_gt_discounted_price' not found in {'__check__ONSTRAIN': {'columns': ['ONSTRAIN'], 'primary_key': False, 'unique': False, 'foreign_key': False, 'check': True, 'index': False}, '__primary__': {'columns': ['id'], 'primary_key': True, 'unique': False, 'foreign_key': False, 'check': False, 'index': False}}

Since installing sqlparse should be easy, I'm thinking it might be easier to raise an error for this case rather than to keep the old implementation that returns a made up name. What do you think?

On Oracle:

======================================================================
FAIL: test_name (constraints.tests.CheckConstraintTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/django/test/testcases.py", line 1117, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/media/evo/django/tests/constraints/tests.py", line 40, in test_name
    self.assertIn('price_gt_discounted_price', constraints)
AssertionError: 'price_gt_discounted_price' not found in {'SYS_C001001355': {'columns': ['discounted_price'], 'primary_key': 0, 'unique': 0, 'foreign_key': None, 'check': 1, 'index': 0}, 'SYS_C001001357': {'columns': ['id'], 'primary_key': 1, 'unique': 1, 'foreign_key': None, 'check': 0, 'index': 1}, 'SYS_C001001353': {'columns': ['id'], 'primary_key': 0, 'unique': 0, 'foreign_key': None, 'check': 1, 'index': 0}, 'SYS_C001001354': {'columns': ['price'], 'primary_key': 0, 'unique': 0, 'foreign_key': None, 'check': 1, 'index': 0}, 'PRICE_GT_DISCOUNTED_PRICE': {'columns': ['discounted_price', 'price'], 'primary_key': 0, 'unique': 0, 'foreign_key': None, 'check': 1, 'index': 0}}

[LilyFoote]

I think I've resolved the failing test that looked for sql_create_fk appropriately.

I am also in favour of requiring sqlparse for introspection on sqlite. What do you think is the best way to raise an error? The simplest would be to just allow the ImportError to raise normally, either when the introspection module is imported or when get_constraints is called.

[LilyFoote]

As for the Oracle failure, I'm not sure why the explicitly set constraint name isn't being used (or read by get_constraints).

[charettes]

As for the Oracle failure, I'm not sure why the explicitly set constraint name isn't being used (or read by get_constraints).

It seems to be in there but it's upper cased PRICE_GT_DISCOUNTED_PRICE.

[charettes]

@Ian-Foote you probably want to rename table_name_converter to identifier_converter and use there as well

django/django/db/backends/oracle/introspection.py

Lines 93 to 95 in b9cf764

	def table_name_converter(self, name):
	"""Table name comparison is case insensitive under Oracle."""
	return name.lower()

[timgraham]

For sqlparse, you could use a technique similar to:

django/django/db/backends/base/operations.py

Lines 301 to 310 in c99d379

	try:
	import sqlparse
	except ImportError:
	raise ImproperlyConfigured(
	"The sqlparse package is required if you don't split your SQL "
	"statements manually."
	)
	else:
	return [sqlparse.format(statement, strip_comments=True)
	for statement in sqlparse.split(sql) if statement]

unless get_constraints() is called often enough the basically every project that uses SQLite will require it. In that case, it might be better to include a message in sqlite3/base.py similar to the error for requiring psycopg2 and mysqlclient.

[LilyFoote]

I've updated again.

[timgraham]

Some tests need to be skipped on sqlite if sqlparse isn't installed or else we need to make sqlparse a mandatory dependency when running the tests with sqlite (I think we'd add a message in runtests.py for the latter option).

[LilyFoote]

@timgraham I see you've merged some of my related PRs. Are you planning to handle them all, or would you like me to take another look at any of them?

I had been planning to get back to this, but I'm happy for you to take over if you prefer.

[timgraham]

What do you think we should do about the previous point I raised? If we go with the first option, I think it's going to be difficult to identify which tests in new patches need to be skipped. The second option adds some friction for new contributors (but only if they skip pip install -r requirements/py3.txt for some reason).

[LilyFoote]

I think the second option sounds preferable.

[timgraham]

Then I think we need to write to django-developers about making sqlparse a mandatory dependency for SQLite users and how that'll work exactly (i.e. should sqlite3/base.py raise an error or should the error be deferred as it is now).

[LilyFoote]

Are we talking about sqlparse being mandatory for all usage or just for running Django's test suite? I thought you were only proposing the latter.

[timgraham]

I think the question is what percentage of SQLite projects are going to end up needing sqlparse based on their migrations? If it's high enough, it might make for a better user experience to have users install it when they're getting started instead of a lazy error when get_constraints() is called.

I'll push a commit that shows how many migrations tests are affected -- but most of them aren't failing until the cleanup phase of the test (i.e. migrate to zero, which deletes indexes, etc.)

[charettes]

FWIW I'd be in favour of shipping this PR without adding an hard dependency on sqlparse.

The fact get_constraints is only used in tests since the normal migrate path must perform a full table rebuild on constraint addition and removal makes it unlikely for this path to be reached unless table introspection is used.

In all cases the ImproperlyConfigured exception clearly states which adjustments are required just like our other soft dependencies such as PIL.

[LilyFoote]

That works for me.

[timgraham]

I see your point, although it looks like get_constraints() is also called by SchemaEditor.alter_index/unique_together() (via _delete_composed_index() -> _constraint_names()).

Are you in favor of making sqlparse a mandatory dependency for running the tests with SQLite? If so, should we add a helpful message in runtests.py about that (rather than somebody seeing a bunch of errors if they don't have it installed)? In that case, I'd create tests/requirements/sqlite.txt to mirror the requirements for other databases.

[charettes]

I see your point, although it looks like get_constraints() is also called by SchemaEditor.alter_index/unique_together() (via _delete_composed_index() -> _constraint_names()).

Ah right, I forgot we merged get_constraints and get_indexes methods a few releases ago in #7132. I wonder if this change still makes sense in a world where we make a distinction between them at the model level. Given this is the only call maybe could move the index retrieval logic to a get_index method (called by get_constraints()) for SQLite and have sqlite.SchemaEditor._constraint_names special case index=True to call get_index instead? That would make sqlparse only a required dependency on SQLite for Django 2.2 if you want to use Meta.constraints which should be a better compromise. Thoughts?

Are you in favor of making sqlparse a mandatory dependency for running the tests with SQLite? If so, should we add a helpful message in runtests.py about that (rather than somebody seeing a bunch of errors if they don't have it installed)? In that case, I'd create tests/requirements/sqlite.txt to mirror the requirements for other databases.

I think that would make sense!

[tbicr]

Hi, new Check and Unique constraints look very useful, thx for that.

However I wanna to left a few comments about extensibility of current schema. I wrote backend for postgres that apply a few tricks to avoid table AccessExclusive locking for long time, like index generation for UNIQUE constraint or validation for FOREIGN KEY or CHECK constraints, that helps me to avoid downtime on migration. And for me incredibly easy to write own backend just with overriding SQL constant or method that generate SQL. So my points:

1. sql parts composition hard to override with different SQL, for example I need create index and than new UNIQUE/PRIMARY KEY constraint with created index as USING or create CHECK/FOREIGN KEY constraint with NOT VALID keywords and than validate it.
2. no special methods that provide complete sql generation as _create_index_sql for indexes, for example changing sql for CHECK constraint in _alter_field method require to rewrite whole _alter_field method.
3. sql generation (sql parts composition) partly moved from schema to constraints.py, that's a different that Index.create_sql do and bit harder to override by custom backend.

I'll be very appreciate if this points will be accepted.

[timgraham]

Simon, sure, I think the less sqlparse is required, the easier it'll be for users. Do you or Ian want to work on that?

Paveł, I don't think this PR is the right place to bring up your points. Can you create a ticket at http://code.djangoproject.com/ and offer a patch?

[charettes]

@timgraham I can work on it. Does a PR off this branch sounds good to you? We could reorder the commits here after.

[timgraham]

Yes

[charettes]

@Ian-Foote I discovered an issue with this approach is that it will include inlined named foreign key and primary key constraints as well.

https://www.sqlite.org/syntaxdiagrams.html#table-constraint

Django doesn't create such constraint itself but usually the introspection module is supposed to be able to deal with any form of schema. I'll include the adjustments in a following commit. Thanks for the parsing logic by the way, this is really neat.

[LilyFoote]

Thanks for the spot. I had help with the parsing logic from @MarkusH.

[timgraham]

Simon, did you fix this issue in your branch? Feel free to push an update here.

[charettes]

Cool I'll do that later tonight.

[charettes]

@timgraham I wasn't sure how to push to this branch.

Here's the commit to cherry-pick

4a89f24

[timgraham]

Thanks. There might be a bug because when I rebase #10337, self.assertIn('unique_name', constraints) fails on SQLite.

[charettes]

I know what's causing it. Let me rebase #10337 on top of this branch. That's where the token.match(Token.Keyword, 'UNIQUE') should live anyway.

[timgraham]

I did the rebase already, shall I push it to Ian's branch?

[charettes]

yeah please do, that'll save me doing the rebase.

[charettes]

Given SQLite also creates an index for unique constraints will be returned twice?

[timgraham]

There are a few failing schema tests on Oracle because of the identifier_converter() addition. The tests expect an upper case name:

if connection.features.uppercases_column_names:
    constraint_name = constraint_name.upper()

but get_constraints() now returns a lowercase name. Should we remove those lines from the tests and mention this in the release notes?

[LilyFoote]

I think removing those lines from the tests is fine. An alternative would be to override identifier_converter for the oracle backend to leave the name alone or explicitly uppercase it. I don't know which of these is best - I'm not familiar with oracle.

[felixxm]

@timgraham @Ian-Foote I'm OK with removing those lines from tests 👍 . In other introspection methods on Oracle we are using lowercase names, so it will be consistent.

[timgraham]

Pushed a commit to fix the tests. To be squashed or else all identifier_converter() changes could go in a separate commit perhaps.

[LilyFoote]

I'm happy with squashing this.

[timgraham]

After trying to separate out the identifier_converter() changes, the purpose and usage of that API seemed unclear to me (I think it merits its own ticket). I added a commit to instead use uppercases_column_names as in the schema tests. I think we could revisit this in a separate PR if needed. Objections?

[LilyFoote]

I don't remember identifier_converter being an important change, rather just a way to get things working.

[felixxm]

@timgraham Agreed 👍. I think we can completely remove uppercases_column_names and use identifier_converter() more widely (especially in the oracle back-end and tests). It deserves a separate ticket.