Skip to content

Fix nullable field issue (#420) #424

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

Fix nullable field issue (#420) #424

wants to merge 14 commits into from

Conversation

br-follow
Copy link
Contributor

@br-follow br-follow commented Aug 29, 2022 •
edited
Loading

UPDATE

changes added to #423

Motivation

Columns that are defined as Optional but are set = Field(nullable=False) are made nullable. This is a bug that was introduced in 9830ee0#r82434170 and described in #420.

Actions Taken

  • 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: #423

@codecov
Copy link

codecov bot commented Aug 29, 2022 •
edited
Loading

Codecov Report

Merging #424 (f0c5fec) into main (85f5e7f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
+ Coverage   97.72%   97.75%   +0.02%     
==========================================
  Files         186      187       +1     
  Lines        6201     6238      +37     
==========================================
+ Hits         6060     6098      +38     
+ Misses        141      140       -1
Impacted Files Coverage Δ
sqlmodel/main.py 84.97% <100.00%> (+0.28%) ⬆️
tests/test_nullable.py 100.00% <100.00%> (ø)

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
Contributor

📝 Docs preview for commit 28eb8f6 at: https://630ca4a1319afc233d609be0--sqlmodel.netlify.app

@br-follow br-follow mentioned this pull request Aug 29, 2022
Closed
8 tasks
@br-follow @JonasKs
Add more checks to the test for the regression.
00cebd2 
Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com>
@github-actions
Copy link
Contributor

📝 Docs preview for commit 00cebd2 at: https://630cb1b0a75f7a2ead21ca8c--sqlmodel.netlify.app

br-follow and others added 2 commits August 29, 2022 08:33
@br-follow @JonasKs
Fix comment on test
63cac24 
Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com>
@br-follow
Formatting, comments
d487f2f 
Co-authored-by: Jonas Krüger Svensson <jonas-ks@hotmail.com>
@github-actions
Copy link
Contributor

📝 Docs preview for commit 63cac24 at: https://630cb248801d6f2b4b8c8823--sqlmodel.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit d487f2f at: https://630cb2a5f80c1f2a11140156--sqlmodel.netlify.app

@br-follow
Remove newline
bb18a08 
Co-authored-by: Jonas Krüger Svensson jonas-ks@hotmail.com
@github-actions
Copy link
Contributor

📝 Docs preview for commit bb18a08 at: https://630cb33de769772b402c1326--sqlmodel.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 7d51ac0 at: https://630cb917b736cd3620509fad--sqlmodel.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit f55e545 at: https://630cba6cfe01ec31bd7acdbc--sqlmodel.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2e962b5 at: https://630cbad4068f4d37448f2b0e--sqlmodel.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 7c428bd at: https://630cbf3194f028332e0aa4b1--sqlmodel.netlify.app

@JonasKs JonasKs mentioned this pull request Aug 29, 2022
Merged
@br-follow
Columns that are defined with an Optional type but a non nullable
e22617e 
`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
@br-follow
Merge branch 'fix-nullable-field-issue' of https://github.com/br-alls…
f0c5fec 
…treet/sqlmodel into fix-nullable-field-issue
@github-actions
Copy link
Contributor

📝 Docs preview for commit f0c5fec at: https://630ccfaf1b8fa546d5654ee5--sqlmodel.netlify.app

@br-follow
Copy link
Contributor Author

I have added my changes to #423

@br-follow br-follow closed this Aug 29, 2022
@tiangolo
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@br-follow @tiangolo