sqlmodel icon indicating copy to clipboard operation
sqlmodel copied to clipboard

🐛 Fix SQLAlchemy version 1.4.36 breaks SQLModel relationships (#315)

Open byrman opened this issue 2 years ago • 9 comments

byrman avatar May 01 '22 08:05 byrman

Can we add unit tests ?

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

byrman avatar May 02 '22 09:05 byrman

I guess just a test for relationships would work.

antont avatar May 02 '22 11:05 antont

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.

byrman avatar May 02 '22 13:05 byrman

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.

antont avatar May 02 '22 13:05 antont

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

mkarbo avatar Jun 04 '22 19:06 mkarbo

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

lonelyteapot avatar Jun 22 '22 20:06 lonelyteapot

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.

bharathanr avatar Jul 06 '22 07:07 bharathanr

@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.

berendt avatar Jul 22 '22 20:07 berendt

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.

leynier avatar Jul 31 '22 08:07 leynier

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%) :arrow_up:

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

codecov[bot] avatar Aug 27 '22 18:08 codecov[bot]

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

github-actions[bot] avatar Aug 27 '22 18:08 github-actions[bot]

Awesome, thank you @byrman! :bow: :rocket:

And thanks everyone for the discussion.

This will be available in the next version, in the next hours. :tada:

tiangolo avatar Aug 27 '22 18:08 tiangolo