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 SQLAlchemy version 1.4.36 breaks SQLModel relationships (#315) #322

Merged
merged 2 commits into from
Aug 27, 2022

Conversation

byrman
Copy link
Contributor

@byrman byrman commented May 1, 2022

No description provided.

Copy link

@jossefaz jossefaz left a comment

Choose a reason for hiding this comment

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

Can we add unit tests ?

@byrman
Copy link
Contributor Author

byrman commented May 2, 2022

Can we add unit tests ?

What would be a good test? I don't know how to write tests for different SA versions.

@antont
Copy link

antont commented May 2, 2022

I guess just a test for relationships would work.

@byrman
Copy link
Contributor Author

byrman commented May 2, 2022

I guess just a test for relationships would work.

OK, but there are plenty of those already. It would be nice if we could prove that it works for both 1.4.36 and older versions of SQLAlchemy.

@antont
Copy link

antont commented May 2, 2022

OK, but there are plenty of those already. It would be nice if we could prove that it works for both 1.4.36 and older versions of SQLAlchemy.

Right - apparently tests just have not been run in Github Actions in 16d so the breakage does not show.

I don't think a test in itself can define the lib version but that would require something more, maybe like a branch where a different SA version is pinned.

@mkarbo
Copy link

mkarbo commented Jun 4, 2022

Running tests without the fix results in broken tests, running with fix all tests pass. You can run them locally to verify

@lonelyteapot
Copy link

lonelyteapot commented Jun 22, 2022

Any updates on when this will be merged? The issue is critical

@bharathanr
Copy link

bharathanr commented Jul 6, 2022

Hi! Is there anything we can do to help with merging this pull request? It's rather critical, so if there's something that can be done, it would be great. I'm willing to put in some time to help.

@berendt
Copy link

berendt commented Jul 22, 2022

@tiangolo Can you take a look at this, please? It is nasty that you have to pin the version of sqlalchemy at the moment. It would be nice if you could make a release after the merge so that the current version of sqlalchemy can be used again.

@leynier
Copy link
Contributor

leynier commented Jul 31, 2022

In this PR (https://github.com/leynier/sqlmodel/pull/37) pointing to a fork, you can see that the tests are successful. I think that It is not necessary to add extra tests for this PR because the existing tests validate the change since without the change of this PR the tests fail.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

#322

#315
fix

@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #322 (a55ae6d) into main (4d20051) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #322   +/-   ##
=======================================
  Coverage   97.49%   97.49%           
=======================================
  Files         181      181           
  Lines        6037     6038    +1     
=======================================
+ Hits         5886     5887    +1     
  Misses        151      151           
Impacted Files Coverage Δ
sqlmodel/main.py 81.79% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

📝 Docs preview for commit a55ae6d at: https://630a5d484e7d9624a7f35029--sqlmodel.netlify.app

@tiangolo
Copy link
Member

Awesome, thank you @byrman! 🙇 🚀

And thanks everyone for the discussion.

This will be available in the next version, in the next hours. 🎉

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.