Open AbdealiLoKo opened 2 years ago
Note that after a bunch of debugging, through sqla-compat and alembic code (Neither of which I am familiar with - so, I may be seeing the wrong thing :) )
I found:
>>> from myapp.models import MyTable
>>> from sqlalchemy_continuum import version_class
>>> MyModel.__table__.c.mycol.nullable
False
>>> version_class(MyModel).__table__.c.mycol.nullable
True
>>> from alembic.util import sqla_compat
>>> sqla_compat._copy(MyModel.__table__.c.mycol).nullable
False
>>> sqla_compat._copy(version_class(MyModel).__table__.c.mycol).nullable
False #### <----- This should have been True?
But when I try to reproduce the same issue without sqla-continuum - the nullable is fine:
>>> sqla_compat._copy(sa.Column('mycol', nullable=True)).nullable
True #### <----- This seems correct
Hi @AbdealiJK ,
Was trying to dig into this issue, it seems it gives nullable = True due to a bug in column replication method in table_builder.column_reflector.
Currently it is
if not column_copy.primary_key: column_copy.nullable = True
Changing if condition to
if not column_copy.primary_key and column_copy.nullable: column_copy.nullable = True
fixes the issue.
EDIT: Not sure if it's a bug or a requirement from SQLA Cont. to have non PK columns as Nullable for version table as i can see there is a testcase covering this specific case
I don't understand the suggested fix @indiVar0508 We want the columns to be nullable.
In your code:
if not column_copy.primary_key and column_copy.nullable:
column_copy.nullable = True
The nullable is already True. So you aren't changing anything
Adding the column_copy.nullable
in condition makes sure that if for a non-PK Column nullable is set to False
, the versioned col also follow that property.
to reproduce the above issue mentioned in thread i created below shown demo model.
from sqlalchemy import Column, BigInteger, Integer
from sqlalchemy.orm import declarative_base, configure_mappers
from sqlalchemy_continuum import make_versioned
Base = declarative_base()
make_versioned(user_cls=None)
class myTable(Base):
__versioned__ = {}
__tablename__='my_table'
id = Column(Integer, primary_key=True, autoincrement=True)
myCol = Column(BigInteger, autoincrement=True, nullable=False)
configure_mappers()
and used below mentioned command(s) to generate issue
alembic revision --autogenerate -m "first_commit"
alembic upgrade head
alembic revision --autogenerate -m "without_change_commit"
alembic revision --autogenerate -m "without_change_commit" INFO [alembic.runtime.migration] Context impl SQLiteImpl. INFO [alembic.runtime.migration] Will assume non-transactional DDL. INFO [alembic.autogenerate.compare] Detected NULL on column 'my_table_version.myCol' Generating /home/sqlalchemy- continuum/tracker_with_change/versions/4a48e12e7a8d_without_change_commit.py ... done
and after that alembic generated upgrade and downgrade methods post commit without_change_commit
as shown below
def upgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
op.alter_column('my_table_version', 'myCol',
existing_type=sa.INTEGER(), # In my case even column type changed, i am assuming it happened because there was no data in table(s)
nullable=True,
autoincrement=False)
# ### end Alembic commands ###
def downgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
op.alter_column('my_table_version', 'myCol',
existing_type=sa.INTEGER(),
nullable=False,
autoincrement=False)
# ### end Alembic commands ###
but after adding that condition to add a condn that non-PK col can also have nullable=False
when i ran for new sqlite file with new alembic init and then second commit without_change_commit
it generated empty upgrade and downgrade methods with no changes detected
alembic revision --autogenerate -m "without_change_commit" INFO [alembic.runtime.migration] Context impl SQLiteImpl. INFO [alembic.runtime.migration] Will assume non-transactional DDL. Generating /home/sqlalchemy- continuum/tracker_with_change/versions/19acafbbcbfc_without_change_commit.py ... done
def upgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### pass # ### end Alembic commands ###
def downgrade() -> None:
pass
# ### end Alembic commands ###
i might be wrong also here as later i saw that a testcase is there specific for this behavior.
version i had while trying to replicate the issue
Flask-SQLAlchemy 2.5.1
marshmallow-sqlalchemy 0.28.1
SQLAlchemy 1.4.40
SQLAlchemy-Continuum 1.3.12
SQLAlchemy-i18n 1.1.0
SQLAlchemy-Utils 0.38.3
sqlalchemy2-stubs 0.0.2a24
Found that this was happenign because of the col._copy()
function in sqlalchemy
After we "copy" the column - we are setting the nullable = True/False
But the value for _user_defined_nullable
is still the previous value from the parent table.
So, when we COPY - the parent table's nullable
is used.
https://github.com/sqlalchemy/sqlalchemy/blob/rel_1_4_41/lib/sqlalchemy/sql/schema.py#L2018
This seems to be a weird issue.
When I add
__versioned__
for the first time to a model and create an alembic migration with:It gives:
Then when I run:
I was expecting it to say "no changes found" But it does detect changes and is saying that
nullable=True
now:This seems to be consistently reproducible - the first time alembic is generating
nullable=False
. Then second time alembic is generatingnullable=True
Versions I use: