#199 fix testsuite failing with SQLAlchemy < 1.4
Merged 2 months ago by kparal. Opened 2 months ago by kparal.

file modified
+11 -11
@@ -45,23 +45,23 @@ 

      status = db.Column(db.String(80), unique=False)

      """E.g. 'NEW' or 'ASSIGNED'"""

      component = db.Column(db.String(80), unique=False)

-     active = db.Column(db.Boolean)

+     active = db.Column(db.Boolean(create_constraint=True, name='active_bool'))

      """An alias for 'open', i.e. `True` when open, `False` when closed"""

      last_bug_sync = db.Column(db.DateTime)

-     needinfo = db.Column(db.Boolean)

+     needinfo = db.Column(db.Boolean(create_constraint=True, name='needinfo_bool'))

      needinfo_requestee = db.Column(db.String(1024), unique=False)

      last_whiteboard_change = db.Column(db.DateTime)

      """The time when we detected a blocker/FE keyword change. It doesn't really mark *any*

      whiteboard change, just for those tracked keywords."""

-     proposed_blocker = db.Column(db.Boolean)

-     proposed_fe = db.Column(db.Boolean)

-     rejected_blocker = db.Column(db.Boolean)

-     rejected_fe = db.Column(db.Boolean)

-     accepted_blocker = db.Column(db.Boolean)

-     accepted_0day = db.Column(db.Boolean)

-     accepted_prevrel = db.Column(db.Boolean)

-     accepted_fe = db.Column(db.Boolean)

-     prioritized = db.Column(db.Boolean)

+     proposed_blocker = db.Column(db.Boolean(create_constraint=True, name='proposed_blocker_bool'))

+     proposed_fe = db.Column(db.Boolean(create_constraint=True, name='proposed_fe_bool'))

+     rejected_blocker = db.Column(db.Boolean(create_constraint=True, name='rejected_blocker_bool'))

+     rejected_fe = db.Column(db.Boolean(create_constraint=True, name='rejected_fe_bool'))

+     accepted_blocker = db.Column(db.Boolean(create_constraint=True, name='accepted_blocker_bool'))

+     accepted_0day = db.Column(db.Boolean(create_constraint=True, name='accepted_0day_bool'))

+     accepted_prevrel = db.Column(db.Boolean(create_constraint=True, name='accepted_prevrel_bool'))

+     accepted_fe = db.Column(db.Boolean(create_constraint=True, name='accepted_fe_bool'))

+     prioritized = db.Column(db.Boolean(create_constraint=True, name='prioritized_bool'))

      milestone_id = db.Column(db.Integer, db.ForeignKey('milestone.id'))

      milestone: Optional['model_milestone.Milestone'] = db.relationship(

          'Milestone', back_populates='bugs')

@@ -29,7 +29,7 @@ 

      id = db.Column(db.Integer, primary_key=True)

      date_created = db.Column(db.DateTime, unique=False)

      criterion = db.Column(db.String(4096), unique=False)

-     current = db.Column(db.Boolean)

+     current = db.Column(db.Boolean(create_constraint=True, name='current_bool'))

      milestone_id = db.Column(db.Integer, db.ForeignKey('milestone.id'))

      milestone: 'model_milestone.Milestone' = db.relationship('Milestone', back_populates='criteria')

      number = db.Column(db.Integer, unique=False)

@@ -44,10 +44,10 @@ 

      """A bugzilla ticket number representing the tracker for blockers"""

      fe_tracker = db.Column(db.Integer, unique=True)

      """A bugzilla ticket number representing the tracker for freeze exceptions"""

-     active = db.Column(db.Boolean)

+     active = db.Column(db.Boolean(create_constraint=True, name='active_bool'))

      """Only active milestones are synced and displayed"""

      last_bug_sync = db.Column(db.DateTime)

-     current = db.Column(db.Boolean)

