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

fix: error if eval()-ing repr(SchemaField) #1046

Merged
merged 5 commits into from Nov 4, 2021
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
16 changes: 10 additions & 6 deletions google/cloud/bigquery/schema.py
Expand Up @@ -268,7 +268,7 @@ def _key(self):
field_type = f"{field_type}({self.precision})"

policy_tags = (
() if self.policy_tags is None else tuple(sorted(self.policy_tags.names))
None if self.policy_tags is None else tuple(sorted(self.policy_tags.names))
)

return (
Expand Down Expand Up @@ -336,7 +336,11 @@ def __hash__(self):
return hash(self._key())

def __repr__(self):
return "SchemaField{}".format(self._key())
key = self._key()
policy_tags = key[-1]
policy_tags_inst = None if policy_tags is None else PolicyTagList(policy_tags)
adjusted_key = key[:-1] + (policy_tags_inst,)
return f"{self.__class__.__name__}{adjusted_key}"


def _parse_schema_resource(info):
Expand Down Expand Up @@ -407,7 +411,7 @@ class PolicyTagList(object):
`projects/*/locations/*/taxonomies/*/policyTags/*`.
"""

def __init__(self, names=()):
def __init__(self, names: Iterable[str] = ()):
self._properties = {}
self._properties["names"] = tuple(names)

Expand All @@ -425,7 +429,7 @@ def _key(self):
Returns:
Tuple: The contents of this :class:`~google.cloud.bigquery.schema.PolicyTagList`.
"""
return tuple(sorted(self._properties.items()))
return tuple(sorted(self._properties.get("names", ())))

def __eq__(self, other):
if not isinstance(other, PolicyTagList):
Expand All @@ -439,7 +443,7 @@ def __hash__(self):
return hash(self._key())

def __repr__(self):
return "PolicyTagList{}".format(self._key())
return f"{self.__class__.__name__}(names={self._key()})"

@classmethod
def from_api_repr(cls, api_repr: dict) -> "PolicyTagList":
Expand Down Expand Up @@ -478,5 +482,5 @@ def to_api_repr(self) -> dict:
A dictionary representing the PolicyTagList object in
serialized form.
"""
answer = {"names": [name for name in self.names]}
answer = {"names": list(self.names)}
return answer
51 changes: 50 additions & 1 deletion tests/unit/test_schema.py
Expand Up @@ -510,9 +510,30 @@ def test___hash__not_equals(self):

def test___repr__(self):
field1 = self._make_one("field1", "STRING")
expected = "SchemaField('field1', 'STRING', 'NULLABLE', None, (), ())"
expected = "SchemaField('field1', 'STRING', 'NULLABLE', None, (), None)"
self.assertEqual(repr(field1), expected)

def test___repr__evaluable_no_policy_tags(self):
field = self._make_one("field1", "STRING", "REQUIRED", "Description")
field_repr = repr(field)
SchemaField = self._get_target_class() # needed for eval # noqa

evaled_field = eval(field_repr)

assert field == evaled_field

Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness, do we need a round-trip test for the case where there is a policy tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would not hurt, I'll add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess what, that extra test actually revealed deeper issues in the repr() of SchemaField and PolicyTagList.

Copy link
Contributor

Choose a reason for hiding this comment

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

W00t!

def test___repr__evaluable_with_policy_tags(self):
policy_tags = PolicyTagList(names=["foo", "bar"])
field = self._make_one(
"field1", "STRING", "REQUIRED", "Description", policy_tags=policy_tags,
)
field_repr = repr(field)
SchemaField = self._get_target_class() # needed for eval # noqa

evaled_field = eval(field_repr)

assert field == evaled_field


# TODO: dedup with the same class in test_table.py.
class _SchemaBase(object):
Expand Down Expand Up @@ -786,6 +807,34 @@ def test___hash__not_equals(self):
set_two = {policy2}
self.assertNotEqual(set_one, set_two)

def test___repr__no_tags(self):
policy = self._make_one()
assert repr(policy) == "PolicyTagList(names=())"

def test___repr__with_tags(self):
policy1 = self._make_one(["foo", "bar", "baz"])
policy2 = self._make_one(["baz", "bar", "foo"])
expected_repr = "PolicyTagList(names=('bar', 'baz', 'foo'))" # alphabetical

assert repr(policy1) == expected_repr
assert repr(policy2) == expected_repr

def test___repr__evaluable_no_tags(self):
policy = self._make_one(names=[])
policy_repr = repr(policy)

evaled_policy = eval(policy_repr)

assert policy == evaled_policy

def test___repr__evaluable_with_tags(self):
policy = self._make_one(names=["foo", "bar"])
policy_repr = repr(policy)

evaled_policy = eval(policy_repr)

assert policy == evaled_policy


@pytest.mark.parametrize(
"api,expect,key2",
Expand Down