PR#199: fix testsuite failing with SQLAlchemy < 1.4 - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#199 fix testsuite failing with SQLAlchemy < 1.4

Merged 3 years ago by kparal. Opened 3 years ago by kparal.

bug/sqlite_constraints into develop

fix testsuite failing with SQLAlchemy < 1.4

Kamil Páral • 3 years ago

7091620

blockerbugs/models/bug.py

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')`

blockerbugs/models/criterion.py

file modified

+1 -1

		`@@ -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)`

blockerbugs/models/milestone.py

file modified

+2 -2

		`@@ -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')`

blockerbugs/models/release.py

file modified

+3 -2

		`@@ -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)`

blockerbugs/models/update.py

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)`

kparal commented 3 years ago

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

zuul commented 3 years ago

Build succeeded.

blockerbugs-pytest : SUCCESS in 1m 46s

jskladan commented 3 years ago

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?

kparal commented 3 years ago

@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?

jskladan commented 3 years ago

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.

kparal commented 3 years ago

Commit 9101de9 fixes this pull-request

Pull-Request has been merged by kparal

3 years ago