Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure xklb-metadata.db has live_status and error columns #265

Closed
wants to merge 19 commits into from

Conversation

deldesir
Copy link
Collaborator

@deldesir deldesir commented Oct 10, 2024

This pull request is related to the creation of xklb-metadata.db . Two key columns: live_status and error, are needed to avoid missing columns error. XKLB does not create them upfront, so we take care of this by adding them.

There additions are in fact a patch in xklb's tube_add.py to ensure we don't have to make Calibre-Web create xklb-metadata.db from scratch using the schema in xb.py. It also ensures we have key columns ready while leveragin xklb's handling of the database.

  • Tested on Ubuntu 24.04

@deldesir deldesir marked this pull request as draft October 10, 2024 03:42
@deldesir deldesir mentioned this pull request Oct 10, 2024
2 tasks
@holta
Copy link
Member

holta commented Oct 10, 2024

@holta holta added the enhancement New feature or request label Oct 10, 2024
deldesir added a commit to deldesir/calibre-web that referenced this pull request Oct 10, 2024
scripts/xklb-patch Outdated Show resolved Hide resolved
@holta
Copy link
Member

holta commented Oct 10, 2024

  1. Get this POC (proof of concept) working ASAP, before anything else!
  2. Possibly optimize later ("Premature Optimization is the Root Of All Evil") by...
    • Eliminating all flags — instead just using the missing code itself — as a de facto flag/indicator that patching is needed?
    • Using Git to modify code programmatically — if absolutely necessary?
    • Better yet, we really should probably/soon work with Jacob Chapman to mechanize this — or upstream this schema enforcement/validation — with an even more future-proof approach! 🙏

scripts/xklb-patch Outdated Show resolved Hide resolved
scripts/xklb-patch Outdated Show resolved Hide resolved
scripts/xklb-patch Outdated Show resolved Hide resolved
@avni
Copy link
Member

avni commented Oct 11, 2024

@deldesir indicates that this PR will need to be changed and use iiab-glue.db because of the DB refactoring that will map books and media.

@holta
Copy link
Member

holta commented Oct 11, 2024

@deldesir indicates that this PR will need to be changed and use iiab-glue.db because of the DB refactoring that will map books and media.

@deldesir please make the decision as-final-as-it-can-be today. The description on top of this PR is empty, which is definitely not helpful. So I'm pasting in your private comments from October 8 — as this needs to accelerate:

PR #191 from June 18 is not enough — here are the reasons according to @deldesir on October 8:

  1. To rely on xklb for creating the db instead of doing it ourselves. This make all the triggers, indexes and FTS mechanisms stay intact
  2. To support CI/CD
  3. To cover more use cases like galleries, filesystems handling, videos playback tracking…
  4. Above is for patching xklb in general. For PR Ensure error and live_status columns are created from the start #191, I would add:
    • Avoid altering the schema ourselves via Calibre-Web because it's xklb's job.”

@holta
Copy link
Member

holta commented Oct 11, 2024

@deldesir please make the decision as-final-as-it-can-be today.

In other words:

  • @deldesir please make this PR NON-DRAFT today, with a more complete explanation of its exact purpose ✅
  • Or close this PR today (with a precise explanation as to what will replace it, Thank You!)

@avni
Copy link
Member

avni commented Oct 12, 2024

@deldesir indicates that this PR will need to be changed and use iiab-glue.db because of the DB refactoring that will map books and media.

@deldesir I may have misunderstood what you were saying Friday morning? From looking at the code changes, I think the check for the columns/additions is independent of book/media mapping.

@deldesir
Copy link
Collaborator Author

@avni I meant PR #255, sorry for the misunderstanding.

@avni
Copy link
Member

avni commented Oct 14, 2024

Makes much more sense! Thank you!

@deldesir deldesir self-assigned this Oct 14, 2024
@deldesir deldesir marked this pull request as ready for review October 14, 2024 21:02
Copy link
Collaborator Author

@deldesir deldesir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patching tube_add did not work. No matter what, error column would not be created because it is being removed from entry when its value is None due to a filtering process.
As a result, the insert method doesn't see the error key and doesn't alter the table schema to include it.

With the latest commit, the add function in db_media.py is modified to ensure the error key is included in entry even when its value is None.

entry = {k: v for k, v in entry.items() if v is not None or k == 'error'}
    if entry is None:
        return

@holta
Copy link
Member

holta commented Oct 15, 2024

error column would not be created

  • Can this PR Ensure xklb-metadata.db has live_status and error columns #265's title (subject line) be clarified a bit, explaining the new approach?

  • Adjust the Description on top of this PR, if it needs revising?

  • Are live_status and error columns still specifically being added by this PR's helper logic? (How does that work, now that this this PR's code has removed the prior tube_add.py mentions of live_status and error ?)

@holta
Copy link
Member

holta commented Oct 15, 2024

@deldesir Does this find usage example help?

root@box:~# find /root/.local/ -type f -path '*/site-packages/xklb/mediadb/db_media.py'
/root/.local/share/pipx/venvs/xklb/lib/python3.12/site-packages/xklb/mediadb/db_media.py

scripts/xklb-patch Outdated Show resolved Hide resolved
@deldesir deldesir marked this pull request as draft October 16, 2024 11:11
@deldesir
Copy link
Collaborator Author

deldesir commented Oct 16, 2024

This PR and its companion iiab/iiab#3827 are on the track to be abandoned. 2 days already, still not working as expected due to the complexity of xklb's db operations. PR #259 is adjusted to achieve the same goal.

@holta
Copy link
Member

holta commented Oct 16, 2024

ONGOING DEBATE: Are the 4 lines below in PR #259's SQLalchemy approach (in cps/xb.py) potentially a complete/safe alternative to this PR #265 ?

        if not os.path.exists(XKLB_DB_FILE):
            print(f"Database file not found at {XKLB_DB_FILE}, creating a new blank database.")
            Base.metadata.create_all(self.engine)
            print("New blank database created.")

https://github.com/deldesir/calibre-web/blob/2a5bf65ac27df905c9facb2f459cfbd0420c53cb/cps/xb.py#L145-L148

@deldesir deldesir closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants