PR#191: db models: replace backref= with back_populates= - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#191 db models: replace backref= with back_populates=

Closed 3 years ago by kparal. Opened 3 years ago by kparal.

feature/relationships into develop

db models: replace backref= with back_populates=

Kamil Páral • 3 years ago

0c6ff08

blockerbugs/models/__init__.py

file modified

+14

		`@@ -23,6 +23,20 @@`
		`import re`
		`from typing import Any`

		`+ from blockerbugs import db`
		`+`
		`+ update_fixes: Any = db.Table(`
		`+ 'update_fixes',`
		`+ db.Column('bug_id', db.Integer, db.ForeignKey('bug.id')),`
		`+ db.Column('update_id', db.Integer, db.ForeignKey('update.id'))`
		`+ )`
		`+`
		`+ update_milestones: Any = db.Table(`
		`+ 'update_milestones',`
		`+ db.Column('milestone_id', db.Integer, db.ForeignKey('milestone.id')),`
		`+ db.Column('update_id', db.Integer, db.ForeignKey('update.id'))`
		`+ )`
		`+`

		`class EnumSymbol(object):`
		`"""Define a fixed symbol tied to a parent class."""`

blockerbugs/models/bug.py

file modified

+13 -8

		`@@ -19,13 +19,13 @@`

		`"""ORM class for bug information stored in the database"""`

		`- from blockerbugs import db, BaseModel`
		`import datetime`
		`import json`
		`from typing import Any, Optional`

		`- # unused imports, but required by dynamic relationships:`
		`- from blockerbugs.models.milestone import Milestone`
		`+ from blockerbugs import db, BaseModel, models`
		`+ import blockerbugs.models.milestone as model_milestone`
		`+ import blockerbugs.models.update as model_update`


		`class Bug(BaseModel):`
		`@@ -63,8 +63,8 @@`
		`accepted_fe = db.Column(db.Boolean)`
		`prioritized = db.Column(db.Boolean)`
		`milestone_id = db.Column(db.Integer, db.ForeignKey('milestone.id'))`
		`- milestone: Optional[Milestone] = db.relationship(`
		`- 'Milestone', backref=db.backref('bugs', lazy='dynamic'))`
		`+ milestone: Optional['model_milestone.Milestone'] = db.relationship(`
		`+ 'Milestone', back_populates='bugs')`
		`discussion_link = db.Column(db.String, unique=False)`
		`votes: str = db.Column(db.Text, default='{}', nullable=False)`
		`"""A JSON representation of dict of all BugVoteTrackers and corresponding output of`
		`@@ -77,6 +77,9 @@`
		```"""
		`_depends_on: str = db.Column('depends_on', db.Text, default='[]', nullable=False)`
		`"""A JSON list of bug numbers which this bug depends on"""`
		`+ updates: list['model_update.Update'] = db.relationship(`
		`+ 'Update', secondary=models.update_fixes, back_populates='bugs', lazy='dynamic',`
		`+ order_by=(model_update.Update.status.desc(), model_update.Update.pending.desc()))`

		`def __init__(self,`
		`bugid: Optional[int],`
		`@@ -84,7 +87,7 @@`
		`summary: Optional[str],`
		`status: Optional[str],`
		`component: Optional[str],`
		`- milestone: Optional[Milestone],`
		`+ milestone: Optional['model_milestone.Milestone'],`
		`active: Optional[bool],`
		`needinfo: Optional[bool],`
		`needinfo_requestee: Optional[str],`
		`@@ -126,7 +129,8 @@`
		`def __repr__(self):`
		`return '<bug %d: %s>' % (self.bugid, self.summary)`

		`- def update(self, buginfo: dict, tracker_type: str, milestone: Milestone) -> None:`
		`+ def update(self, buginfo: dict, tracker_type: str, milestone: 'model_milestone.Milestone'`
		`+ ) -> None:`
		"""Update this Bug with new values. See `Bug.from_data()` for the description of
		`arguments.`
		`"""`
		`@@ -163,7 +167,8 @@`
		`'as separate instances!')`

		`@classmethod`
		`- def from_data(cls, buginfo: dict[str, Any], milestone: Milestone, tracker_type: str) -> 'Bug':`
		`+ def from_data(cls, buginfo: dict[str, Any], milestone: 'model_milestone.Milestone',`
		`+ tracker_type: str) -> 'Bug':`
		"""Create a `Bug` instance from the provided arguments

		:param buginfo: for a definition of this dict, see `BugSync.extract_information()`

blockerbugs/models/criterion.py

file modified

+4 -1

		`@@ -19,9 +19,11 @@`

		`"""Database model for release criteria"""`

		`- from blockerbugs import db, BaseModel`
		`import datetime`

		`+ from blockerbugs import db, BaseModel`
		`+ import blockerbugs.models.milestone as model_milestone`
		`+`

		`class Criterion(BaseModel):`
		`id = db.Column(db.Integer, primary_key=True)`
		`@@ -29,6 +31,7 @@`
		`criterion = db.Column(db.String(4096), unique=False)`
		`current = db.Column(db.Boolean)`
		`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)`

		`def __init__(self, criterion, milestone, number,`

blockerbugs/models/milestone.py

file modified

+15 -11

		`@@ -19,12 +19,11 @@`

		`"""ORM class for release milestone info stored in the database"""`

		`- from blockerbugs import db, BaseModel`
		`from typing import Any, Optional`

		`- # unused imports, but required by dynamic relationships:`
		`- from blockerbugs.models.criterion import Criterion`
		`- from blockerbugs.models.release import Release`
		`+ from blockerbugs import db, BaseModel, models`
		`+ from blockerbugs.models import bug, criterion, update`
		`+ from blockerbugs.models import release as model_release`


		`class Milestone(BaseModel):`
		`@@ -34,8 +33,8 @@`

		`id = db.Column(db.Integer, primary_key=True)`
		`release_id = db.Column(db.Integer, db.ForeignKey('release.id'))`
		`- release: Optional[Release] = db.relationship(`
		`- 'Release', backref=db.backref('milestones', lazy='dynamic'))`
		`+ release: 'Optional[model_release.Release]' = db.relationship(`
		`+ 'Release', back_populates='milestones')`
		`version = db.Column(db.String(80))`
		`"""E.g. 'beta' or 'final'"""`
		`name = db.Column(db.String(80), unique=True)`
		`@@ -51,14 +50,19 @@`
		`current = db.Column(db.Boolean)`
		`"""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."""`
		`- criteria: list[Criterion] = db.relationship('Criterion', backref='milestone', lazy='dynamic')`
		`+ bugs: list['bug.Bug'] = db.relationship('Bug', back_populates='milestone', lazy='dynamic')`
		`+ updates: list['update.Update'] = db.relationship('Update', secondary=models.update_milestones,`
		`+ back_populates='milestones')`
		`+ criteria: list['criterion.Criterion'] = db.relationship('Criterion', back_populates='milestone',`
		`+ lazy='dynamic')`
		`succeeds_id = db.Column(db.Integer, db.ForeignKey('milestone.id'), nullable=True)`
		`- succeeds = db.relationship(`
		`- 'Milestone', backref=db.backref('succeeded_by', remote_side=[id], uselist=False),`
		`- lazy='dynamic')`
		`+ succeeds: 'Milestone' = db.relationship('Milestone', back_populates='succeeded_by',`
		`+ lazy='dynamic')`
		`+ succeeded_by: 'Milestone' = db.relationship('Milestone', back_populates='succeeds',`
		`+ remote_side=[id], uselist=False)`

		`def __init__(self,`
		`- release: Optional[Release],`
		`+ release: Optional['model_release.Release'],`
		`version: Optional[str],`
		`blocker_tracker: Optional[int],`
		`fe_tracker: Optional[int],`

blockerbugs/models/release.py

file modified

+7 -1

		`@@ -19,10 +19,12 @@`

		`"""Database model for releases"""`

		`- from blockerbugs import db, BaseModel`
		`from datetime import datetime`
		`from typing import Optional`

		`+ from blockerbugs import db, BaseModel`
		`+ from blockerbugs.models import milestone, update`
		`+`

		`class Release(BaseModel):`
		`"""This represents an OS release, primarily determined by its number."""`
		`@@ -36,6 +38,10 @@`
		"""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)`
		`+ milestones: list['milestone.Milestone'] = db.relationship('Milestone', back_populates='release',`
		`+ lazy='dynamic')`
		`+ updates: list['update.Update'] = db.relationship('Update', back_populates='release',`
		`+ lazy='dynamic')`

		`def __init__(self, number: int, active: Optional[bool] = True,`
		`discussions_closed: Optional[bool] = False) -> None:`

blockerbugs/models/update.py

file modified

+16 -36

		`@@ -19,25 +19,9 @@`

		`"""Database model for Bodhi updates"""`

		`- from blockerbugs import db, BaseModel`
		`-`
		`- # imports required by dynamic relationships:`
		`- from blockerbugs.models.bug import Bug`
		`- from blockerbugs.models.release import Release # noqa: F401`
		`- from blockerbugs.models.milestone import Milestone # noqa: F401`
		`-`
		`-`
		`- update_fixes = db.Table(`
		`- 'update_fixes',`
		`- db.Column('bug_id', db.Integer, db.ForeignKey('bug.id')),`
		`- db.Column('update_id', db.Integer, db.ForeignKey('update.id'))`
		`- )`
		`-`
		`- update_milestones = db.Table(`
		`- 'update_milestones',`
		`- db.Column('milestone_id', db.Integer, db.ForeignKey('milestone.id')),`
		`- db.Column('update_id', db.Integer, db.ForeignKey('update.id'))`
		`- )`
		`+ from blockerbugs import db, BaseModel, models`
		`+ from blockerbugs.models import bug, milestone`
		`+ from blockerbugs.models import release as model_release`


		`class Update(BaseModel):`
		`@@ -53,16 +37,12 @@`
		`date_pushed_testing = db.Column(db.DateTime, nullable=True)`
		`date_pushed_stable = db.Column(db.DateTime, nullable=True)`
		`date_obsoleted = db.Column(db.DateTime, nullable=True)`
		`- bugs = db.relationship('Bug',`
		`- secondary=update_fixes,`
		`- backref=db.backref('updates',`
		`- lazy='dynamic',`
		`- order_by=(status.desc(), pending.desc())))`
		`+ bugs: list['bug.Bug'] = db.relationship(`
		`+ 'Bug', secondary=models.update_fixes, back_populates='updates')`
		`release_id = db.Column(db.Integer, db.ForeignKey('release.id'))`
		`- release = db.relationship('Release',`
		`- backref=db.backref('bugs', lazy='dynamic'))`
		`- milestones = db.relationship('Milestone', secondary=update_milestones,`
		`- backref='updates')`
		`+ release: 'model_release.Release' = db.relationship('Release', back_populates='updates')`
		`+ milestones: list['milestone.Milestone'] = db.relationship(`
		`+ 'Milestone', secondary=models.update_milestones, back_populates='updates')`

		`def __init__(self, title, url, karma, status, bugs, release,`
		`milestones, stable_karma=3, date_submitted=None):`
		`@@ -96,15 +76,15 @@`
		`if updateinfo['date_pushed_stable']:`
		`self.date_pushed_stable = updateinfo['date_pushed_stable']`
		`self.pending = updateinfo['pending']`
		`- current_bugkeys = [(bug.bugid, bug.milestone) for bug in self.bugs]`
		`+ current_bugkeys = [(currentbug.bugid, currentbug.milestone) for currentbug in self.bugs]`
		`for bugid in updateinfo['bugs']:`
		`- newbugs = Bug.query.filter_by(bugid=bugid).all()`
		`- for bug in newbugs:`
		`- if not (bug.bugid, bug.milestone) in current_bugkeys:`
		`- self.bugs.append(bug)`
		`- current_bugkeys.append((bug.bugid, bug.milestone))`
		`- if bug.milestone and bug.milestone not in self.milestones:`
		`- self.milestones.append(bug.milestone)`
		`+ newbugs = bug.Bug.query.filter_by(bugid=bugid).all()`
		`+ for newbug in newbugs:`
		`+ if not (newbug.bugid, newbug.milestone) in current_bugkeys:`
		`+ self.bugs.append(newbug)`
		`+ current_bugkeys.append((newbug.bugid, newbug.milestone))`
		`+ if newbug.milestone and newbug.milestone not in self.milestones:`
		`+ self.milestones.append(newbug.milestone)`

		`@classmethod`
		`def from_data(cls, updateinfo, release):`

kparal commented 3 years ago

The advantage of back_populates= is that it is defined on both sides of the
relationship. Therefore when you look at a class, it is clear which attributes
it has, no attributes are created silently by a completely different class/
module. You can also define additional parameters like lazy and order_by
on the correct side of the relationship, rather than have it reversed.

The downside of defining the relationship on both sides is that it's much easier
to stumble into a circular import. I had to rework the imports to only import
a module (instead of a particular class), and all typehints had to be defined
as strings. The association tables had to be moved into an independent module
(I decided to use __init__.py).

@jskladan I'd very appreciate if you could look at this patch and tell me if the change seems sane to you, or if there's some gotcha in changing backref to back_populates that I haven't thought of. The test suite passes and the app runs, so it seems to work exactly as before, but I'm really not sure whether there could be some other consequences. Thanks a lot.