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

Allow for non-nullable fields to be handled by the database #196

Closed
eheiberg opened this issue Oct 5, 2020 · 4 comments · Fixed by #214
Closed

Allow for non-nullable fields to be handled by the database #196

eheiberg opened this issue Oct 5, 2020 · 4 comments · Fixed by #214

Comments

@eheiberg
Copy link

eheiberg commented Oct 5, 2020

Hi! Hopefully there's a good answer to this. I put in as much information as I could to be easy to replicate.

Summary

If our database is responsible for setting the value of a column, we do not want to set that column via SQLAlchemy; instead, we would like the database to handle it via trigger. We do not want the model generated to set the value to None.

This is especially useful in cases where we want timestamps, and we wish them to be handled by the database.

Setup (3 files)

1: openapi.yml

openapi: '3.0.2'
info:
  title: 'Null Value issue'
  version: '1.0.1'
servers:
  - url: https://api.server.test/v1
paths: {}  
components:
  schemas:
    Pet:
      type: object
      x-tablename: pets
      required:
        - name
      properties:
        id:
          type: integer
          format: int64
          x-primary-key: true
          x-autoincrement: true
        name:
          type: string
        created_at:
          type: string
          format: date-time
          nullable: false

2: main.py

from pathlib import Path

from open_alchemy import init_yaml
from open_alchemy import models
import sqlalchemy
from sqlalchemy import MetaData
from sqlalchemy import orm


OPENAPI_SPEC_FILE = Path("./openapi.yml")
SQLITE_PATH = 'sqlite:///petstore.db'

def main():
    """Define the main entrypoint of the program."""
    # Load the models.
    init_yaml(OPENAPI_SPEC_FILE)

    # Connect to the database.
    db = sqlalchemy.create_engine(SQLITE_PATH)
    db_session = orm.scoped_session(orm.sessionmaker(bind=db))

    metadata = MetaData()
    metadata.reflect(bind=db)

    # create a Pet
    pet = models.Pet(name="Rover")
    db_session.add(pet)

    # After the following line, Pet will attempt to be added to the database.
    # and an error will occur.
    db_session.commit()


if __name__ == "__main__":
    main()

3: A sqlite database: petstore.db

eheiberg:~/dev/scratch/oa-issue$ sqlite3 oa_issue/petstore.db
SQLite version 3.28.0 2019-04-15 14:49:49
Enter ".help" for usage hints.
sqlite> .fullschema
CREATE TABLE pets (
    id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
    name VARCHAR(64),
    created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL
);
/* No STAT tables available */
sqlite>

To setup the project...

poetry config virtualenvs.in-project true --local
poetry new oa_issue
cd oa_issue
poetry add SQLAlchemy OpenAlchemy

Copy all 3 files above into the main oa_issue folder. Then run the script.

poetry run python main.py

Results

You receive an error from the database.

sqlalchemy.exc.IntegrityError: (sqlite3.IntegrityError) 
NOT NULL constraint failed: pets.created_at
[SQL: INSERT INTO pets (name, created_at) VALUES (?, ?)]
[parameters: ('Rover', None)]
(Background on this error at: http://sqlalche.me/e/13/gkpj)

The error happens because the model is setting the created_at column to None explicitly. We don't get the same error on the id column, of course, because OpenAlchemy understands that this is auto-generated by the database, and sets the internal flag of x-generated on that attribute.

Just before the commit, looking in a debugger,

open_alchemy.models.Pet(id=None, name='Rover', created_at=None)

Ideally, we'd like the same thing for the created_at column.

Question

Is there any way to get around this, while still keeping the created_at
column in the openapi.yml file?

@jdkandersson
Copy link
Owner

jdkandersson commented Oct 6, 2020

Thanks for the detailed question, I appreciate the effort!

I think the problem is as follows:

  1. The column is set to be non-nullable
  2. I would assume that the behaviour of the default value is that if an INSERT is generated without created_at it is then auto-generated
  3. SQLAlchemy doesn't know that this is the expected behaviour and defaults the columns that are not provided to None
  4. This means that the insert statement attempts to push in a None value

The standard way in SQLAlchemy to solve this is is using the server_default argument. This will require a change to OpenAlchemy to support. Here is the proposal, please let me know if this addresses this feature request:

A new extension property (proposed: x-server-default) will be added. The value of that property is passed as the server_default argument to the Column constructor wrapped in SQLAlchemy.text.

I just did a POC and this works. Without it, constructing without created_at raises an exception, with it it does not. TBC whether the value gets generated correctly, I assume it does.


An additional recommendation would be to mark created_at as a readOnly: true value in your case to enforce that it does not get sent by the consumer.

@eheiberg
Copy link
Author

eheiberg commented Oct 6, 2020

@jdkandersson That sounds ideal! And yes, that is a great solution. I was thinking something along those lines.
And absolutely, I think the readOnly attribute will be used in our case.

@jdkandersson
Copy link
Owner

One thing I should have noted at the time, a temporary work around is to use the x-kwargs extension property to apply server_default to a column. More information is here: https://openapi-sqlalchemy.readthedocs.io/en/latest/technical_details/column_modifiers.html#additional-kwargs

@eheiberg
Copy link
Author

Oh wow, yes, that's a great idea. I don't think I knew enough about server_default at the time to understand, but your explanation makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants