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

Provide better type checking for values in from_dict #587

Merged
merged 4 commits into from
Oct 8, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 31 additions & 3 deletions tests/test_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@


class TestDict(unittest.TestCase):
def test_exclusive(self):
d = {
def setUp(self):
self.d = {
"Cpu": 1,
"Environment": [
{
Expand Down Expand Up @@ -34,8 +34,9 @@ def test_exclusive(self):
]
}

def test_valid_data(self):
t = Template()
cd = ecs.ContainerDefinition.from_dict("mycontainer", d)
cd = ecs.ContainerDefinition.from_dict("mycontainer", self.d)
td = ecs.TaskDefinition(
"taskdef",
ContainerDefinitions=[cd],
Expand All @@ -45,5 +46,32 @@ def test_exclusive(self):
t.add_resource(td)
t.to_json()

def test_invalid_toplevel_property(self):
self.d["BlahInvalid"] = "Invalid"
with self.assertRaises(AttributeError):
ecs.ContainerDefinition.from_dict("mycontainer", self.d)

def test_invalid_sub_property(self):
self.d["Environment"][0]["BlahInvalid"] = "Invalid"
with self.assertRaises(AttributeError):
ecs.ContainerDefinition.from_dict("mycontainer", self.d)

def test_toplevel_helper_fn(self):
self.d["Cpu"] = Ref("MyCPU")
cd = ecs.ContainerDefinition.from_dict("mycontainer", self.d)
self.assertEquals(cd.Cpu.data, {"Ref": "MyCPU"})

def test_sub_property_helper_fn(self):
self.d["Environment"][0]["Value"] = Ref("RegistryStorage")
cd = ecs.ContainerDefinition.from_dict("mycontainer", self.d)
self.assertEquals(cd.Environment[0].Value.data,
{"Ref": "RegistryStorage"})

def test_invalid_subproperty_definition(self):
self.d["Environment"][0] = "BadValue"
with self.assertRaises(ValueError):
ecs.ContainerDefinition.from_dict("mycontainer", self.d)


if __name__ == '__main__':
unittest.main()
49 changes: 45 additions & 4 deletions troposphere/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# See LICENSE file for full license.


import collections
import json
import re
import sys
Expand Down Expand Up @@ -162,10 +163,50 @@ def validate(self):
pass

@classmethod
def from_dict(cls, title, dict):
obj = cls(title)
obj.properties.update(dict)
return obj
def _from_dict(cls, title=None, **kwargs):
props = {}
for prop_name, value in kwargs.items():
try:
prop_attrs = cls.props[prop_name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cls.props could be missing too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it can - this is on AWSBaseObject, and AWSBaseObject sets up self.props in its init.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you're right, this is the cls object. i was thinking of the function you had before that took any class

except KeyError:
raise AttributeError("Object type %s does not have a "
"%s property." % (cls.__name__,
prop_name))
prop_type = prop_attrs[0]
if prop_name in kwargs:
value = kwargs[prop_name]
is_aws_object = False
try:
is_aws_object = issubclass(prop_type, BaseAWSObject)
# prop_type isn't a class
except TypeError:
pass
if is_aws_object:
if not isinstance(value, collections.Mapping):
raise ValueError("Property definition for %s must be "
"a Mapping type" % prop_name)
value = prop_type._from_dict(**value)

if isinstance(prop_type, list):
if not isinstance(value, list):
raise TypeError("Attribute %s must be a "
"list." % prop_name)
new_value = []
for v in value:
if not isinstance(v, collections.Mapping):
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would this work if this was a list of role names? does this always have to be a list of mapping types?

"Property definition for %s must be "
"a list of Mapping types" % prop_name)
new_value.append(prop_type[0]._from_dict(**v))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if v isn't a dict? it seems like this needs the same check for aws object

value = new_value
props[prop_name] = value
if title:
return cls(title, **props)
return cls(**props)

@classmethod
def from_dict(cls, title, d):
return cls._from_dict(title, **d)

def JSONrepr(self):
for k, (_, required) in self.props.items():
Expand Down