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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: disambiguate missing policy tags from explicitly unset policy tags #983

Merged
merged 7 commits into from Sep 24, 2021
Merged

fix: disambiguate missing policy tags from explicitly unset policy tags #983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f7c4a6
fix: disambiguate missing policy tags from explicitly unset policy tags
tswast Sep 22, 2021
200ad28
fix test for how to explicitly unset policy tags
tswast Sep 22, 2021
527b7b9
expect no policyTags key
tswast Sep 22, 2021
2479f7a
attempt to fix pytype when sentinel value is used
tswast Sep 23, 2021
cba58d2
avoid collections.abc
tswast Sep 23, 2021
c3c3d92
use enum for sentinel value
tswast Sep 23, 2021
dd73b64
Fix typo
plamut Sep 24, 2021
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
1 change: 1 addition & 0 deletions docs/reference.rst
View file
Open in desktop
Expand Up @@ -128,6 +128,7 @@ Schema
:toctree: generated

schema.SchemaField
schema.PolicyTagList


Query
Expand Down
2 changes: 2 additions & 0 deletions google/cloud/bigquery/__init__.py
View file
Open in desktop
Expand Up @@ -88,6 +88,7 @@
from google.cloud.bigquery.routine import RoutineReference
from google.cloud.bigquery.routine import RoutineType
from google.cloud.bigquery.schema import SchemaField
from google.cloud.bigquery.schema import PolicyTagList
from google.cloud.bigquery.table import PartitionRange
from google.cloud.bigquery.table import RangePartitioning
from google.cloud.bigquery.table import Row
Expand Down Expand Up @@ -140,6 +141,7 @@
"RoutineReference",
# Shared helpers
"SchemaField",
"PolicyTagList",
"UDFResource",
"ExternalConfig",
"BigtableOptions",
Expand Down
59 changes: 20 additions & 39 deletions google/cloud/bigquery/schema.py
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@
"""Schemas for BigQuery tables / queries."""

import collections
from typing import Optional
from typing import Optional, Tuple

from google.cloud.bigquery_v2 import types

Expand Down Expand Up @@ -81,15 +81,15 @@ class SchemaField(object):

