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

Include overidden by fk field that should be serialized #18

Conversation

@dpwrussell
Copy link
Contributor

commented Aug 28, 2015

When using joined table inheritance this is a common pattern:

class Person(Base):
    __tablename__ = 'person'

    type = sa.Column(sa.String(50))
    __mapper_args__ = {
        'polymorphic_identity': 'person',
        'polymorphic_on': type
    }

    id = sa.Column(sa.Integer, primary_key=True)
    name = sa.Column(sa.String)

class Author(Person):
    __tablename__ = 'authors'

    __mapper_args__ = {
        'polymorphic_identity': 'author'
    }

    id = sa.Column(sa.Integer, sa.ForeignKey('person.id'), primary_key=True)
    featured = sa.Column(sa.Boolean)

id is overridden and in the sub-class will always have the ForeignKey to the immediate super-class. This means that when determining which fields to include in the model serialization for Author, id is skipped. It looks like this:

{'featured': None, 'books': [1], 'type': u'author', 'name': u'Chuck Paluhniuk'}

If I was to serialize this with the PersonSchema then the result would be:

{'type': u'author', 'id': 1, 'name': u'Chuck Paluhniuk'}

which does include the id.

It is of course possible to specify include_fk=True, but I believe the reason that this exists is so that where there are relationships these are shown instead of the ids. It definitely does not make sense to have a relationship defined between Author.id and Person.id.

I think it would make sense in this situation for the overridden field to be included in the serialization and this can be easily determined by simply checking all the corresponding columns instead of just the first one.

I have added a unit test to make sure this field is serialized where appropriate and the existing test already makes sure that this does not apply to regular columns with foreign keys.

The above example would now serialize like so:

{'featured': None, 'books': [1], 'type': u'author', 'id': 1, 'name': u'Chuck Paluhniuk'}

Cheers

dpwrussell added some commits Aug 28, 2015

@@ -227,6 +264,10 @@ def test_include_fk(self, models):
student_fields2 = fields_for_model(models.Student, include_fk=True)
assert 'current_school_id' in student_fields2

def test_overridden_with_fk(self, models):
graded_paper_fields = fields_for_model(models.GradedPaper)

This comment has been minimized.

Copy link
@sloria

sloria Aug 31, 2015

Member

It might be worth explicitly passing include_fk=False to make sure we reach the added code, even if the default value for include_fk changes.

if not include_fk:
# Only skip a column if there is no overriden column
# which does not have a Foreign Key.
all_fk = True

This comment has been minimized.

Copy link
@sloria

sloria Aug 31, 2015

Member

This can be refactored using an else clause on the loop:

# Only skip a column if there is no overridden column
# which does not have a Foreign Key.
for column in prop.columns:
    if not column.foreign_keys:
        break
else:
    continue
@sloria

This comment has been minimized.

Copy link
Member

commented Aug 31, 2015

This is a good change. Thanks!

With the minor changes suggested above, this is good to merge.

sloria added a commit that referenced this pull request Sep 1, 2015

Merge pull request #18 from dpwrussell/include_overidden_by_fk_field_…
…that_should_be_serialized

Include overidden by fk field that should be serialized

@sloria sloria merged commit c1eaacb into marshmallow-code:dev Sep 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dpwrussell dpwrussell deleted the dpwrussell:include_overidden_by_fk_field_that_should_be_serialized branch Sep 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.