🐛 Fix auto detecting and setting nullable, allowing overrides in field
#423
Conversation
|
📝 Docs preview for commit 7f853e6 at: https://630c8adba75f7a0e2321cc8c--sqlmodel.netlify.app |
Codecov Report
@@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 97.72% 97.62% -0.11%
==========================================
Files 186 187 +1
Lines 6201 6268 +67
==========================================
+ Hits 6060 6119 +59
- Misses 141 149 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
📝 Docs preview for commit df83c46 at: https://630c975d319afc17b0609c69--sqlmodel.netlify.app |
|
📝 Docs preview for commit d1fd2e3 at: https://630c99a7801d6f14b58c951a--sqlmodel.netlify.app |
`Field` are inserting into the database as nullable. This was introduced in fastapi@9830ee0#r82434170 and described in fastapi#420. Example: ``` required_field: Optional[str] = Field(nullable=False) ``` - Added a test to confirm the regression - Fixed test by re-ordering the code that determines if a field is nullable. Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com>, building off his PR: fastapi#423 add coverage reports run code coverage Add more checks to the test for the regression. Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com> Fix comment on test Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com> Formatting, comments Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com> Remove newline Co-authored-by: Jonas Krüger Svensson jonas-ks@hotmail.com remove comment simplify test rename variable add missing assert
setting an `Optional` type but passing `nullable=True` to the `Field` definition - Fix the bug
|
📝 Docs preview for commit 4ca8a40 at: https://630cd3ae95131e4649ab9d4b--sqlmodel.netlify.app |
|
📝 Docs preview for commit 0253e58 at: https://630cd4c4b736cd51455110cf--sqlmodel.netlify.app |
|
There are other cases not covered here. For example, what would you expect the nullability of this field to be: |
|
Thanks @JonasKs and @br-allstreet 🙇 🚀 I added a couple of test cases, including default strings as you suggested @br-allstreet. Then I tweaked the implementation a bit more as well to properly handle those cases. This will be available in the next release in the next hours, SQLModel |
|
📝 Docs preview for commit fc62e38 at: https://630e3835f8bb537d8b6f9a9c--sqlmodel.netlify.app |
3 participants
See #420, potentially closes it
#424 copied my code, so contributions are a bit messed up if that is merged instead of this one. I don't mind too much though, so you can close this if you'd like.