- more tests

- changelog, migration, docs and all that

5 files changed, 239 insertions, 21 deletions

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index e376fe191..f599ee018 100644
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -22,6 +22,31 @@
     :version: 1.1.0b1
 
     .. change::
+        :tags: change, sql, mysql
+        :tickets: 3216
+
+        The system by which a :class:`.Column` considers itself to be an
+        "auto increment" column has been changed, such that autoincrement
+        is no longer implicitly enabled for a :class:`.Table` that has a
+        composite primary key.  In order to accommodate being able to enable
+        autoincrement for a composite PK member column while at the same time
+        maintaining SQLAlchemy's long standing behavior of enabling
+        implicit autoincrement for a single integer primary key, a third
+        state has been added to the :paramref:`.Column.autoincrement` parameter
+        ``"auto"``, which is now the default.  Additionally, the PRIMARY KEY
+        constraint generated for a table will now always place the autoincrement
+        column first, which in particular removes the need for an extra
+        "KEY" directive that was needed by MySQL.
+
+        .. seealso::
+
+            :ref:`change_3216`
+
+        :paramref:`.Column.autoincrement` parameter has been changed to
+        now feature three distinct states, False, True and ``'auto'``, the
+        default being ``'auto'``.
+
+    .. change::
         :tags: change, sqlite
         :pullreq: github:198
 
diff --git a/doc/build/changelog/migration_11.rst b/doc/build/changelog/migration_11.rst
index 78f77e694..d2e8d0f04 100644
--- a/doc/build/changelog/migration_11.rst
+++ b/doc/build/changelog/migration_11.rst
@@ -256,6 +256,59 @@ configuration of the existing object-level technique of assigning
 New Features and Improvements - Core
 ====================================
 
+.. _change_3216:
+
+The ``.autoincrement`` directive no longer implicitly enables for a composite primary key column
+-------------------------------------------------------------------------------------------------
+
+SQLAlchemy has always had the convenience feature of enabling the backend database's
+"autoincrement" feature for a single-column integer primary key; by "autoincrement"
+we mean that the back-end database column will be SERIAL on the Postgresql backend
+or AUTO_INCREMENT on the MySQL backend, and additionally that the :meth:`.Table.insert`
+construct will be aware that some form of autoincrement is present and provide for
+this automatically generated value to be available after the INSERT proceeds.
+
+What's changed is that this feature no longer turns on automatically for a
+*composite* primary key; previously, a table definition such as::
+
+    Table(
+        'some_table', metadata,
+        Column('x', Integer, primary_key=True),
+        Column('y', Integer, primary_key=True)
+    )
+
+Would have "autoincrement" semantics applied to the ``'x'`` column, only
+because it's first in the list of primary key columns.  In order to
+disable this, one would have to turn off ``autoincrement`` on all columns::
+
+    # old way
+    Table(
+        'some_table', metadata,
+        Column('x', Integer, primary_key=True, autoincrement=False),
+        Column('y', Integer, primary_key=True, autoincrement=False)
+    )
+
+With the new behavior, the composite primary key will not have autoincrement
+semantics unless a column is marked explcitly with ``autoincrement=True``::
+
+    # column 'y' will be SERIAL/AUTO_INCREMENT/ auto-generating
+    Table(
+        'some_table', metadata,
+        Column('x', Integer, primary_key=True),
+        Column('y', Integer, primary_key=True, autoincrement=True)
+    )
+
+In order to anticipate some potential backwards-incompatible scenarios,
+the :meth:`.Table.insert` construct will perform more thorough checks
+for missing primary key values on composite primary key columns that don't
+have autoincrement set up.
+
+
+.. seealso::
+
+    :ref:`change_mysql_3216`
+
+:ticket:`3216`
 
 .. _change_2528:
 
@@ -787,6 +840,57 @@ emits::
 Dialect Improvements and Changes - MySQL
 =============================================
 
