graphql-python / graphene-sqlalchemy

Graphene SQLAlchemy integration
http://docs.graphene-python.org/projects/sqlalchemy/en/latest/
MIT License
980 stars 226 forks source link

Make column not required if server_default exists #364

Closed deepstqte closed 1 year ago

deepstqte commented 1 year ago

This PR does the following:

This is because when there is a server_default set, we might not wanna have it be required, and have the server_default value be used when creating a new entry.

deepstqte commented 1 year ago

@jnak would this addition be good? If so, I'm a first-time contributor so this would need a maintainer to approve running workflows.

erikwrede commented 1 year ago

hey, sorry for the long wait! I'm not sure if I understand the purpose of this PR correctly. Just because a column has a server_default, it doesn't mean that updates to it can set it to null, causing a null return for a required field. The server_default is only relevant during the insert operation; it translates

Column('test', DateTime, server_default=text('NOW()'))

to

test DATETIME DEFAULT NOW()

see https://docs.sqlalchemy.org/en/14/core/metadata.html#sqlalchemy.schema.Column.params.server_default

which wouldn't prevent the server (or some other instance with access to the DB) to run this:

UPDATE Model SET test=null;

In fact, setting the default to null is also supported. Conclusively, making the required status of a field depend on the server_default may yield unexpected behavior.

erikwrede commented 1 year ago

Hey @0x1723, I'm closing this for now due to the reasons stated above. If this comes up again, feel free to post an issue referencing this PR. Happy to elaborate further over there. ☺️

Cheers Erik