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

Kwargs for RelatedField

Open AbdealiLoKo opened this issue 6 years ago • 5 comments

I had a model which looks something like this:

import sqlalchemy as sa
import marshmallow_sqlalchemy as ma_sqla
from sqlalchemy.ext.declarative import declarative_base

BaseModel = declarative_base()


class BookCategory(BaseModel):
    __tablename__ = 'book_category'
    id = sa.Column(sa.Integer(), primary_key=True)
    name = sa.Column(sa.String(255), nullable=False, unique=True)


class Book(BaseModel):
    __tablename__ = 'book'
    id = sa.Column(sa.Integer(), primary_key=True)
    name = sa.Column(sa.String(255), nullable=False, unique=True)
    category_id = sa.Column(sa.Integer, sa.ForeignKey('book_category.id'), nullable=False)
    category = sa.orm.relationship(BookCategory, uselist=False)


field = ma_sqla.field_for(Book, 'category', column='name')
# print("field", field)
print("required", field.required)
print("allow_none", field.allow_none)

# Output:
required False
allow_none True

This was weird, because I was setting nullable=False in the category_id column I had. But ma-sqla is still making the marshmallow field with required=False and allow_none=True

I digged around and saw: https://github.com/marshmallow-code/marshmallow-sqlalchemy/blob/b6903803f95b68a65d2eeceaaad6e29a161a9c0c/marshmallow_sqlalchemy/convert.py#L243 Which essentially says, always use nullable=True if uselist is present in the relationship. Im not sure is this logic is correct here.

AbdealiLoKo avatar Dec 17 '18 09:12 AbdealiLoKo

Looks like a bug. Would you be up for a PR?

sloria avatar Dec 18 '18 14:12 sloria

Yep, im wondering what the expected behaviour is. Looks like if the relationship is a list -> nullable should be False (because empty list is a valid case) Looks like if the relationship is not a list -> nullable should be True

So, just if prop.uselist is True: -> if prop.uselist is not True: Does that sound right ?

AbdealiLoKo avatar Dec 18 '18 14:12 AbdealiLoKo

Not sure, and unfortunately don't have the time to code dive right now. Here's the PR that added that code https://github.com/marshmallow-code/marshmallow-sqlalchemy/pull/17 . I recommend evaluating that first.

sloria avatar Dec 18 '18 14:12 sloria

I've found more or less the same problem:

class ContactGroup(Model):
    id = Column(Integer, primary_key=True)
    name = Column(String(50), unique=True, nullable=False)

    def __repr__(self):
        return self.name

class Contact(Model):
    id = Column(Integer, primary_key=True)
    name = Column(String(150), unique=True, nullable=False)
    contact_group_id = Column(Integer, ForeignKey('contact_group.id'), nullable=False)
    contact_group = relationship("ContactGroup")

field = field_for(Contact, 'contact_group')
print("required", field.required)

Output: required False

dpgaspar avatar Mar 27 '19 12:03 dpgaspar

I am facing the same issue.

Not sure that I do understand SQLAlchemy relationship implementation details but I suppose that Related should always have allow_none=False. The only exception is case when we have MANYTOONE relation with nullable=True.

So, method should be like:

    def _add_relationship_kwargs(self, kwargs, prop):
        nullable = False
        if not self.DIRECTION_MAPPING[prop.direction.name]:
            nullable = all([pair[0].nullable for pair in prop.local_remote_pairs])
        kwargs.update({"allow_none": nullable, "required": not nullable})

Does it makes sense?

vit-ivanov avatar Jun 10 '20 13:06 vit-ivanov