+.. _change_mysql_3216:
+
+No more generation of an implicit KEY for composite primary key w/ AUTO_INCREMENT
+---------------------------------------------------------------------------------
+
+The MySQL dialect had the behavior such that if a composite primary key
+on an InnoDB table featured AUTO_INCREMENT on one of its columns which was
+not the first column, e.g.::
+
+    t = Table(
+        'some_table', metadata,
+        Column('x', Integer, primary_key=True, autoincrement=False),
+        Column('y', Integer, primary_key=True, autoincrement=True),
+        mysql_engine='InnoDB'
+    )
+
+DDL such as the following would be generated::
+
+    CREATE TABLE some_table (
+        x INTEGER NOT NULL,
+        y INTEGER NOT NULL AUTO_INCREMENT,
+        PRIMARY KEY (x, y),
+        KEY idx_autoinc_y (y)
+    )ENGINE=InnoDB
+
+Note the above "KEY" with an auto-generated name; this is a change that
+found its way into the dialect many years ago in response to the issue that
+the AUTO_INCREMENT would otherwise fail on InnoDB without this additional KEY.
+
+This workaround has been removed and replaced with the much better system
+of just stating the AUTO_INCREMENT column *first* within the primary key::
+
+    CREATE TABLE some_table (
+        x INTEGER NOT NULL,
+        y INTEGER NOT NULL AUTO_INCREMENT,
+        PRIMARY KEY (y, x)
+    )ENGINE=InnoDB
+
+Along with the change :ref:`change_3216`, composite primary keys with
+or without auto increment are now easier to specify; ``autoincrement``
+now defaults to the value ``"auto"`` and the ``autoincrement=False``
+directives are no longer needed::
+
+    t = Table(
+        'some_table', metadata,
+        Column('x', Integer, primary_key=True),
+        Column('y', Integer, primary_key=True, autoincrement=True),
+        mysql_engine='InnoDB'
+    )
+
+
 
 Dialect Improvements and Changes - SQLite
 =============================================
diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py
index cc7076ac3..eaad680d6 100644
--- a/lib/sqlalchemy/sql/crud.py
+++ b/lib/sqlalchemy/sql/crud.py
@@ -212,9 +212,7 @@ def _scan_cols(
 
     for c in cols:
         col_key = _getattr_col_key(c)
-        #if col_key == "id":
-        #    import pdb
-        #    pdb.set_trace()
+
         if col_key in parameters and col_key not in check_columns:
 
             _append_param_parameter(
@@ -230,7 +228,6 @@ def _scan_cols(
                         c is not stmt.table._autoincrement_column
                     ):
 
-                # MARKMARK
                 if implicit_returning:
                     _append_param_insert_pk_returning(
                         compiler, stmt, c, values, kw)
diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
index 8ae2441c5..6bcfd0a78 100644
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -904,17 +904,26 @@ class Column(SchemaItem, ColumnClause):
           argument is available such as ``server_default``, ``default``
           and ``unique``.
 
-        :param autoincrement: This flag may be set to ``False`` to
-          indicate an integer primary key column that should not be
-          considered to be the "autoincrement" column, that is
-          the integer primary key column which generates values
-          implicitly upon INSERT and whose value is usually returned
-          via the DBAPI cursor.lastrowid attribute.   It defaults
-          to ``True`` to satisfy the common use case of a table
-          with a single integer primary key column.  If the table
-          has a composite primary key consisting of more than one
-          integer column, set this flag to True only on the
-          column that should be considered "autoincrement".
+        :param autoincrement: Set up "auto increment" semantics for an integer
+          primary key column.  The default value is the string value ``auto``
+          which indicates that a single-column primary key that is of
+          an INTEGER type should receive auto increment semantics.  This
+          includes that back end DDL such as Postgresql SERIAL or MySQL
+          AUTO_INCREMENT will be emitted for this column during a table
+          create, as well as that the column is assumed to generate new
+          integer primary key values when an INSERT statement invokes which
+          will be retrieved by the dialect.
+
+          The flag may be set to ``True`` to indicate that a column which
+          is part of a composite (e.g. multi-column) primary key should
+          have autoincrement semantics, though note that only one column
+          within a primary key may have this setting.   It can also be
+          set to ``False`` on a single-column primary key that has a
+          datatype of INTEGER, but auto increment semantics are not desired.
+
+          .. versionchanged:: 1.1 The autoincrement flag now defaults to
+             ``"auto"`` which indicates autoincrement semantics by default
+             for single-column integer primary keys only, not composite.
 
           The setting *only* has an effect for columns which are:
 
@@ -960,12 +969,6 @@ class Column(SchemaItem, ColumnClause):
             to generate primary key identifiers (i.e. Firebird, Postgresql,
             Oracle).
 
-          .. versionchanged:: 0.7.4
-              ``autoincrement`` accepts a special value ``'ignore_fk'``
-              to indicate that autoincrementing status regardless of foreign
-              key references.  This applies to certain composite foreign key
-              setups, such as the one demonstrated in the ORM documentation
-              at :ref:`post_update`.
 
         :param default: A scalar, Python callable, or
             :class:`.ColumnElement` expression representing the
diff --git a/test/sql/test_insert.py b/test/sql/test_insert.py
index f66f0b391..7e5e1699e 100644
--- a/test/sql/test_insert.py
+++ b/test/sql/test_insert.py
@@ -390,6 +390,95 @@ class InsertTest(_InsertTestBase, fixtures.TablesTest, AssertsCompiledSQL):
             checkparams={"name_1": "foo"}
         )
 
+    def test_anticipate_no_pk_composite_pk(self):
+        t = Table(
+            't', MetaData(), Column('x', Integer, primary_key=True),
+            Column('y', Integer, primary_key=True)
+        )
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.y' is marked as a member.*"
+            "Note that as of SQLAlchemy 1.1,",
+            t.insert().compile, column_keys=['x']
+
+        )
+
+    def test_anticipate_no_pk_composite_pk_implicit_returning(self):
+        t = Table(
+            't', MetaData(), Column('x', Integer, primary_key=True),
+            Column('y', Integer, primary_key=True)
+        )
+        d = postgresql.dialect()
+        d.implicit_returning = True
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.y' is marked as a member.*"
+            "Note that as of SQLAlchemy 1.1,",
+            t.insert().compile, dialect=d, column_keys=['x']
+
+        )
+
+    def test_anticipate_no_pk_composite_pk_prefetch(self):
+        t = Table(
+            't', MetaData(), Column('x', Integer, primary_key=True),
+            Column('y', Integer, primary_key=True)
+        )
+        d = postgresql.dialect()
+        d.implicit_returning = False
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.y' is marked as a member.*"
+            "Note that as of SQLAlchemy 1.1,",
+            t.insert().compile, dialect=d, column_keys=['x']
+
+        )
+
+    def test_anticipate_no_pk_non_composite_pk(self):
+        t = Table(
+            't', MetaData(),
+            Column('x', Integer, primary_key=True, autoincrement=False),
+            Column('q', Integer)
+        )
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.x' is marked as a member.*"
+            "may not store NULL.$",
+            t.insert().compile, column_keys=['q']
+
+        )
+
+    def test_anticipate_no_pk_non_composite_pk_implicit_returning(self):
+        t = Table(
+            't', MetaData(),
+            Column('x', Integer, primary_key=True, autoincrement=False),
+            Column('q', Integer)
+        )
+        d = postgresql.dialect()
+        d.implicit_returning = True
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.x' is marked as a member.*"
+            "may not store NULL.$",
+            t.insert().compile, dialect=d, column_keys=['q']
+
+        )
+
+    def test_anticipate_no_pk_non_composite_pk_prefetch(self):
+        t = Table(
+            't', MetaData(),
+            Column('x', Integer, primary_key=True, autoincrement=False),
+            Column('q', Integer)
+        )
+        d = postgresql.dialect()
+        d.implicit_returning = False
+        assert_raises_message(
+            exc.CompileError,
+            "Column 't.x' is marked as a member.*"
+            "may not store NULL.$",
+            t.insert().compile, dialect=d, column_keys=['q']
+
+        )
+
 
 class InsertImplicitReturningTest(
         _InsertTestBase, fixtures.TablesTest, AssertsCompiledSQL):