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

Field(nullable=True) is ignored due to recent changes released in version 0.0.7 #420

Closed
8 tasks done
br-follow opened this issue Aug 28, 2022 · 11 comments
Closed
8 tasks done
Labels
answered bug Something isn't working

Comments

@br-follow
Copy link
Contributor

br-follow commented Aug 28, 2022

First Check

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the SQLModel documentation, with the integrated search.
  • I already searched in Google "How to X in SQLModel" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to SQLModel but to Pydantic.
  • I already checked if it is not related to SQLModel but to SQLAlchemy.

Commit to Help

  • I commit to help with one of those options 👆

Example Code

from typing import Optional

from sqlmodel import Field, Session, SQLModel, create_engine

class Hero(SQLModel, table=True):
  id: Optional[int] = Field(default=None, primary_key=True)
  # This should not be nullable
  name: Optional[str] = Field(default=None, nullable=False)

hero = Hero()
engine = create_engine("sqlite:///database.db")

SQLModel.metadata.create_all(engine)
with Session(engine) as session:
  session.add(hero)
  session.commit()
  session.refresh(hero)
  print(hero)

Description

  • Create a hero model with a non-nullable column by assigning it to Field(nullable=True)
  • Create a hero instance without setting the column
  • Save the instance to the DB. Should get an error but the hero instance saves successfully.

I believe this issue was introduced here: 9830ee0#r82434170

Operating System

macOS

Operating System Details

No response

SQLModel Version

0.0.7

Python Version

Python 3.10.5

Additional Context

No response

@br-follow br-follow added the question Further information is requested label Aug 28, 2022
@JonasKs

This comment was marked as outdated.

@JonasKs
Copy link
Contributor

JonasKs commented Aug 29, 2022

I've submitted a new test that tests nullable fields, and it seems correct to me.
See PR #423.

@br-follow
Copy link
Contributor Author

I've submitted a new test that tests nullable fields, and it seems correct to me. See PR #423.

I believe I have confirmed and addressed the issue in #424.

@JonasKs
Copy link
Contributor

JonasKs commented Aug 29, 2022

Aha, I understand now. Seems like a weird pattern to set a default value, but not allow it.. To me it seems like you just shouldn't type it as optional.

As a side note, please either branch out from the PR / ask for branch permissions to add more commits / add the original author as a co-author when you copy/paste code from another PR 😊 You've copied everything I wrote, but I am not listed as a contributor. 😉

@br-follow
Copy link
Contributor Author

You might type is as optional so that you can create the object without setting all the fields.

Happy to do either. If you give me branch permissions, I can make my change to your existing branch.

Aha, I understand now. Seems like a weird pattern to set a default value, but not allow it.. To me it seems like you just shouldn't type it as optional.

As a side note, please either branch out from the PR / ask for branch permissions to add more commits / add the original author as a co-author when you copy/paste code from another PR 😊

@br-follow
Copy link
Contributor Author

br-follow commented Aug 29, 2022

You might type is as optional so that you can create the object without setting all the fields.

Happy to do either. If you give me branch permissions, I can make my change to your existing branch.

Aha, I understand now. Seems like a weird pattern to set a default value, but not allow it.. To me it seems like you just shouldn't type it as optional.
As a side note, please either branch out from the PR / ask for branch permissions to add more commits / add the original author as a co-author when you copy/paste code from another PR 😊

Added you as a co-author: 00cebd2

@JonasKs
Copy link
Contributor

JonasKs commented Aug 29, 2022

Gave you access to mine, but up to you. I assume you have to rebase those commits into one commit to get it merged anyway 😄

@JonasKs
Copy link
Contributor

JonasKs commented Aug 29, 2022

Since you added me as a co-author on the wrong commit, I honestly think you should just finish your PR, copy over the changes you'd like into my PR, and then close yours. Then git history will be correct, and git commit history could be two commits with the correct authors.

br-follow added a commit to br-follow/sqlmodel that referenced this issue Aug 29, 2022
`Field` are inserting into the database as nullable. This was introduced
in
tiangolo@9830ee0#r82434170
and described in tiangolo#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: tiangolo#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
Copy link
Contributor Author

br-follow commented Aug 29, 2022

I assumed @tiangolo would squash the commits before merging the PR. But I will commit to your PR.

@tiangolo
Copy link
Owner

Thanks for the report @JonasKs! And thanks for the help to you both.

#423 is now merged. I tweaked it a bit with extra cases and fixes. It will be available in the next version released in the next hours. SQLModel 0.0.8 🚀

@tiangolo tiangolo added bug Something isn't working answered and removed question Further information is requested labels Aug 30, 2022
@github-actions
Copy link

Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants