Bunch of SQLAlchemy definitions #1192
Conversation
maximemulder
added
the
A-ORM
maximemulder
changed the title
maximemulder
force-pushed
the
2024-10-09_bunch-mysqlalchemy-defs
branch
4 times, most recently
from
c3add28
to
5d550fa
Compare
| mri_qc_first_change_time : Mapped[Optional[datetime]] = mapped_column('MRIQCFirstChange') | ||
| mri_qc_last_change_time : Mapped[Optional[datetime]] = mapped_column('MRIQCLastChange') | ||
| mri_caveat : Mapped[str] = mapped_column('MRICaveat') | ||
| language_id : Mapped[Optional[int]] = mapped_column('languageID') | ||
|
|
||
| candidate : Mapped['db_candidate.DbCandidate'] = relationship('DbCandidate', back_populates='sessions') |
There was a problem hiding this comment.
I think it is missing the CohortID foreign key and relationship here.
There was a problem hiding this comment.
It is also missing the ProjectID table definition and the relationship with session and candidate
| @@ -15,10 +19,10 @@ class DbCandidate(Base): | |||
| dete_of_death : Mapped[Optional[date]] = mapped_column('DoD') | |||
| edc : Mapped[Optional[date]] = mapped_column('EDC') | |||
| sex : Mapped[Optional[str]] = mapped_column('Sex') | |||
| registration_site_id : Mapped[int] = mapped_column('RegistrationCenterID') | |||
| registration_site_id : Mapped[int] = mapped_column('RegistrationCenterID', ForeignKey('psc.CenterID')) | |||
There was a problem hiding this comment.
missing FK for ProjectID and Project table definition
| return db.execute(select(DbConfig) | ||
| .join(DbConfig.setting) | ||
| .where(DbConfigSetting.name == name) | ||
| ).scalar_one() |
There was a problem hiding this comment.
I think it is possible that a config is not set in which case this would return None.
There was a problem hiding this comment.
I guess that if the config is not existent it will raise an exception but do we catch those? What is the error message when that happens?
There was a problem hiding this comment.
For now it raises an exception, but in the future I was planning to have a python/lib/config.py module to make the queries and handle the exceptions by exiting the program (unless the setting can be None, in which case it should probably return None).
There was a problem hiding this comment.
I think some Config could be not set (like the S3 configs) so probably best to support those cases.
| def get_notification_type_with_name(db: Database, name: str): | ||
| """ | ||
| Get a notification type from the database using its configuration setting name, or raise an | ||
| exception if no notification type is found. |
There was a problem hiding this comment.
do we want to raise an exception if notification type is not existant or do we want to create the notification type?
There was a problem hiding this comment.
The current code does not create the notification type, but I thought about it and I agree creating it would probably be the better option.
There was a problem hiding this comment.
yes, that is what I assumed. I think for now it is fine but we could probably add that in the future. The same way we create the parameter_type when it does not exist. I don't think this is high priority though.
python/lib/db/query/site.py
Outdated
|
|
||
| def try_get_site_with_psc_id_visit_label(db: Database, psc_id: str, visit_label: str): | ||
| """ | ||
| Get a session from the database using its candidate CandID and visit label, or return `None` |
There was a problem hiding this comment.
the doc says it uses the candID but the query uses the PSCID.
There was a problem hiding this comment.
Whoops, that is indeed an oversight on my part, I'll be fixing that !
|
@cmadjar With regards to the missing foreign keys, I did not add foreign keys to table that do not have an ORM definition yet, and was planning to do it later. But I can add the other ORM tables in this PR if you want to. |
|
@maximemulder I thought about the Project table because it can be useful when handling sessions potentially. I realize that the other imaging tables are not there yet but it can be done in a separate PR. |
maximemulder
added
the
S-Medium
maximemulder
force-pushed
the
2024-10-09_bunch-mysqlalchemy-defs
branch
from
5d550fa
to
cc63fe6
Compare
maximemulder
force-pushed
the
2024-10-09_bunch-mysqlalchemy-defs
branch
from
343c54d
to
d604b24
Compare
This is bunch of SQLAlchemy definitions I extracted from my various PRs in order to make them easier to review.
These definitions do not interact with the current code without the related PRs. Nevertheless, the (few) unit tests work, and the manual tests I did on each PR worked as well. I don't think there is any testing to do on this PR as it will be done when we review the related PRs instead, as well as with the integration testing when possible.