Latest migration changes broke Postgres support

[justjanne]

Roughly 1.5 months ago, the project ran just fine with postgres as database backend. Due to new migrations which have been added, and relevant changes, this is now broken.

This would be a minor change, being a huge improvement nonetheless.

[keesbos]

diff --git a/migrations/versions/3f76448bb6de_add_user_confirmed_column.py b/migrations/versions/3f76448bb6de_add_user_confirmed_column.py
index d1b6d29..490aebe 100644
--- a/migrations/versions/3f76448bb6de_add_user_confirmed_column.py
+++ b/migrations/versions/3f76448bb6de_add_user_confirmed_column.py
@@ -18,8 +18,12 @@ depends_on = None
 def upgrade():
     with op.batch_alter_table('user') as batch_op:
         batch_op.add_column(
-            sa.Column('confirmed', sa.Boolean(), nullable=False,
+            sa.Column('confirmed', sa.Boolean(), nullable=True,
                       default=False))
+    with op.batch_alter_table('user') as batch_op:
+        user = sa.sql.table('user', sa.sql.column('confirmed'))
+        batch_op.execute(user.update().values(confirmed=False))
+        batch_op.alter_column('confirmed', nullable=False)

[ngoduykhanh]

Hi,

Can you show your setup and the error message during the migration?
I've tried with PostgreSQL 12 and it looks good to me.

[keesbos]

Error:

# flask db upgrade
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 856bb94b7040 -> b0fea72a3f20, Update domain serial columns type
INFO  [alembic.runtime.migration] Running upgrade b0fea72a3f20 -> 3f76448bb6de, Add user.confirmed column
Traceback (most recent call last):
  File "/opt/powerdns-admin/py-pdns-adm/lib/python3.6/site-packages/sqlalchemy/engine/base.py", line 1246, in _execute_context
    cursor, statement, parameters, context
  File "/opt/powerdns-admin/py-pdns-adm/lib/python3.6/site-packages/sqlalchemy/engine/default.py", line 581, in do_execute
    cursor.execute(statement, parameters)
psycopg2.errors.NotNullViolation: column "confirmed" contains null values

Ubuntu (in lxd container):

# lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.3 LTS
Release:	18.04
Codename:	bionic

Postgresql:

# dpkg -l | grep postg
ii  postgresql                     10+190ubuntu0.1                    all          object-relational SQL database (supported version)
ii  postgresql-10                  10.10-0ubuntu0.18.04.1             amd64        object-relational SQL database, version 10 server
ii  postgresql-client-10           10.10-0ubuntu0.18.04.1             amd64        front-end programs for PostgreSQL 10
ii  postgresql-client-common       190ubuntu0.1                       all          manager for multiple PostgreSQL client versions
ii  postgresql-common              190ubuntu0.1                       all          PostgreSQL database-cluster manager

(Note, the git diff fixes it for me)

It looks like the default value isn't applied to the table with sqlalchemy. With pg_dump:

CREATE TABLE public."user" (
    id integer NOT NULL,
    username character varying(64),
    password character varying(64),
    firstname character varying(64),
    lastname character varying(64),
    email character varying(128),
    otp_secret character varying(16),
    role_id integer,
    confirmed boolean NOT NULL
);

And no alter table set default for the confirmed column.

The only alter table set default statements I see in then dump are for nextval() (i.e. the id columns).

[bjo81]

Any update on this? Same issue here with postgresql-11.

[halkeye]

sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) column "confirmed" contains null values [SQL: ALTER TABLE "user" ADD COLUMN confirmed BOOLEAN NOT NULL]
(Background on this error at: http://sqlalche.me/e/13/gkpj)

So I applied the update manually

ALTER TABLE "user" ADD COLUMN confirmed BOOLEAN NOT NULL default false;
update "user" set confirmed=true;
update "alembic_version" set "version_num"='3f76448bb6de';

Proper fix:
7739bf7#diff-5b6d721374eb2724c78efcc59227e40954dc25bff97a4b6d72123ac5d16cbea2R21 should get a default value to match https://github.com/ngoduykhanh/PowerDNS-Admin/blob/5f10f739ea95f51d539ebd7927d90bba5c5dc7ae/powerdnsadmin/models/user.py#L31

Since i've already applied the migration, I don't have lots of motivation to make a PR.

[joachimtingvold]

Same happens with PostgreSQL 13. The patch from @keesbos works, so that can essentially be used as the PR for fixing it (unless someone wants to refine it more).

(venv) pdnsadmin@dnsadmin1:/srv/vhosts/dnsadmin.foo.bar$ flask db upgrade
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 654298797277 -> 0fb6d23a4863, Remove user avatar
INFO  [alembic.runtime.migration] Running upgrade 0fb6d23a4863 -> 856bb94b7040, Add comment column in domain template record table
INFO  [alembic.runtime.migration] Running upgrade 856bb94b7040 -> b0fea72a3f20, Update domain serial columns type
INFO  [alembic.runtime.migration] Running upgrade b0fea72a3f20 -> 3f76448bb6de, Add user.confirmed column
Traceback (most recent call last):
  File "/srv/vhosts/dnsadmin.foo.bar/venv/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1276, in _execute_context
    self.dialect.do_execute(
  File "/srv/vhosts/dnsadmin.foo.bar/venv/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 593, in do_execute
    cursor.execute(statement, parameters)
psycopg2.errors.NotNullViolation: column "confirmed" of relation "user" contains null values

[...]

  File "/srv/vhosts/dnsadmin.foo.bar/venv/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 593, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) column "confirmed" of relation "user" contains null values

[SQL: ALTER TABLE "user" ADD COLUMN confirmed BOOLEAN NOT NULL]

[jbe-dw]

@keesbos I've not checked the migration scripts but it looks to me that it hasn't been fixed. Can you confirm ?

[jbe-dw]

Related #787

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. PDA is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

[justjanne]

Awesome, thanks! :)