Default MetaData kwarg naming_convention can create issues with certain databases

[jvanasco]

• this is a docs ticket; assign to @jvanasco
• reference group discussion: https://groups.google.com/forum/#!topic/sqlalchemy/8xOu-Mlcobo

The alembic docs reference this naming convention: https://alembic.sqlalchemy.org/en/latest/naming.html

convention = {
  "ix": "ix_%(column_0_label)s",
  "uq": "uq_%(table_name)s_%(column_0_name)s",
  "ck": "ck_%(table_name)s_%(constraint_name)s",
  "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
  "pk": "pk_%(table_name)s"
}

metadata = MetaData(naming_convention=convention)

a Boolean/Enum column will try to create a constraint on sqlite by default, but constraint_name is not defined. this will cause an error in Engine.create_all(

Tasks:

[ ] have the docs use custom callables to offer an automatic constraint name.
[ ] long term: descope into another issue a fallback option for constraint name.

see referenced discussion for details of above.

[zzzeek]

the workaround case looks like:

def auto_constraint_name(constraint, table):
    if constraint.name is None or constraint.name == "_unnamed_":
        return "sa_autoname_%s" % str(uuid.uuid4())[0:5]
    else:
        return constraint.name

NAMING_CONVENTION = {
    "auto_constraint_name": auto_constraint_name,
    "ix": 'ix_%(column_0_label)s',
    "uq": "uq_%(table_name)s_%(column_0_name)s",
    "ck": "ck_%(table_name)s_%(auto_constraint_name)s",
    "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
    "pk": "pk_%(table_name)s"
}
_metadata = sqlalchemy.MetaData(naming_convention=NAMING_CONVENTION)

one problem to fix is that the above is convoluted because the internal "unnamed" constraint thing is leaking into the recipe, which is something nobody should have to know about. the logic at https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/sql/naming.py#L147 is involved with this, the _defer_none_name symbol is the issue here that it would be nicer if end-user code wasn't exposed to this.

[jvanasco]

one problem to fix is that the above is convoluted because the internal "unnamed" constraint thing is leaking into the recipe, which is something nobody should have to know about.

That worried me too. So it looks like _defer_none_name is defaulted into _unnamed_ here, and there is (more than) a bit of abstraction across sqlalchemy to use _defer_none_name and _defer_name instead of _NONE_NAME. This makes me want to NOT expose 'unnamed' in text,.

Assuming the recipe stands as is... I think it would be better if there were a 'constant' value that could be imported for the recipe. Maybe something like from sqlalchemy import unnamed_name; which should allow for more backwards compatibility if you change things. so the target namespace might have...

from sqlalchemy.sql.elements import _NONE_NAME
unnamed_name = _NONE_NAME  # making this a new var, so one can not overwrite _NONE_NAME

however...

what if i makedef auto_constraint_name( a sqlalchemy function itself that is imported? that would keep all the logic within sqlalchemy, and users could either

1. use this default auto_naming_convention
2. pass in their own function.

then the alembic/etc docs would be...

from sqlalchemy import auto_constraint_name
NAMING_CONVENTION = {
    "auto_constraint_name": auto_constraint_name,
    ...

The last thing I want to do is add a new feature/function to be supported, but this seems rather lightweight and more beneficial.

I guess this is solving a bit of a fringe issue as (somehow) no one has identified this before - but more open source projects seem to be actively pushing Alembic to manage SqlAlchemy over the past year, so i'm thinking about the potential for issues with new users.

[zzzeek]

what does "auto_constraint_name" do? as always, putting an opinion in here (e.g. it counts _12345 or something) means more bugs and requests to change it

[jvanasco]

what does "auto_constraint_name" do?

Right now, it would just be the code from your example above:

def auto_constraint_name(constraint, table):
    if constraint.name is None or constraint.name == "_unnamed_":
        return "sa_autoname_%s" % str(uuid.uuid4())[0:5]
    else:
        return constraint.name

means more bugs and requests to change it

I can't stress my line above enough: The last thing I want to do is add a new feature/function to be supported,

I'm trying to find a good balance between somehow exposing _unnamed_ and providing for this functionality.

Another approach could also be leaving the naming convention stuff as-is, and catching the sqlite error for not having a constraint provided for (only) BOOL and ENUM during create_tables, then doing a rollback and retrying with an automatically generated name. I'm trying to think of the most delicate way to handle this.

[zzzeek]

what does "auto_constraint_name" do?

Right now, it would just be the code from your example above:

def auto_constraint_name(constraint, table):
    if constraint.name is None or constraint.name == "_unnamed_":
        return "sa_autoname_%s" % str(uuid.uuid4())[0:5]
    else:
        return constraint.name

means more bugs and requests to change it

I can't stress my line above enough: The last thing I want to do is add a new feature/function to be supported,

I don't want the string "autoname" to be in SQLAlchemy. I want the user to define that in the same place they define all their other naming components. it will be cut and pasted from the SQLAlchemy docs, but that is fine, they can see it, and they can change it and they can't come to me with some bizarre issue about a certain hardcoded name. no hardcoding, that is the goal.

we just need a quick way to set up something that works like this:

convention = {
  "fallback_constraint_name": "my_autoname_%(uuid_4)s",
  "ix": "ix_%(column_0_label)s",
  "uq": "uq_%(table_name)s_%(column_0_name)s",
  "ck": "ck_%(table_name)s_%(constraint_name:'my_autoname_%(uuid_4)s')s",
  "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
  "pk": "pk_%(table_name)s"
}

I'm trying to find a good balance between somehow exposing _unnamed_ and providing for this functionality.

exposing "unnamed" is just a bug. the logic that does the end-user function thing needs to be fixed.

Another approach could also be leaving the naming convention stuff as-is, and catching the sqlite error for not having a constraint provided for (only) BOOL and ENUM during create_tables, then doing a rollback and retrying with an automatically generated name. I'm trying to think of the most delicate way to handle this.

that sounds hard

[jvanasco]

we just need a quick way to set up something that works like this:

ok. i'll try to implement this first!

that sounds hard

I thought so too, but it popped in my head and there was a slight chance you'd say "oh there's a hook for that stuff already!"

[zzzeek]

just remembered. a uuid4 isn't sufficient because the whole point of a naming convention is that it should repeat each time. so....the uuid either has to be based on something repeatable, which is hard to do, or we have to use column_0_name etc., which for CHECK constraint is very hard because a CHECK doesn't necessarily have any column objects in it, so perhaps a uuid based on "ck" + the constraint's order in the list of constraints of that particular type, not really sure.

[jvanasco]

are there any other places where a fallback constraint could be invoked? i tried to find some but couldn't; the only spot I found was this initial column setup.

if this is isolated, would it be acceptable to take the first 5 chars of md5("%(table_name)s.%(column_name)s") ? that would be repeatable across uses but have a sufficiently large pool to avoid conflicts.

[zzzeek]

are there any other places where a fallback constraint could be invoked?

do you mean, "constraint_name" but the name is not present? any constraint object can have "None" for the name and any naming convention can use "%(constraint_name)s".

if this is isolated, would it be acceptable to take the first 5 chars of md5("%(table_name)s.%(column_name)s") ? that would be repeatable across uses but have a sufficiently large pool to avoid conflicts.

when you have a CheckConstraint, it is usually created against a string SQL expression, so it has no column names. The name for the constraint must stay constant even if the SQL expression is changed since this is used to track alembic autogenerate.

I think perhaps Boolean and Enum should maybe add additional state to the constraint so that a name can be generated, perhaps a link back to the datatype itself.

[zzzeek]

column_0_name works fine for the CheckConstraint generated by these types:

from sqlalchemy import Boolean
from sqlalchemy import Column
from sqlalchemy import Enum
from sqlalchemy import MetaData
from sqlalchemy import Table
from sqlalchemy.engine.default import DefaultDialect
from sqlalchemy.schema import CreateTable

NAMING_CONVENTION = {
    "ix": 'ix_%(column_0_label)s',
    "uq": "uq_%(table_name)s_%(column_0_name)s",
    "ck": "ck_%(table_name)s_%(column_0_name)s",
    "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
    "pk": "pk_%(table_name)s"
}

m = MetaData(naming_convention=NAMING_CONVENTION)

t = Table(
    't', m, Column('q', Boolean), Column('p', Enum('one', 'two', 'three'))
)

d = DefaultDialect()

d.supports_native_enum = False
d.supports_native_boolean = False

print(
    CreateTable(t).compile(dialect=d)
)

[zzzeek]

generates:

CREATE TABLE t (
	q BOOLEAN, 
	p VARCHAR(5), 
	CONSTRAINT ck_t_q CHECK (q IN (0, 1)), 
	CONSTRAINT ck_t_p CHECK (p IN ('one', 'two', 'three'))
)

[zzzeek]

but if you add this:

CheckConstraint('z > 10')

you get:

 File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/sql/naming.py", line 67, in _key_column_X_name
    return self._column_X(idx).name
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/sql/naming.py", line 47, in _column_X
    return list(self.const.columns)[idx]
IndexError: list index out of range

which also should be improved, there are no columns there so a nicer message should say that

[zzzeek]

another option, the CheckConstraint created by Boolean and Enum is a so-called "type-bound" constraint, so perhaps they could derive from a different key in the naming convention, like "type_ck" that refers to column_0.

[zzzeek]

e.g. there's a flag so we know the CheckConstraint is related to one of these datatypes.

[jvanasco]

I've put the type-bound approach to the top of my task list. This seems the cleanest/simplest, as the issue I'm concerned about is "new users not understanding an implicit check constraint created for a default column type". Having a default constraint name with uuid is a nice feature, but it looks like the type constraint approach would largely eliminate the need for it. All the other ways to create a constraint seem to be explicitly invoked and uniform across database back ends. Throwing an exception in those situations seems fine to me - the reason should be apparent and the developers probably want to name the constraint. I'll handle Improving the error message you brought up in another ticket/pr.

[zzzeek]

I've put the type-bound approach to the top of my task list. This seems the cleanest/simplest, as the issue I'm concerned about is "new users not understanding an implicit check constraint created for a default column type".

of course. yes let's ad support for "type_ck". in 1.4 it can even default to something, since after all we have a default for indexes still in there. This would also eliminate the need for the "unnamed" symbol thing but I'm not sure we can change that part in 1.3, maybe we can but I have to remember how that works.

Having a default constraint name with uuid is a nice feature, but it looks like the type constraint approach would largely eliminate the need for it.

yes uuid I forgot that we need them to be deterministic which makes it less worth it.

All the other ways to create a constraint seem to be explicitly invoked and uniform across database back ends. Throwing an exception in those situations seems fine to me

yes let's make sure the exceptions are very descriptive b.c. right now I think they are just KeyError / TypeError crap, including that column_X_name thing which can easily be improved

• the reason should be apparent and the developers probably want to name the constraint. I'll handle Improving the error message you brought up in another ticket/pr.

great!

[jvanasco]

The PR is shaping up like this:

• add a TypeCheckConstaint that is just a subclass of CheckConstraint with the naming convention:

      "type_ck": "type_ck_%(table_name)s_%(column_0_name)s",
  

• in 1.3: specifying a constraint name will still generate a CheckConstraint, but not specifying a constraint name will generate a TypeCheckConstraint

• in 1.4: everything will generate a TypeCheckConstraint. This avoids introducing a potentially breaking change in 1.3 (assuming people are testing/checking for the constraint types)

[zzzeek]

OK, let me make one change, which is that all Constraint classes have a flag called "_type_bound" that we should use here, it's not worth a whole subclass just for this change, we can change the lookup as:

diff --git a/lib/sqlalchemy/sql/naming.py b/lib/sqlalchemy/sql/naming.py
index 68a6190cfc..4e331a5396 100644
--- a/lib/sqlalchemy/sql/naming.py
+++ b/lib/sqlalchemy/sql/naming.py
@@ -126,12 +126,18 @@ _prefix_dict = {
     ForeignKeyConstraint: "fk",
 }
 
-
-def _get_convention(dict_, key):
-
-    for super_ in key.__mro__:
-        if super_ in _prefix_dict and _prefix_dict[super_] in dict_:
-            return dict_[_prefix_dict[super_]]
+def _get_prefixes(constraint):
+    for super_ in type(constraint).__mro__:
+        if super_ in _prefix_dict:
+            prefix = _prefix_dict[super_]
+            if isinstance(constraint, Constraint) and constraint._type_bound:
+                yield "type_%s" % prefix, super_
+            yield prefix, super_
+
+def _get_convention(dict_, constraint):
+    for prefix, super_ in _get_prefixes(constraint):
+        if prefix in dict_:
+            return dict_[prefix]
         elif super_ in dict_:
             return dict_[super_]
     else:
@@ -140,7 +146,7 @@ def _get_convention(dict_, key):
 
 def _constraint_name_for_table(const, table):
     metadata = table.metadata
-    convention = _get_convention(metadata.naming_convention, type(const))
+    convention = _get_convention(metadata.naming_convention, const)
 
     if isinstance(const.name, conv):
         return const.name

[jvanasco]

Thanks! This is actually great. After some tests on the subclass method, I couldn't get the same class to appear in 1.3 for ENUM because it defaults to using the lowercase .__name__ of the enum class.

[jvanasco]

@zzzeek sorry to pester you on this again.

i've got some test cases running and using your approach as-is, not declaring a type_ck in the naming_convention will fallback onto the ck prefix. is this intentional?

if this is not intentional - altering the if/else slightly, i can limit the yields generated by _get_prefixes to only the type_bound. If this happens though, the result is that no constraint name is utilized

so instead of:

CREATE TABLE "user" (x BOOLEAN, CONSTRAINT type_ck_user_x CHECK (x IN (0, 1)))

we see can see either:

# fallback to ck prefix
CREATE TABLE "user" (x BOOLEAN, CONSTRAINT ck_user_x CHECK (x IN (0, 1)))

# no constraint is created
CREATE TABLE "user" (x BOOLEAN, CHECK (x IN (0, 1)))

Which do you prefer?

[zzzeek]

@zzzeek sorry to pester you on this again.

i've got some test cases running and using your approach as-is, not declaring a type_ck in the naming_convention will fallback onto the ck prefix. is this intentional?

that was what I had in mind....if people put an existing naming convention for "ck" why wouldn't that be used ?

if this is not intentional - altering the if/else slightly, i can limit the yields generated by _get_prefixes to only the type_bound. If this happens though, the result is that no constraint name is utilized

so instead of:

CREATE TABLE "user" (x BOOLEAN, CONSTRAINT type_ck_user_x CHECK (x IN (0, 1)))

we see can see either:

# fallback to ck prefix
CREATE TABLE "user" (x BOOLEAN, CONSTRAINT ck_user_x CHECK (x IN (0, 1)))

# no constraint is created
CREATE TABLE "user" (x BOOLEAN, CHECK (x IN (0, 1)))

Which do you prefer?

if people are putting a naming convention in , that means they don't want DB-generated names at all, so they should try to get a name. falling back to "ck" maintains the current behavior.

[sqla-tester]

jonathan vanasco has proposed a fix for this issue in the master branch:

Fixes #4784 regarding constraints: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2224

[jvanasco]

Note: perhaps use the enum listener example in #5151