def __init__(
self,
name,
field_type,
mode="NULLABLE",
description=_DEFAULT_VALUE,
fields=(),
policy_tags=None,
precision=_DEFAULT_VALUE,
scale=_DEFAULT_VALUE,
max_length=_DEFAULT_VALUE,
name: str,
field_type: str,
mode: Optional[str] = "NULLABLE",
plamut marked this conversation as resolved.
Show resolved Hide resolved
description: Optional[str] = _DEFAULT_VALUE,
fields: Optional[Tuple["SchemaField"]] = (),
policy_tags: Optional["PolicyTagList"] = _DEFAULT_VALUE,
precision: Optional[int] = _DEFAULT_VALUE,
scale: Optional[int] = _DEFAULT_VALUE,
max_length: Optional[int] = _DEFAULT_VALUE,
):
self._properties = {
"name": name,
Expand All @@ -105,28 +105,12 @@ def __init__(
self._properties["scale"] = scale
if max_length is not _DEFAULT_VALUE:
self._properties["maxLength"] = max_length
if policy_tags is not _DEFAULT_VALUE:
self._properties["policyTags"] = (
policy_tags.to_api_repr() if policy_tags is not None else None
)
self._fields = tuple(fields)

self._policy_tags = self._determine_policy_tags(field_type, policy_tags)

@staticmethod
def _determine_policy_tags(
field_type: str, given_policy_tags: Optional["PolicyTagList"]
) -> Optional["PolicyTagList"]:
"""Return the given policy tags, or their suitable representation if `None`.

Args:
field_type: The type of the schema field.
given_policy_tags: The policy tags to maybe ajdust.
"""
if given_policy_tags is not None:
return given_policy_tags

if field_type is not None and field_type.upper() in _STRUCT_TYPES:
return None

return PolicyTagList()

@staticmethod
def __get_int(api_repr, name):
v = api_repr.get(name, _DEFAULT_VALUE)
Expand All @@ -152,10 +136,10 @@ def from_api_repr(cls, api_repr: dict) -> "SchemaField":
mode = api_repr.get("mode", "NULLABLE")
description = api_repr.get("description", _DEFAULT_VALUE)
fields = api_repr.get("fields", ())
policy_tags = api_repr.get("policyTags", _DEFAULT_VALUE)

policy_tags = cls._determine_policy_tags(
field_type, PolicyTagList.from_api_repr(api_repr.get("policyTags"))
)
if policy_tags is not None and policy_tags is not _DEFAULT_VALUE:
policy_tags = PolicyTagList.from_api_repr(api_repr.get("policyTags"))
plamut marked this conversation as resolved.
Show resolved Hide resolved

return cls(
field_type=field_type,
Expand Down Expand Up @@ -230,7 +214,8 @@ def policy_tags(self):
"""Optional[google.cloud.bigquery.schema.PolicyTagList]: Policy tag list
definition for this field.
"""
return self._policy_tags
resource = self._properties.get("policyTags")
return PolicyTagList.from_api_repr(resource) if resource is not None else None

def to_api_repr(self) -> dict:
"""Return a dictionary representing this schema field.
Expand All @@ -244,10 +229,6 @@ def to_api_repr(self) -> dict:
# add this to the serialized representation.
if self.field_type.upper() in _STRUCT_TYPES:
answer["fields"] = [f.to_api_repr() for f in self.fields]
else:
# Explicitly include policy tag definition (we must not do it for RECORD
# fields, because those are not leaf fields).
answer["policyTags"] = self.policy_tags.to_api_repr()

# Done; return the serialized dictionary.
return answer
Expand All @@ -272,7 +253,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))
() if self.policy_tags is None else tuple(sorted(self.policy_tags.names))
)

return (
Expand Down
4 changes: 0 additions & 4 deletions tests/unit/job/test_load_config.py
View file
Open in desktop
Expand Up @@ -484,13 +484,11 @@ def test_schema_setter_fields(self):
"name": "full_name",
"type": "STRING",
"mode": "REQUIRED",
"policyTags": {"names": []},
}
age_repr = {
"name": "age",
"type": "INTEGER",
"mode": "REQUIRED",
"policyTags": {"names": []},
}
self.assertEqual(
config._properties["load"]["schema"], {"fields": [full_name_repr, age_repr]}
Expand All @@ -503,13 +501,11 @@ def test_schema_setter_valid_mappings_list(self):
"name": "full_name",
"type": "STRING",
"mode": "REQUIRED",
"policyTags": {"names": []},
}
age_repr = {
"name": "age",
"type": "INTEGER",
"mode": "REQUIRED",
"policyTags": {"names": []},
}
schema = [full_name_repr, age_repr]
config.schema = schema
Expand Down
64 changes: 22 additions & 42 deletions tests/unit/test_client.py
View file
Open in desktop
Expand Up @@ -1024,18 +1024,8 @@ def test_create_table_w_schema_and_query(self):
{
"schema": {
"fields": [
{
"name": "full_name",
"type": "STRING",
"mode": "REQUIRED",
"policyTags": {"names": []},
},
{
"name": "age",
"type": "INTEGER",
"mode": "REQUIRED",
"policyTags": {"names": []},
},
{"name": "full_name", "type": "STRING", "mode": "REQUIRED"},
{"name": "age", "type": "INTEGER", "mode": "REQUIRED"},
]
},
"view": {"query": query},
Expand Down Expand Up @@ -1069,18 +1059,8 @@ def test_create_table_w_schema_and_query(self):
},
"schema": {
"fields": [
{
"name": "full_name",
"type": "STRING",
"mode": "REQUIRED",
"policyTags": {"names": []},
},
{
"name": "age",
"type": "INTEGER",
"mode": "REQUIRED",
"policyTags": {"names": []},
},
{"name": "full_name", "type": "STRING", "mode": "REQUIRED"},
{"name": "age", "type": "INTEGER", "mode": "REQUIRED"},
]
},
"view": {"query": query, "useLegacySql": False},
Expand Down Expand Up @@ -2003,6 +1983,7 @@ def test_update_routine(self):

def test_update_table(self):
from google.cloud.bigquery.schema import SchemaField
from google.cloud.bigquery.schema import PolicyTagList
from google.cloud.bigquery.table import Table

path = "projects/%s/datasets/%s/tables/%s" % (
Expand All @@ -2029,7 +2010,6 @@ def test_update_table(self):
"type": "INTEGER",
"mode": "REQUIRED",
"description": "New field description",
"policyTags": {"names": []},
},
]
},
Expand All @@ -2040,7 +2020,15 @@ def test_update_table(self):
}
)
schema = [
SchemaField("full_name", "STRING", mode="REQUIRED", description=None),
# Explicly setting policyTags to no names should be included in the sent resource.
# https://github.com/googleapis/python-bigquery/issues/981
SchemaField(
"full_name",
"STRING",
mode="REQUIRED",
description=None,
policy_tags=PolicyTagList(names=()),
),
SchemaField(
"age", "INTEGER", mode="REQUIRED", description="New field description"
),
Expand Down Expand Up @@ -2078,7 +2066,6 @@ def test_update_table(self):
"type": "INTEGER",
"mode": "REQUIRED",
"description": "New field description",
"policyTags": {"names": []},
},
]
},
Expand Down Expand Up @@ -2197,21 +2184,14 @@ def test_update_table_w_query(self):
"type": "STRING",
"mode": "REQUIRED",
"description": None,
"policyTags": {"names": []},
},
{
"name": "age",
"type": "INTEGER",
"mode": "REQUIRED",
"description": "this is a column",
"policyTags": {"names": []},
},
{
"name": "country",
"type": "STRING",
"mode": "NULLABLE",
"policyTags": {"names": []},
},
{"name": "country", "type": "STRING", "mode": "NULLABLE"},
]
}
schema = [
Expand Down Expand Up @@ -6795,7 +6775,13 @@ def test_load_table_from_dataframe(self):
assert field["type"] == table_field.field_type
assert field["mode"] == table_field.mode
assert len(field.get("fields", [])) == len(table_field.fields)
assert field["policyTags"]["names"] == []
# Avoid accidentally updating policy tags when not explicitly included.
# https://github.com/googleapis/python-bigquery/issues/981
# Also, avoid 403 if someone has permission to write to table but
# not update policy tags by omitting policy tags we might have
# received from a get table request.
# https://github.com/googleapis/python-bigquery/pull/557
assert "policyTags" not in field
# Omit unnecessary fields when they come from getting the table
# (not passed in via job_config)
assert "description" not in field
Expand Down Expand Up @@ -8069,21 +8055,18 @@ def test_schema_to_json_with_file_path(self):
"description": "quarter",
"mode": "REQUIRED",
"name": "qtr",
"policyTags": {"names": []},
"type": "STRING",
},
{
"description": "sales representative",
"mode": "NULLABLE",
"name": "rep",
"policyTags": {"names": []},
"type": "STRING",
},
{
"description": "total sales",
"mode": "NULLABLE",
"name": "sales",
"policyTags": {"names": []},
"type": "FLOAT",
},
]
Expand Down Expand Up @@ -8116,21 +8099,18 @@ def test_schema_to_json_with_file_object(self):
"description": "quarter",
"mode": "REQUIRED",
"name": "qtr",
"policyTags": {"names": []},
"type": "STRING",
},
{
"description": "sales representative",
"mode": "NULLABLE",
"name": "rep",
"policyTags": {"names": []},
"type": "STRING",
},
{
"description": "total sales",
"mode": "NULLABLE",
"name": "sales",
"policyTags": {"names": []},
"type": "FLOAT",
},
]
Expand Down
9 changes: 1 addition & 8 deletions tests/unit/test_external_config.py
View file
Open in desktop
Expand Up @@ -78,14 +78,7 @@ def test_to_api_repr_base(self):
ec.schema = [schema.SchemaField("full_name", "STRING", mode="REQUIRED")]

exp_schema = {
"fields": [
{
"name": "full_name",
"type": "STRING",
"mode": "REQUIRED",
"policyTags": {"names": []},
}
]
"fields": [{"name": "full_name", "type": "STRING", "mode": "REQUIRED"}]
}
got_resource = ec.to_api_repr()
exp_resource = {
Expand Down