Skip to content

Commit

Permalink
Fix ambiguity error with Polymorphism candidates (isinstance() proble…
Browse files Browse the repository at this point in the history
…m) (#691)

* Fix for better Polymorphism support

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

models:

```python
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:

```python
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)

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.

* adjusted ambiguity test for new feature

* stricter check for type

* changed to simple class check

* type() check for Polymorphism, adjusted tests

* type() check for Polymorphism, adjusted tests

* removed local dev files

* updated test name, also PR in docblock
  • Loading branch information
idchlife authored and SteadBytes committed Sep 12, 2019
1 parent ddc2aee commit a7e363a
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 6 deletions.
2 changes: 1 addition & 1 deletion flask_restplus/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ def output(self, key, obj, ordered=False, **kwargs):
if not hasattr(value, '__class__'):
raise ValueError('Polymorph field only accept class instances')

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

if len(candidates) <= 0:
raise ValueError('Unknown class: ' + value.__class__.__name__)
Expand Down
8 changes: 5 additions & 3 deletions tests/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -1177,7 +1177,10 @@ class Child2(object):
with pytest.raises(ValueError):
api.marshal({'owner': object()}, thing)

def test_polymorph_field_ambiguous_mapping(self, api):
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,
})
Expand All @@ -1201,8 +1204,7 @@ class Child(Parent):
'owner': fields.Polymorph(mapping),
})

with pytest.raises(ValueError):
api.marshal({'owner': Child()}, thing)
api.marshal({'owner': Child()}, thing)

def test_polymorph_field_required_default(self, api):
parent = api.model('Person', {
Expand Down
4 changes: 2 additions & 2 deletions tests/test_swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -2279,11 +2279,11 @@ def get(self):
assert path['get']['responses']['200']['schema']['$ref'] == '#/definitions/Output'

def test_polymorph_inherit_list(self, api, client):
class Child1:
class Child1(object):
name = 'Child1'
extra1 = 'extra1'

class Child2:
class Child2(object):
name = 'Child2'
extra2 = 'extra2'

Expand Down

0 comments on commit a7e363a

Please sign in to comment.