Skip to content

Commit

Permalink
Prevent error when time partitioning is populated with empty dict (#7904
Browse files Browse the repository at this point in the history
)

* Prevent error when time partitioning is populated with empty dict

As reported in internal bug 131167013, calling `table.time_partitioning`
can sometimes fail with `KeyError: 'type'` when attempting to read this
property on a non-partitioned table.

* Add tests for TimePartitioning.from_api_repr

* Remove unused imports

* Speed up lint session by installing local deps inplace.
  • Loading branch information
tswast authored May 9, 2019
1 parent a4abd9b commit 1b2c6f8
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 4 deletions.
4 changes: 2 additions & 2 deletions bigquery/google/cloud/bigquery/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -1787,7 +1787,7 @@ def type_(self):
"""google.cloud.bigquery.table.TimePartitioningType: The type of time
partitioning to use.
"""
return self._properties["type"]
return self._properties.get("type")

@type_.setter
def type_(self, value):
Expand Down Expand Up @@ -1849,7 +1849,7 @@ def from_api_repr(cls, api_repr):
google.cloud.bigquery.table.TimePartitioning:
The ``TimePartitioning`` object.
"""
instance = cls(api_repr["type"])
instance = cls()
instance._properties = api_repr
return instance

Expand Down
6 changes: 4 additions & 2 deletions bigquery/noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,10 @@ def lint(session):
serious code quality issues.
"""

session.install("black", "flake8", *LOCAL_DEPS)
session.install(".")
session.install("black", "flake8")
for local_dep in LOCAL_DEPS:
session.install("-e", local_dep)
session.install("-e", ".")
session.run("flake8", os.path.join("google", "cloud", "bigquery"))
session.run("flake8", "tests")
session.run("flake8", os.path.join("docs", "snippets.py"))
Expand Down
68 changes: 68 additions & 0 deletions bigquery/tests/unit/test_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,54 @@ def test__build_resource_w_custom_field_not_in__properties(self):
with self.assertRaises(ValueError):
table._build_resource(["bad"])

def test_time_partitioning_getter(self):
from google.cloud.bigquery.table import TimePartitioning
from google.cloud.bigquery.table import TimePartitioningType

dataset = DatasetReference(self.PROJECT, self.DS_ID)
table_ref = dataset.table(self.TABLE_NAME)
table = self._make_one(table_ref)

table._properties["timePartitioning"] = {
"type": "DAY",
"field": "col1",
"expirationMs": "123456",
"requirePartitionFilter": False,
}
self.assertIsInstance(table.time_partitioning, TimePartitioning)
self.assertEqual(table.time_partitioning.type_, TimePartitioningType.DAY)
self.assertEqual(table.time_partitioning.field, "col1")
self.assertEqual(table.time_partitioning.expiration_ms, 123456)
self.assertFalse(table.time_partitioning.require_partition_filter)

def test_time_partitioning_getter_w_none(self):
dataset = DatasetReference(self.PROJECT, self.DS_ID)
table_ref = dataset.table(self.TABLE_NAME)
table = self._make_one(table_ref)

table._properties["timePartitioning"] = None
self.assertIsNone(table.time_partitioning)

del table._properties["timePartitioning"]
self.assertIsNone(table.time_partitioning)

def test_time_partitioning_getter_w_empty(self):
from google.cloud.bigquery.table import TimePartitioning

dataset = DatasetReference(self.PROJECT, self.DS_ID)
table_ref = dataset.table(self.TABLE_NAME)
table = self._make_one(table_ref)

# Even though there are required properties according to the API
# specification, sometimes time partitioning is populated as an empty
# object. See internal bug 131167013.
table._properties["timePartitioning"] = {}
self.assertIsInstance(table.time_partitioning, TimePartitioning)
self.assertIsNone(table.time_partitioning.type_)
self.assertIsNone(table.time_partitioning.field)
self.assertIsNone(table.time_partitioning.expiration_ms)
self.assertIsNone(table.time_partitioning.require_partition_filter)

def test_time_partitioning_setter(self):
from google.cloud.bigquery.table import TimePartitioning
from google.cloud.bigquery.table import TimePartitioningType
Expand Down Expand Up @@ -2211,6 +2259,20 @@ def test_constructor_explicit(self):
self.assertEqual(time_partitioning.expiration_ms, 10000)
self.assertTrue(time_partitioning.require_partition_filter)

def test_from_api_repr_empty(self):
klass = self._get_target_class()

# Even though there are required properties according to the API
# specification, sometimes time partitioning is populated as an empty
# object. See internal bug 131167013.
api_repr = {}
time_partitioning = klass.from_api_repr(api_repr)

self.assertIsNone(time_partitioning.type_)
self.assertIsNone(time_partitioning.field)
self.assertIsNone(time_partitioning.expiration_ms)
self.assertIsNone(time_partitioning.require_partition_filter)

def test_from_api_repr_minimal(self):
from google.cloud.bigquery.table import TimePartitioningType

Expand All @@ -2223,6 +2285,12 @@ def test_from_api_repr_minimal(self):
self.assertIsNone(time_partitioning.expiration_ms)
self.assertIsNone(time_partitioning.require_partition_filter)

def test_from_api_repr_doesnt_override_type(self):
klass = self._get_target_class()
api_repr = {"type": "HOUR"}
time_partitioning = klass.from_api_repr(api_repr)
self.assertEqual(time_partitioning.type_, "HOUR")

def test_from_api_repr_explicit(self):
from google.cloud.bigquery.table import TimePartitioningType

Expand Down

0 comments on commit 1b2c6f8

Please sign in to comment.