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

fix(database-converter): use string concat instead of var substitution #1483

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

vincejv
Copy link
Contributor

@vincejv vincejv commented Mar 27, 2024

We don't need to sanitize the input as it doesn't come from the user and this is simply not allowed in any sql database: https://stackoverflow.com/questions/20678725/how-to-set-table-name-in-dynamic-sql-query

Will test on my local to see if there are other migration issues

We don't need to sanitize the input as it doesn't come from the user and this is simply not allowed in any sql database: https://stackoverflow.com/questions/20678725/how-to-set-table-name-in-dynamic-sql-query
@KyleSanderson
Copy link
Collaborator

Is there some background on this one? Does this not work right now?

@vincejv
Copy link
Contributor Author

vincejv commented Mar 27, 2024

Yes it doesn't run with the current code will just result in an error due to thing mentioned in the post, SQL Table names cannot have var substitution, query must be concatenated manually

autobrrctl db:convert --sqlite-db autobrr_final.db --postgres-url \
      postgres://autobrr:autobrr@postgres.docker.internal:5432/autobrrtest
2024/03/28 05:43:25 Failed to query SQLite table 'action': SQL logic error: near "?": syntax error (1)

@vincejv
Copy link
Contributor Author

vincejv commented Mar 27, 2024

There are other migration issues actually but not sure how to solve them, I've fixed it manually by running sql scripts

  1. Run Auto-increment index reset for all tables with AI PK, or else new inserts would fail due to id already existing.
  2. Failed migration of actions, indexer tables and etc due to FK violations, workaround is to run the command twice. Possible fix is to migrate tables in order.
  3. Create tables first before migration OR improve documentation to run autobrr on Postgres mode to seed the database before starting the migration as migration will fail with no tables in the database

@zze0s
Copy link
Collaborator

zze0s commented Mar 27, 2024

It's indeed not super straight-forward to switch database engines 😄

The migrate commands had some issues at time of merge, and I did not give it the priority it should have gotten.

If you make further improvements then keep it all in this PR so it's easier to manage.

@zze0s zze0s added database Database related autobrrctl labels Apr 19, 2024
@zze0s zze0s changed the title fix(autobrrctl): use string concat instead of var substitution fix(database-converter): use string concat instead of var substitution Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autobrrctl database Database related
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants