arbeitsgruppe-digitale-altnordistik / Sammlung-Toole

A new look on Handrit.is data
https://arbeitsgruppe-digitale-altnordistik.github.io/Sammlung-Toole/
MIT License
0 stars 0 forks source link

refactor: change SQLite implementation to use SQLModel #131

Closed BalduinLandolt closed 1 year ago

BalduinLandolt commented 1 year ago

resolves #119

BalduinLandolt commented 1 year ago

@kraus-s SQLModel is much more of a pain than I anticipated (I just don't like the whole relational database world and all of its ecosystem ^^).
One issue I'm currently encountering is that it's a pain to try to have multiple, separate databases. (Currently we're separating groups and handrit data.) Do you think it would be acceptable to have them in the same database? This would mean that it's hard to wipe the groups alone (though perfectly doable), and that re-creating the DB would automatically wipe the groups.
I'm a bit torn between not liking the loss of control, and liking the additional simplicity of only one DB and only one set of database operations. Also I think there might be rare occurrences where not wiping the groups when recreating the database, would lead to broken IDs, so these two workflows might have to be coupled anyways.
What do you think?

kraus-s commented 1 year ago

@kraus-s SQLModel is much more of a pain than I anticipated (I just don't like the whole relational database world and all of its ecosystem ^^). One issue I'm currently encountering is that it's a pain to try to have multiple, separate databases. (Currently we're separating groups and handrit data.) Do you think it would be acceptable to have them in the same database? This would mean that it's hard to wipe the groups alone (though perfectly doable), and that re-creating the DB would automatically wipe the groups. I'm a bit torn between not liking the loss of control, and liking the additional simplicity of only one DB and only one set of database operations. Also I think there might be rare occurrences where not wiping the groups when recreating the database, would lead to broken IDs, so these two workflows might have to be coupled anyways. What do you think?

I am actually very much in favor of having only one DB to work with. Clearing the groups should not be to hard either, I suppose? We just dump * the corresponding tables; Idea for db layout (table_name(column_names)):

Groups(GroupID PRIMARY KEY, GroupName STRING, GroupDesc OPTIONAL) GroupxMSsJunction: (localID PRIMARY KEY, GroupID FOREIGN KEY, MsID FOREIGN KEY) GroupxPPlJunction: same as above basically but with people

-> set both junction tables to cascade on delete -> then we can just dump the Groups table and it should cascade-delete all entries in the junciton tables and not (I really hope!) delete any of the MS or PPL data.

BalduinLandolt commented 1 year ago

@kraus-s SQLModel is much more of a pain than I anticipated (I just don't like the whole relational database world and all of its ecosystem ^^). One issue I'm currently encountering is that it's a pain to try to have multiple, separate databases. (Currently we're separating groups and handrit data.) Do you think it would be acceptable to have them in the same database? This would mean that it's hard to wipe the groups alone (though perfectly doable), and that re-creating the DB would automatically wipe the groups. I'm a bit torn between not liking the loss of control, and liking the additional simplicity of only one DB and only one set of database operations. Also I think there might be rare occurrences where not wiping the groups when recreating the database, would lead to broken IDs, so these two workflows might have to be coupled anyways. What do you think?

I am actually very much in favor of having only one DB to work with. Clearing the groups should not be to hard either, I suppose? We just dump * the corresponding tables; Idea for db layout (table_name(column_names)):

Ok, cool! I will proceed like this then.

Groups(GroupID PRIMARY KEY, GroupName STRING, GroupDesc OPTIONAL) GroupxMSsJunction: (localID PRIMARY KEY, GroupID FOREIGN KEY, MsID FOREIGN KEY) GroupxPPlJunction: same as above basically but with people

-> set both junction tables to cascade on delete
-> then we can just dump the Groups table and it should cascade-delete all entries in the junciton tables and not (I really hope!) delete any of the MS or PPL data.

Sounds like a plan, and definitely better than what we currently have in our groups DB. But I'd prefer to make this a separate task. This one will already be a big enough PR to review and test...

codecov-commenter commented 1 year ago

Codecov Report

Base: 46.52% // Head: 51.97% // Increases project coverage by +5.45% :tada:

Coverage data is based on head (d7b06b7) compared to base (6bd334f). Patch coverage: 66.57% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #131 +/- ## ========================================== + Coverage 46.52% 51.97% +5.45% ========================================== Files 26 24 -2 Lines 1595 1622 +27 ========================================== + Hits 742 843 +101 + Misses 853 779 -74 ``` | [Impacted Files](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik) | Coverage Δ | | |---|---|---| | [src/lib/metadatahandler.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9tZXRhZGF0YWhhbmRsZXIucHk=) | `29.78% <0.00%> (ø)` | | | [tests/unit/lib/xml/test\_tamer.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-dGVzdHMvdW5pdC9saWIveG1sL3Rlc3RfdGFtZXIucHk=) | `95.00% <ø> (ø)` | | | [src/lib/utils.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi91dGlscy5weQ==) | `46.19% <20.00%> (-0.04%)` | :arrow_down: | | [src/lib/database/deduplicate.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhYmFzZS9kZWR1cGxpY2F0ZS5weQ==) | `18.18% <22.22%> (-22.17%)` | :arrow_down: | | [src/lib/datahandler.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhaGFuZGxlci5weQ==) | `36.27% <25.45%> (+13.54%)` | :arrow_up: | | [src/lib/database/db\_init.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhYmFzZS9kYl9pbml0LnB5) | `30.00% <27.08%> (+7.64%)` | :arrow_up: | | [src/lib/xml/tamer.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi94bWwvdGFtZXIucHk=) | `22.53% <43.58%> (+3.20%)` | :arrow_up: | | [src/lib/database/sqlite/database\_sqlite\_impl.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhYmFzZS9zcWxpdGUvZGF0YWJhc2Vfc3FsaXRlX2ltcGwucHk=) | `47.36% <47.36%> (ø)` | | | [src/lib/database/database.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhYmFzZS9kYXRhYmFzZS5weQ==) | `59.45% <63.63%> (+24.79%)` | :arrow_up: | | [src/lib/database/sqlite/models.py](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik#diff-c3JjL2xpYi9kYXRhYmFzZS9zcWxpdGUvbW9kZWxzLnB5) | `92.03% <92.03%> (ø)` | | | ... and [9 more](https://codecov.io/gh/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/131?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arbeitsgruppe-digitale-altnordistik)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

kraus-s commented 1 year ago

Feel free to assign me to designing the group db stuff! Perhaps I can do it right after our SQL session with Tarrin tomorrow

BalduinLandolt commented 1 year ago

Feel free to assign me to designing the group db stuff! Perhaps I can do it right after our SQL session with Tarrin tomorrow

It will have to wait for this task to be finished, otherwise we'll have merge conflicts from hell... I pretty much left no database-stone unturned. :speak_no_evil:
(Also, you'll have to learn SQLModel)

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

BalduinLandolt commented 1 year ago

@kraus-s this has become a horror PR to review... sorry about that. If you prefer, I can walk you through it, and in the meanwhile also explain the whole SQLModel stuff... Just let me know what you prefer.

I will also open a bunch of issues, they mostly assume that this PR is already merged.

kraus-s commented 1 year ago

@BalduinLandolt I'll have a look later today or tomorrow and then we'll see if I need a guide XD

BalduinLandolt commented 1 year ago

@BalduinLandolt I'll have a look later today or tomorrow and then we'll see if I need a guide XD

No rush! I need a break from this anyways ;-)

BalduinLandolt commented 1 year ago

Ok, I have made it all the way through and it seems you have basically rewritten most of the backend, which is pretty impressive. I'm not sure I understand all of it in detail, but I'm confident I got the gist of it :D I'll understand it in more depth once I get on my issues and work with the code. I would be fine with merging this, unless you, @BalduinLandolt , feel that I should have a better understanding of what I'm signing off on here.

Well... maybe not rewritten the entire backend... but yea, it got a bit out of hand, I hoped I would get away with less changes (and less hours invested), but one thing required the next, etc.

IMO it's ok to not understand every detail at this point in time. I'm somewhat confident that it behaves mostly the same as it did before, so merging it shouldn't break too much. And as you say, you'll get it once you work on it. (But if there proves to be something you don't understand, don't hesitate to ask. And note also, that it's not yet done perfectly [see the issues I created], so maybe if something seems stupid, you don't need to seek for sense in it, it might just be as stupid as it looks.)

Long story short: I'll merge it