make autoincrement=True less surprising in composite PK setups...somehow
E.g. in this example:
class Foo(Base):
__tablename__ = 'foo'
id1 = Column(Integer, primary_key=True, nullable=False)
id2 = Column(Integer, primary_key=True, nullable=False)
I believe it does not make much sense to create id1
as autoincrement/serial while id2
is a plain integer columns.
Unless there are database engines which actually support multiple serial columns, I would either disable autoincrement for both columns in that case or raise an error, forcing the user to enable/disable autoincrement explictly.
Comments (15)
-
repo owner -
reporter But doesn't it make sense to be explicit about this if you actually want this behavior? Usually you have one autoincrementing primary key or no autoincrementing field at all...
-
repo owner maybe if it were version 0.2 but we're heading into 1.0 and i don't know that this change makes anyone's life better. there are definitely users that do something like an "autoincrement + a version counter" type of thing who would be surprised. But also, the way "autoincrement" is, it's a flag set that defaults to True. It makes the autoincrement thing happen any time it's valid, and in this case, it is valid. if we made it not do this, then we need to change autoincrement to a three-value field in order to make this possible, and then it's yet another major API change that might throw people off. it affects literally every integer primary key column out there and I don't know offhand what the impacts of that are going to be, given alembic, sqlalchemy-migrate, etc. we're at that phase where I am constantly amazed at how the most innocuous changes completely blow up openstack and other major projects that are using something in not quite the way you expected.
-
reporter True, probably not a good idea to change the API in such a way.
-
repo owner ill see if i can think of a way to make this less surprising.
-
repo owner - changed milestone to 1.x.xx
- changed title to make autoincrement=True less surprising in composite PK setups...somehow
I dont have an idea here right now, so pushing this out for the moment.
-
I really hate the default behavior of current 'autoincrement' , like the following, I need write " autoincrement=False" time and time again in dozens of tables.
class PhoneNumberConfig(Base): __tablename__ = 'foo' plan= Column(Integer, primary_key=True, autoincrement=False, nullable=False) terminal = Column(Integer, primary_key=True, autoincrement=False, nullable=False) port = Column(SmallInteger, primary_key=True, autoincrement=False, nullable=False) phone_number = Column(Integer, primary_key=True, autoincrement=False, nullable=False)
Why not just like the SQL language, declare autoincrement=True if user wants. For API compatibility, it may gave user a way to change such defaut behavior like some global config.
-
repo owner because it would be extremely backwards incompatible. global config is not an option because many individual parts of an application may be using SQLAlchemy separately. Also I'd not advise storing phone numbers as integers.
For any situation in Python where you don't want to type something, make yourself a def:
def Column(*arg, **kw): kw.setdefault('autoincrement', False) return sqlalchemy.Column(*arg, **kw)
-
repo owner good news everyone! I got bit by this stupid issue now. but bad news! I still have no idea how to solve it without breaking tons of environments.
-
repo owner - new approach to autoincrement, towards references
#3216 - We add a new value of "autoincrement" called "auto". This is now the default for all columns, no longer True.
- the work of determining _autoincrement_column now moves to PrimaryKeyConstraint itself, as "autoincrement" is explictly a pk-only concept
- pkc detects autoincrement at the same time Table did, based on memoized_property. attempts to make this a more "active" setting are more complicated because during table construction with or without reflection the columns within the pkc are in flux and determiniations can't really be made without looking at all columns.
- with the difference between "auto" and True, we now can add validation rules when True is set: 1. only one column may have autoincrement=True 2. autoincrement=True requires an Integer type, error is raised otherwise 3. column default has to be None or a Sequence 4. column server_default has to be None or server-reflected
- when autoincrement=True is not set, we're looking for autoincrement='auto'. We only care about 'auto' when there is only one column in the PK. E.g. we no longer set any column to "autoincrement" implicitly if the key is composite, this requires "autoincrement=True". This is the core of
- we get rid of the awkward rule that we create "KEY idx_autoinc_foo" for a MySQL InnoDB column that is explicit autoincrement in a composite PK and not the first column. that was a terrible idea. when the non-first column in a composite PK is autoincrement=True, we render it first in the PRIMARY KEY constraint. Problem solved, no more implicit index, no more guessing if we're innodb (this guess was wrong in the case of innodb set on the server anyway). this is again a proposal for a big change in behavior; positives include that a surprise KEY is no longer created, won't show up in alembic autogen and generates a more efficient structure.
- databases where "autoincrement" is entirely implicit, e.g. SQLite, should raise an error now when autoincrement=True is set and can't be satisfied; SQLite dialect now raises this if autoincrement=True on a composite primary key since SQLite never does this (note that as always, this is distinct from SQLite's "sqlite_autoincrement" keyword which we support distinctly).
→ <<cset 8f81f07917a7>>
- new approach to autoincrement, towards references
-
repo owner okey doke
-
repo owner - changed status to resolved
- 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. fixes#3216 - The MySQL dialect no longer generates an extra "KEY" directive when generating CREATE TABLE DDL for a table using InnoDB with a composite primary key with AUTO_INCREMENT on a column that isn't the first column; to overcome InnoDB's limitation here, the PRIMARY KEY constraint is now generated with the AUTO_INCREMENT column placed first in the list of columns.
→ <<cset 414af7b61291>>
-
repo owner - update the mssql autoincrement reflection test to accommodate
the new behavior of the autoincrement flag as per ref
#3216
→ <<cset 6713817e1186>>
- update the mssql autoincrement reflection test to accommodate
the new behavior of the autoincrement flag as per ref
-
repo owner - changed status to open
users rejecting the CompileError. change to a warning.
-
repo owner - changed status to resolved
Change autoincrement compileerror to a warning
Users are complaining that IntegrityError is no longer raised.
Change-Id: I0855d5b7a98d4338f0910501b6e6d404ba33634d Fixes:
#3216→ <<cset 8a13957db790>>
- Log in to comment
I'm pretty sure a lot of people want this.