marshmallow-sqlalchemy icon indicating copy to clipboard operation
marshmallow-sqlalchemy copied to clipboard

field_for() converter for Relationships not detecting nullable=False

Open AbdealiLoKo opened this issue 5 years ago • 3 comments

Hi, I had a case where I had the following (this is example code):

class Book:
    ...
    author_id = Column(Integer, ForeignKey('author.id'), nullable=False)
    author = relationship('Author', lazy='selectin')

And when I tried to do an author = auto_field() here on 'author' relationship - it gave me an error. i dug into it, and I found the following:

from marshmallow_sqlalchemy.convert import default_converter

rel = Book.__mapper__.get_property('author')
rel_kwargs = {}
default_converter._add_relationship_kwargs(rel_kwargs, rel)
print(rel_kwargs)
# Output: {'allow_none': True, 'required': False}

col = Book.__mapper__.get_property('author_id').columns[0]
col_kwargs = {}
default_converter._add_column_kwargs(col_kwargs, col)
print(col_kwargs)
# Output: {'required': True}

This got me confused ... cause it looks like :

  • Using the ForeignKey column - it detect the required=True correctly
  • Using the Relationship property- it does NOT detect the required=True correctly

AbdealiLoKo avatar Aug 21 '20 09:08 AbdealiLoKo

Hi @AbdealiJK ,

Tried to replicate the issue based on the debug notes, @_add_relationship_kwargs checks for a uselist attribute, if it not set to True it treats the relationship as scalar, by default sqlalchemy sets manytoone relation as scalar and set uselist as False . i was able to make it work by defining uselist=True and creating model objects relationship object in list and it seems to work.

Code used to test:

import sqlalchemy as sa
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import scoped_session, sessionmaker, relationship, backref

engine = sa.create_engine("sqlite:///:memory:")
session = scoped_session(sessionmaker(bind=engine))
Base = declarative_base()


class Author(Base):
    __tablename__ = "authors"
    id = sa.Column(sa.Integer, primary_key=True)
    name = sa.Column(sa.String, nullable=False)

    def __repr__(self):
        return "<Author(name={self.name!r})>".format(self=self)


class Book(Base):
    __tablename__ = "books"
    id = sa.Column(sa.Integer, primary_key=True)
    title = sa.Column(sa.String)
    author_id = sa.Column(sa.Integer, sa.ForeignKey("authors.id"), nullable=False)
    author = relationship("Author", uselist=True,backref=backref("books"))


Base.metadata.create_all(engine)

from src.marshmallow_sqlalchemy import SQLAlchemySchema, auto_field


class AuthorSchema(SQLAlchemySchema):
    class Meta:
        model = Author
        load_instance = True  # Optional: deserialize to model instances

    id = auto_field()
    name = auto_field()
    books = auto_field()


class BookSchema(SQLAlchemySchema):
    class Meta:
        model = Book
        load_instance = True

    id = auto_field()
    title = auto_field()
    author_id = auto_field()
    author = auto_field()

author = Author(name="Chuck Paluhniuk")
author_schema = AuthorSchema()
book = Book(title="Fight Club", author=[author])
session.add(author)
session.add(book)
session.commit()

from src.marshmallow_sqlalchemy.convert import default_converter

rel = Book.__mapper__.get_property('author')
print(rel.direction)
# Output: symbol('MANYTOONE')
rel_kwargs = {}
default_converter._add_relationship_kwargs(rel_kwargs, rel)
print(rel_kwargs)
# Output: {'allow_none': True, 'required': True}

col = Book.__mapper__.get_property('author_id').columns[0]
col_kwargs = {}
default_converter._add_column_kwargs(col_kwargs, col)
print(col_kwargs)
# Output: {'required': True}

indiVar0508 avatar Aug 27 '22 14:08 indiVar0508

In the example I was speaking of - The Book.author can have only 1 value So, why do you explicitly provide uselist=True ? I would not expect all users to force userlist=True even when it is not required.

sqlalchemy supports it - so I would expect marshmallow-sqlalchemy to also give me the same behavior

AbdealiLoKo avatar Aug 29 '22 14:08 AbdealiLoKo

Hi @AbdealiJK ,

Made an attempt to address the issue and have raised PR for same, using the DIRECTORY_MAPPING attribute available to decide if a column should be nullable or not,

indiVar0508 avatar Sep 04 '22 05:09 indiVar0508