+     current = db.Column(db.Boolean(create_constraint=True, name='current_bool'))

      """Current milestone is the most relevant one currently. Usually it is the nearest milestone

      in the future. There should be at most one milestone marked as current."""

      bugs: list['bug.Bug'] = db.relationship('Bug', back_populates='milestone', lazy='dynamic')

@@ -32,9 +32,10 @@ 

      id = db.Column(db.Integer, primary_key=True)

      number = db.Column(db.Integer)

      """E.g. 35 (for Fedora 35)"""

-     active = db.Column(db.Boolean)

+     active = db.Column(db.Boolean(create_constraint=True, name='active_bool'))

      """Only active releases are synced and displayed"""

-     discussions_closed = db.Column(db.Boolean)

+     discussions_closed = db.Column(db.Boolean(create_constraint=True,

+                                               name='discussions_closed_bool'))

      """This is used for past (historical) releases. If this is `True`, all discussion tickets (as in

      `Bug.discussion_link`) have been closed ("cleaned up")."""

      last_update_sync: datetime = db.Column(db.DateTime)

file modified
+1 -1
@@ -32,7 +32,7 @@ 

      stable_karma = db.Column(db.Integer, unique=False)

      status = db.Column(db.String(80), unique=False)

      request = db.Column(db.String(80), unique=False, nullable=True)

-     pending = db.Column(db.Boolean, unique=False)

+     pending = db.Column(db.Boolean(create_constraint=True, name='pending_bool'), unique=False)

      date_submitted = db.Column(db.DateTime)

      date_pushed_testing = db.Column(db.DateTime, nullable=True)

      date_pushed_stable = db.Column(db.DateTime, nullable=True)

The new SQLAlchemy naming convention fails with SQLAlchemy < 1.4 when executed
on SQLite (used in the test suite). The reason is that SQLite doesn't support
Boolean type natively, and SQLAlchemy automatically tries to create a
constraint, but that fails with a constraint name specified. SQLAlchemy 1.4
resolves this issue by not creating constraints at all (for non-native types).
You can read more about it here:
https://github.com/pallets/flask-sqlalchemy/pull/886
https://docs.sqlalchemy.org/en/14/core/constraints.html#configuring-naming-for-boolean-enum-and-other-schema-types

Since we need to support SQLAlchemy 1.3 at the moment (in Fedora 33 and 34),
out of several solutions I decided to name constraints for all Boolean types.
I also specified that the constraint should be created (no reason not to create
it, when we have it named already). Otherwise the contraints would stop being
created once upgraded to SQLAlchemy >= 1.4.

Fixes: https://pagure.io/fedora-qa/blockerbugs/issue/197

Build succeeded.

How about we add a comment there to say why it's done this way, so we can get rid of the workaround, once 1.4 is our target?

@jskladan Originally I intended to add create_constraint=False to each Boolean, and yes, I wanted to add a comment "TODO: this is no longer needed with SQLAlchemy 1.4, remove it". Because create_constraint=False is the new default in 1.4. However, I opted for the opposite approach - make sure the constraint is created in all versions (1.3 and 1.4+), and name it properly, so that it doesn't fail on SQLite. So it's not really a workaround, but instead opting-in into optional behavior. See this:
https://docs.sqlalchemy.org/en/14/changelog/migration_14.html#enum-and-boolean-datatypes-no-longer-default-to-create-constraint

The reason is that if I store something invalid in the DB, I'd like to see it fail even with SQLite, e.g. in the test suite. This even allows me to test these failures in unit tests. I'm not aware of any downside to this approach (except that I need to manually request it with each Boolean).

I could write all this reasoning above one selected = db.Column(db.Boolean(... line, but it seemed excessive (and people looking at other files wouldn't find it anyway). That's why I provided longer explanation in the commit message. Wdyt?

I clearly misunderstood what this does, I thought that the constraint names are overloading the naming conventions, while these only add the relevant parts (%(constraint_name)s) to the naming scheme of 'ck': 'ck_%(table_name)s_%(constraint_name)s'

Forget I said anything, then, LGTM.

Commit 9101de9 fixes this pull-request

Pull-Request has been merged by kparal

2 months ago