From 3ebc3710a72c9bb724e7074ef0409ae69cfc39fe Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Thu, 7 Jan 2021 21:22:52 +0100 Subject: [PATCH] ``Identity`` implies ``nullable=False``. Altered the behavior of the :class:`_schema.Identity` construct such that when applied to a :class:`_schema.Column`, it will automatically imply that the value of :paramref:`_sql.Column.nullable` should default to ``False``, in a similar manner as when the :paramref:`_sql.Column.primary_key` parameter is set to ``True``. This matches the default behavior of all supporting databases where ``IDENTITY`` implies ``NOT NULL``. The PostgreSQL backend is the only one that supports adding ``NULL`` to an ``IDENTITY`` column, which is here supported by passing a ``True`` value for the :paramref:`_sql.Column.nullable` parameter at the same time. Fixes: #5775 Change-Id: I0516d506ff327cff35cda605e8897a27440e0373 --- doc/build/changelog/unreleased_14/5775.rst | 14 +++++ lib/sqlalchemy/dialects/oracle/base.py | 2 +- lib/sqlalchemy/dialects/postgresql/base.py | 9 ++-- lib/sqlalchemy/sql/compiler.py | 2 +- lib/sqlalchemy/sql/schema.py | 59 ++++++++++++++++------ test/dialect/postgresql/test_compiler.py | 19 +++++++ test/sql/test_identity_column.py | 37 ++++++++++++-- test/sql/test_metadata.py | 24 +++++++++ 8 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/5775.rst diff --git a/doc/build/changelog/unreleased_14/5775.rst b/doc/build/changelog/unreleased_14/5775.rst new file mode 100644 index 0000000000..8a2786f542 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5775.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: changed, schema + :tickets: 5775 + + Altered the behavior of the :class:`_schema.Identity` construct such that + when applied to a :class:`_schema.Column`, it will automatically imply that + the value of :paramref:`_sql.Column.nullable` should default to ``False``, + in a similar manner as when the :paramref:`_sql.Column.primary_key` + parameter is set to ``True``. This matches the default behavior of all + supporting databases where ``IDENTITY`` implies ``NOT NULL``. The + PostgreSQL backend is the only one that supports adding ``NULL`` to an + ``IDENTITY`` column, which is here supported by passing a ``True`` value + for the :paramref:`_sql.Column.nullable` parameter at the same time. + diff --git a/lib/sqlalchemy/dialects/oracle/base.py b/lib/sqlalchemy/dialects/oracle/base.py index a496f03543..4754beebeb 100644 --- a/lib/sqlalchemy/dialects/oracle/base.py +++ b/lib/sqlalchemy/dialects/oracle/base.py @@ -38,7 +38,7 @@ The CREATE TABLE for the above :class:`_schema.Table` object would be: .. sourcecode:: sql CREATE TABLE mytable ( - id INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 3) NOT NULL, + id INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 3), ..., PRIMARY KEY (id) ) diff --git a/lib/sqlalchemy/dialects/postgresql/base.py b/lib/sqlalchemy/dialects/postgresql/base.py index 7a898cb8ac..fd258bc5a2 100644 --- a/lib/sqlalchemy/dialects/postgresql/base.py +++ b/lib/sqlalchemy/dialects/postgresql/base.py @@ -68,8 +68,7 @@ The CREATE TABLE for the above :class:`_schema.Table` object would be: .. sourcecode:: sql CREATE TABLE data ( - id INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 42 CYCLE) - NOT NULL, + id INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 42 CYCLE), data VARCHAR, PRIMARY KEY (id) ) @@ -107,7 +106,7 @@ The CREATE TABLE for the above :class:`_schema.Table` object would be: Will generate on the backing database as:: CREATE TABLE t ( - id INT GENERATED BY DEFAULT AS IDENTITY NOT NULL, + id INT GENERATED BY DEFAULT AS IDENTITY, data VARCHAR, PRIMARY KEY (id) ) @@ -2325,8 +2324,10 @@ class PGDDLCompiler(compiler.DDLCompiler): if column.identity is not None: colspec += " " + self.process(column.identity) - if not column.nullable: + if not column.nullable and not column.identity: colspec += " NOT NULL" + elif column.nullable and column.identity: + colspec += " NULL" return colspec def visit_check_constraint(self, constraint): diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index 45cb75c252..ef7db0bbe5 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -4130,7 +4130,7 @@ class DDLCompiler(Compiled): if column.identity is not None: colspec += " " + self.process(column.identity) - if not column.nullable: + if not column.nullable and not column.identity: colspec += " NOT NULL" return colspec diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index 36d69456ef..f420eb6210 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -70,6 +70,17 @@ BLANK_SCHEMA = util.symbol( """, ) +NULL_UNSPECIFIED = util.symbol( + "NULL_UNSPECIFIED", + """Symbol indicating the "nullable" keyword was not passed to a Column. + + Normally we would expect None to be acceptable for this but some backends + such as that of SQL Server place special signficance on a "nullability" + value of None. + + """, +) + def _get_table_key(name, schema): if schema is None: @@ -1257,11 +1268,18 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause): phrase to be added when generating DDL for the column. When ``True``, will normally generate nothing (in SQL this defaults to "NULL"), except in some very specific backend-specific edge cases - where "NULL" may render explicitly. Defaults to ``True`` unless - :paramref:`_schema.Column.primary_key` is also ``True``, - in which case it - defaults to ``False``. This parameter is only used when issuing - CREATE TABLE statements. + where "NULL" may render explicitly. + Defaults to ``True`` unless :paramref:`_schema.Column.primary_key` + is also ``True`` or the column specifies a :class:`_sql.Identity`, + in which case it defaults to ``False``. + This parameter is only used when issuing CREATE TABLE statements. + + .. note:: + + When the column specifies a :class:`_sql.Identity` this + parameter is in general ignored by the DDL compiler. The + PostgreSQL database allows nullable identity column by + setting this parameter to ``True`` explicitly. :param onupdate: A scalar, Python callable, or :class:`~sqlalchemy.sql.expression.ClauseElement` representing a @@ -1393,8 +1411,17 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause): super(Column, self).__init__(name, type_) self.key = kwargs.pop("key", name) - self.primary_key = kwargs.pop("primary_key", False) - self.nullable = kwargs.pop("nullable", not self.primary_key) + self.primary_key = primary_key = kwargs.pop("primary_key", False) + + self._user_defined_nullable = udn = kwargs.pop( + "nullable", NULL_UNSPECIFIED + ) + + if udn is not NULL_UNSPECIFIED: + self.nullable = udn + else: + self.nullable = not primary_key + self.default = kwargs.pop("default", None) self.server_default = kwargs.pop("server_default", None) self.server_onupdate = kwargs.pop("server_onupdate", None) @@ -1466,10 +1493,6 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause): def _extra_kwargs(self, **kwargs): self._validate_dialect_kwargs(kwargs) - # @property - # def quote(self): - # return getattr(self.name, "quote", None) - def __str__(self): if self.name is None: return "(no name)" @@ -1664,12 +1687,14 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause): if isinstance(type_, SchemaEventTarget): type_ = type_.copy(**kw) + if self._user_defined_nullable is not NULL_UNSPECIFIED: + column_kwargs["nullable"] = self._user_defined_nullable + c = self._constructor( name=self.name, type_=type_, key=self.key, primary_key=self.primary_key, - nullable=self.nullable, unique=self.unique, system=self.system, # quote=self.quote, # disabled 2013-08-27 (commit 031ef080) @@ -3596,7 +3621,8 @@ class PrimaryKeyConstraint(ColumnCollectionConstraint): for c in self.columns: c.primary_key = True - c.nullable = False + if c._user_defined_nullable is NULL_UNSPECIFIED: + c.nullable = False if table_pks: self.columns.extend(table_pks) @@ -4777,9 +4803,12 @@ class Identity(IdentityOptions, FetchedValue, SchemaItem): "autoincrement=False" ) self.column = parent + parent.identity = self - # self.column.server_onupdate = self - self.column.server_default = self + if parent._user_defined_nullable is NULL_UNSPECIFIED: + parent.nullable = False + + parent.server_default = self def _as_for_update(self, for_update): return self diff --git a/test/dialect/postgresql/test_compiler.py b/test/dialect/postgresql/test_compiler.py index b3a0b9bbde..ec165960f4 100644 --- a/test/dialect/postgresql/test_compiler.py +++ b/test/dialect/postgresql/test_compiler.py @@ -1778,6 +1778,25 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): "(INCREMENT BY 7 START WITH 4))", ) + def test_column_identity_null(self): + # all other tests are in test_identity_column.py + m = MetaData() + t = Table( + "t", + m, + Column( + "y", + Integer, + Identity(always=True, start=4, increment=7), + nullable=True, + ), + ) + self.assert_compile( + schema.CreateTable(t), + "CREATE TABLE t (y INTEGER GENERATED ALWAYS AS IDENTITY " + "(INCREMENT BY 7 START WITH 4) NULL)", + ) + def test_index_extra_include_1(self): metadata = MetaData() tbl = Table( diff --git a/test/sql/test_identity_column.py b/test/sql/test_identity_column.py index 3ac97db92f..1ce15f38c9 100644 --- a/test/sql/test_identity_column.py +++ b/test/sql/test_identity_column.py @@ -102,7 +102,7 @@ class _IdentityDDLFixture(testing.AssertsCompiledSQL): CreateTable(t), "CREATE TABLE foo_table (" "foo INTEGER GENERATED ALWAYS AS IDENTITY (START " - "WITH 3) NOT NULL, UNIQUE (foo))", + "WITH 3), UNIQUE (foo))", ) def test_autoincrement_true(self): @@ -120,10 +120,41 @@ class _IdentityDDLFixture(testing.AssertsCompiledSQL): self.assert_compile( CreateTable(t), "CREATE TABLE foo_table (" - "foo INTEGER GENERATED ALWAYS AS IDENTITY (START WITH 3) NOT NULL" + "foo INTEGER GENERATED ALWAYS AS IDENTITY (START WITH 3)" ", PRIMARY KEY (foo))", ) + def test_nullable_kwarg(self): + t = Table( + "t", + MetaData(), + Column("a", Integer(), Identity(), nullable=False), + Column("b", Integer(), Identity(), nullable=True), + Column("c", Integer(), Identity()), + ) + + is_(t.c.a.nullable, False) + is_(t.c.b.nullable, True) + is_(t.c.c.nullable, False) + + nullable = "" + if getattr(self, "__dialect__", None) != "default" and testing.against( + "postgresql" + ): + nullable = " NULL" + + self.assert_compile( + CreateTable(t), + ( + "CREATE TABLE t (" + "a INTEGER GENERATED BY DEFAULT AS IDENTITY, " + "b INTEGER GENERATED BY DEFAULT AS IDENTITY%s, " + "c INTEGER GENERATED BY DEFAULT AS IDENTITY" + ")" + ) + % nullable, + ) + class IdentityDDL(_IdentityDDLFixture, fixtures.TestBase): # this uses the connection dialect @@ -167,7 +198,7 @@ class NotSupportingIdentityDDL(testing.AssertsCompiledSQL, fixtures.TestBase): Column("foo", Integer(), Identity("always", start=3)), ) self.assert_compile( - CreateTable(t), "CREATE TABLE foo_table (foo INTEGER)" + CreateTable(t), "CREATE TABLE foo_table (foo INTEGER NOT NULL)" ) diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 9bf351f5c4..17228dad61 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -1838,6 +1838,30 @@ class TableTest(fixtures.TestBase, AssertsCompiledSQL): assert not t2.c.x.nullable assert not t1.c.x.nullable + def test_pk_can_be_nullable(self): + m = MetaData() + + t1 = Table( + "t1", + m, + Column("x", Integer, nullable=True), + PrimaryKeyConstraint("x"), + ) + + t2 = Table( + "t2", m, Column("x", Integer, primary_key=True, nullable=True) + ) + + eq_(list(t1.primary_key), [t1.c.x]) + + eq_(list(t2.primary_key), [t2.c.x]) + + assert t1.c.x.primary_key + assert t2.c.x.primary_key + + assert t2.c.x.nullable + assert t1.c.x.nullable + def test_must_exist(self): with testing.expect_raises_message( exc.InvalidRequestError, "Table 'foo' not defined" -- 2.47.2