From 0c6ff087a31d4712bf4c9a973903d6767bdf040c Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: Jul 09 2021 09:54:11 +0000 Subject: db models: replace backref= with back_populates= 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`). --- diff --git a/blockerbugs/models/__init__.py b/blockerbugs/models/__init__.py index ae3e400..91aa93a 100644 --- a/blockerbugs/models/__init__.py +++ b/blockerbugs/models/__init__.py @@ -23,6 +23,20 @@ from sqlalchemy.types import SchemaType, TypeDecorator, Enum # type: ignore[att 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.""" diff --git a/blockerbugs/models/bug.py b/blockerbugs/models/bug.py index 583e0aa..7e9b86a 100644 --- a/blockerbugs/models/bug.py +++ b/blockerbugs/models/bug.py @@ -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 @@ class Bug(BaseModel): 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 @@ class Bug(BaseModel): ```""" _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 @@ class Bug(BaseModel): 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 @@ class Bug(BaseModel): def __repr__(self): return '' % (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 @@ class Bug(BaseModel): '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()` diff --git a/blockerbugs/models/criterion.py b/blockerbugs/models/criterion.py index 6f4a0ca..79a806f 100644 --- a/blockerbugs/models/criterion.py +++ b/blockerbugs/models/criterion.py @@ -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 @@ class Criterion(BaseModel): 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, diff --git a/blockerbugs/models/milestone.py b/blockerbugs/models/milestone.py index 0db60c2..318dc5e 100644 --- a/blockerbugs/models/milestone.py +++ b/blockerbugs/models/milestone.py @@ -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 @@ class Milestone(BaseModel): 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 @@ class Milestone(BaseModel): 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], diff --git a/blockerbugs/models/release.py b/blockerbugs/models/release.py index 8ae54ba..2050cf8 100644 --- a/blockerbugs/models/release.py +++ b/blockerbugs/models/release.py @@ -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 @@ class Release(BaseModel): """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: diff --git a/blockerbugs/models/update.py b/blockerbugs/models/update.py index 6e1d9b0..7d36885 100644 --- a/blockerbugs/models/update.py +++ b/blockerbugs/models/update.py @@ -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 @@ class Update(BaseModel): 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 @@ class Update(BaseModel): 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):