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_for() converter for Relationships not detecting nullable=False #336

Open
AbdealiLoKo opened this issue Aug 21, 2020 · 3 comments
Open

Comments

@AbdealiLoKo
Copy link
Collaborator

AbdealiLoKo commented Aug 21, 2020

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
@indiVar0508
Copy link
Contributor

indiVar0508 commented Aug 27, 2022

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}

@AbdealiLoKo
Copy link
Collaborator Author

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

@indiVar0508
Copy link
Contributor

indiVar0508 commented Sep 4, 2022

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 added a commit to indiVar0508/marshmallow-sqlalchemy that referenced this issue Sep 11, 2022
indiVar0508 added a commit to indiVar0508/marshmallow-sqlalchemy that referenced this issue Feb 24, 2023
indiVar0508 added a commit to indiVar0508/marshmallow-sqlalchemy that referenced this issue Mar 1, 2023
indiVar0508 added a commit to indiVar0508/marshmallow-sqlalchemy that referenced this issue Mar 1, 2023
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

No branches or pull requests

2 participants