Skip to content

fix: various alembic migration issues with queries #2773

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

Conversation

michael-genson
Copy link
Collaborator

What type of PR is this?

(REQUIRED)

  • bug

What this PR does / why we need it:

(REQUIRED)

Building off of #2760, another issue came up where committing the transaction was executing the faulty SELECT statement. This PR should prevent this by disabling auto refreshes after commits for this session.

Which issue(s) this PR fixes:

(REQUIRED)

Mentioned in #2769

Testing

(fill-in or delete this section)

Pulled a backup from an older version with dupes and confirmed it migrates correctly now.

@zierbeek
Copy link
Contributor

could you maybe give my backup a test?

@michael-genson
Copy link
Collaborator Author

Sure, do you have a link?

@zierbeek
Copy link
Contributor

@michael-genson
Copy link
Collaborator Author

michael-genson commented Nov 29, 2023

Might need to re-upload, GitHub doesn't seem to like it. It also might be too big for GH:

image

@zierbeek
Copy link
Contributor

@michael-genson michael-genson marked this pull request as draft November 29, 2023 22:52
@michael-genson michael-genson changed the title fix: SQLAlchemy auto-refreshes models during migration fix: various alembic migration issues with queries Nov 30, 2023
@michael-genson michael-genson marked this pull request as ready for review November 30, 2023 02:48
@michael-genson
Copy link
Collaborator Author

Whelp, your backup broke several things. They seem fixed now, though to be honest I'm not quite sure why. I built the SQL manually and ran some tests locally to make sure data was still intact. I'm going to send over the migrated db on Discord, if you can check to make sure it works that'd be great.

@michael-genson
Copy link
Collaborator Author

I had two different sqlite formats in my test data - one had UUIDs with dashes, one didn't. I resolved the ambiguity by changing the select statement to just give me the raw DB value, rather than trying to coerce things. Both of my test dbs were successful with this change

Copy link
Collaborator

@hay-kot hay-kot left a comment

Choose a reason for hiding this comment

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

LGTM, will leave it up to you to merge 🚀

@michael-genson michael-genson enabled auto-merge (squash) December 6, 2023 18:32
@michael-genson michael-genson merged commit 310069a into mealie-recipes:mealie-next Dec 6, 2023
@michael-genson michael-genson deleted the fix/more-lazy-loading-issues branch December 12, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants