Fix ambiguity error with Polymorphism candidates (isinstance() problem)

[idchlife]

Right now Polymorphism fails when using something like this (models in sqlalchemy):

models:

class Product(BaseModel):
	discr = Column(String)

	__mapper_args__ = {
		'polymorphic_on': discr,
        'polymorphic_identity': "product"
	}

class VegetableProduct(Product):
	name = Column(String)

    __mapper_args__ = {
		'polymorphic_identity': "vegetable"
	}

class DairyProduct(VegetableProduct):
    __mapper_args__ = {
		'polymorphic_identity': "dairy"
	}

mapper:

resource_product = api.model("Product", {
	'discr': fields.String
})

product_mapper = {
	VegetableProduct: api.inherit("Vegetable", resource_product, { 'name': fields.String }),
	DairyProduct: api.inherit("Dairy", resource_product, { 'name': fields.String }),
}

resource_result = api.model("ResourceResult", {
	'products': fields.List(fields.Polymorphism(product_mapper))
})

This sparkles error here:

https://github.com/noirbizarre/flask-restplus/blob/master/flask_restplus/fields.py#L682

Because candidates will be: VegetableProduct and DairyProduct, since isinstance() will return True for both classes (I personally surprised it works like this)

But actually it should be only DairyProduct

Checking by __class__.__name__ removes this error and it works as intended.

I hope my quick fix did not break anything, I'm eager to update it if I don't know, more elegant solution is needed.

[j5awry]

Thank you for the contribution. We'll need to take a look at this one. In the meantime, could you adjust the tests based on the change in functionality? This change breaks the test below. Please refer to the contribution doc for what we expect:

https://github.com/noirbizarre/flask-restplus/blob/master/CONTRIBUTING.rst

   def test_polymorph_field_ambiguous_mapping(self, api):
        parent = api.model('Parent', {
            'name': fields.String,
        })
    
        child = api.inherit('Child', parent, {
            'extra': fields.String,
        })
    
        class Parent(object):
            name = 'parent'
    
        class Child(Parent):
            extra = 'extra'
    
        mapping = {
            Parent: parent,
            Child: child
        }
    
        thing = api.model('Thing', {
            'owner': fields.Polymorph(mapping),
        })
    
        with pytest.raises(ValueError):
>           api.marshal({'owner': Child()}, thing)
E           Failed: DID NOT RAISE <class 'ValueError'>
tests/test_fields.py:1205: Failed

[idchlife]

@j5awry sorry I did not answer right away. Did not expect your response to be this quick! 😄

I adjusted tests. With new functionality requirement is exactly opposite - no errors with ambiguous (for isinstance()) feature

I suspect this functionality better suits current way of writing mappings:

mapping = {
	Parent: parent_resource,
	Child: child_resource,
	SecondChild: second_child_resource
}

If we write explicitly source classes which instances should be serialized, we expect Polymorph to use exact classes, which is not possible right now with isinstance check, but possible with .__class__.__name__ check

[SteadBytes]

Thanks for using Flask-RESTPlus and for your contribution!

Just to explain the reason for the isinstance behaviour, from the isinstance Python Docs:

Return true if the object argument is an instance of the classinfo argument, or of a (direct, indirect or virtual) subclass thereof.

Using the built in Python API for inspecting classes would be preferable to manually inspecting __class__.__name__ attributes (i'll explain why in a moment). The type() function should work for this use case in fields.py:

candidates = [fields for cls, fields in iteritems(self.mapping) if type(value) == type(cls)]

Manually checking the __class__.__name__ attribute as it is not a strong guarantee that the actual types match due to the dynamic nature of Python (it could be modified at runtime). For example, given the following classes:

class Parent:
    pass

class Child:
    pass

p = Parent()
c = Child()

One could break the proposed changes:

>>> c.__class__.__name__ == p.__class.__name__
True
>>> c.__class__.__name__ = 'foo'
>>> c.__class__.__name__ == p.__class.__name__
False

When using the type function this is not the case:

>>> type(c) == type(p)
True
>>> c.__class__.__name__ = 'foo'
>>> type(c) == type(p)
True

Would you be willing to make these changes?

[idchlife]

@SteadBytes I tried to do as you said, but I suspect I will need advice and thoughts on problem which occured with with type() check.

python 2.7 and pypy python do not have similar behaviour as later versions.

python == 2.7

type(A()) == A
False

python > 2.7

type(A()) == A
True

May I suggest to make to value.__class__ == cls check? We remove possibility of runtime modification error, also we pass functionality tests for python2.7 and pypy

My last commit users simply __class__, without __name__

[SteadBytes]

For Python 2.7, the base class needs to inherit from object to make them new-style classes (although not very new anymore 😄 ):

class Parent(object):
    pass

class Child(Parent):
    pass

p = Parent()
c = Child()

The type() equality then works as expected:

>>> type(p) == type(c)
False

Apologies for making my previous example Python 3 only, does the above make sense?

[idchlife]

@SteadBytes ok, got it! Thanks! What do you think about my current code? With val.__class__ == cls check? Is it as suitable as type() check? Or should I revert to type() check anyway?

[SteadBytes]

If using type() passes the tests on all supported versions then I would prefer to use that over checking __class__ 👍

[idchlife]

@SteadBytes using type() requires to change tests functionality, adding (object) to test with Polymorphism. Inheritance within those tests did not have additional (object) in inheritance.

Using type() will require test files to be additionally modified. Using .__class__ will not require additional modifications, it works with python 2.7 >

Should I modify tests for type(), so classes inherit (object) for compatibility with python 2.7 and pypy, or should I leave current .__class__, which is compatible with all python versions and tests as they are at the current moment?

Test which would be modified for type():

https://github.com/noirbizarre/flask-restplus/blob/master/tests/test_swagger.py#L2240

[SteadBytes]

Yes those tests should be modified IMO. It is well documented that, in Python 2, classes should inherit from object in order to support the 'new style classes' which are now the default in Python 3.

Since that is the best practice we should follow suit for our Python 2 compatibility 👍

[idchlife]

@SteadBytes I believe this is it! Adjusted tests and using type() in the code.

[idchlife]

@SteadBytes sorry to ping you, but maybe there is possibility this changes can be merged, since tests are ok and it's relatively small changes and they are consistent with current documentation?

[SteadBytes]

@idchlife No need to apologise - thank you for pinging me! It can be difficult to keep track of issues so please don't hesitate to ping me if I've not replied for a while 👍 I'm just looking through this now, the test_polymorph_field_ambiguous_mapping test is now incorrect. It is meant to take an ambiguous mapping where the and assert that a ValueError is thrown. The mapping previously provided in the test is now no longer ambiguous, so instead of changing the test assertion as you've done we need to replace the mapping being tested with one that is now ambiguous. I'll post back shortly with a suggestion 😄

[SteadBytes]

Previously that error would be thrown if multiple candidates fields were available for a given value, this would be the case in your original issue - a mapping where multiple classes would return True for isinstance(value, cls). However, with these changes I don't think this 'ambiguity' is actually possible, this check https://github.com/idchlife/flask-restplus/blob/dc1783be156d91018c91a3ff7428e6358824961c/flask_restplus/fields.py#L681 can't be triggered and therefore the test_polymorph_field_ambiguous_mapping test is not required.

@j5awry I would appreciate if you could give your thoughts on this - have I misunderstood and do you agree with this new behaviour?

@idchlife Once we have this clarified (I just want to be sure that I've understood that properly) we will finish this up 👍

[j5awry]

@SteadBytes my understanding is the same -- the ambiguity is being removed, thus the test isn't valid. That's fine.

As for the approach, in concept I'm fine with it. I think we should do some good PR on this. It's a slight, subtle change that shouldn't break anything that currently works, but offers something to more experienced users.

[idchlife]

@SteadBytes hello again! Maybe I should just rename test file so it will be suitable now? Like testing the exact opposite functionality: lack of error because there cannot be ambiguity.

[SteadBytes]

@idchlife The correct behaviour is already tested in test_polymorph_field here. We could keep the test in as a regression test to ensure the old behaviour isn't re-introduced. Perhaps something like this:

    def test_polymorph_field_does_not_have_ambiguous_mappings(self, api):
        """
        Regression test for https://github.com/noirbizarre/flask-restplus/pull/691
        """
        parent = api.model('Parent', {
            'name': fields.String,
        })

        child = api.inherit('Child', parent, {
            'extra': fields.String,
        })

        class Parent(object):
            name = 'parent'

        class Child(Parent):
            extra = 'extra'

        mapping = {
            Parent: parent,
            Child: child
        }

        thing = api.model('Thing', {
            'owner': fields.Polymorph(mapping),
        })

        api.marshal({'owner': Child()}, thing)

[idchlife]

@SteadBytes thank you! I made last commit with your suggestion. I suspect all is ready now :)

Also, thank you very much for thorough replies!

[SteadBytes]

@idchlife You are more than welcome! Thank you for contributing to the project and for taking on board our feedback 😄

I will approve and merge the PR now 👍