Repository navigation
feat(core): add direct library ownership to media - #1462
aaronleopold merged 2 commits into
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
aaronleopold
left a comment
aaronleopold
left a comment
There was a problem hiding this comment.
lgtm, i had a few comments but i think anything of those worth sorting out can be sorted separately. thank you! i'll aim to merge later this evening / tomorrow am
There was a problem hiding this comment.
Did you test this works on SQLite? To my knowledge, you cannot alter a table and add fks (see here, section 5)
There was a problem hiding this comment.
Okay, leaving for posterity (and to showcase my wrong-ness, or at the very least imprecision in my initial understanding): it only works because you've allowed it to default to NULL, right? And you've sorta worked around the previously-linked constraint of:
It is not possible to use the "ALTER TABLE ... ADD COLUMN" syntax to add a column that includes a REFERENCES clause, unless the default value of the new column is NULL. Attempting to do so returns an error.
By doing that dance after where you add constraints/triggers (postgres, sqlite, respectively) after the fact. I think this is largely fine, the one part I'm a little concerned about is the disconnect it causes between postgres and sqlite. The alternative would be that table redefinition, which while it would give a real composite fk does carry a bit more weight to it wrt potential problems. Open to discussion if you have thoughts, this is going to a feature branch so won't block my approval on this
There was a problem hiding this comment.
Yep, tested on populated SQLite and Postgres databases, including rollback. The NULL default is what makes it work here.
| .query_one(Statement::from_string( | ||
| backend, | ||
| "SELECT media.id FROM media | ||
| LEFT JOIN series ON series.id = media.series_id | ||
| LEFT JOIN libraries ON libraries.id = series.library_id | ||
| WHERE libraries.id IS NULL LIMIT 1", | ||
| )) |
There was a problem hiding this comment.
While I don't necessarily find this scenario likely anyways, I imagine if orphan's exist we'd want to catch them all and not just the first one
There was a problem hiding this comment.
Agreed on reporting all the orphans, I’ll include that in PR 2. I also found a difference when changing a library ID: Postgres cascades it, while SQLite’s triggers reject it. Normal edits don’t change IDs, but I’ll revisit constraint parity alongside the virtual-series migrations in PR 3.
There was a problem hiding this comment.
Ended up addressing both in a small follow-up commit here: it now reports all orphan IDs and allows library ID changes to cascade on SQLite too. Tested on both databases, including rollback. SQLite still uses triggers.
2ff898f
into
stumpapp:metadata-based-libraries
Add direct library ownership to books
Adds direct library ownership to books and backfills existing ownership from their series. SQLite and PostgreSQL enforce valid ownership and matching library/series relationships. New books receive ownership during scanning.
This is the foundation for metadata-only libraries. Query refactors and the remaining feature work will follow in separate PRs.
AI assistance was used for implementation and review.
Refs #288
Ready?
Please read each item and check the boxes:
nightlyand notmainStump Contributor License Agreement
By contributing to Stump, you agree that your contributions will be licensed under the following licenses (where applicable):