Skip to content

Commit

Permalink
Remove the creation counter for Property. Fixes #6317. (googleapis#6335)
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Rossi authored Oct 31, 2018
1 parent 9f9ddb4 commit da6b75e
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 39 deletions.
12 changes: 8 additions & 4 deletions MIGRATION_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ The primary differences come from:
original implementation didn't allow in excess of 500 bytes, but it seems
the limit has been raised by the backend. (FWIW, Danny's opinion is that
the backend should enforce these limits, not the library.)
- I renamed `Property.__creation_counter_global` to
`Property._CREATION_COUNTER`.
- `Property.__creation_counter_global` has been removed as it seems to have
been included for a feature that was never implemented. See
[Issue #175][1] for original rationale for including it and [Issue #6317][2]
for discussion of its removal.
- `ndb` uses "private" instance attributes in many places, e.g. `Key.__app`.
The current implementation (for now) just uses "protected" attribute names,
e.g. `Key._key` (the implementation has changed in the rewrite). We may want
Expand Down Expand Up @@ -125,11 +127,13 @@ The primary differences come from:
- The whole `bytes` vs. `str` issue needs to be considered package-wide.
For example, the `Property()` constructor always encoded Python 2 `unicode`
to a Python 2 `str` (i.e. `bytes`) with the `utf-8` encoding. This fits
in some sense: the property name in the [protobuf definition][1] is a
in some sense: the property name in the [protobuf definition][3] is a
`string` (i.e. UTF-8 encoded text). However, there is a bit of a disconnect
with other types that use property names, e.g. `FilterNode`.
- There is a giant web of module interdependency, so runtime imports (to avoid
import cycles) are very common. For example `model.Property` depends on
`query` but `query` depends on `model`.

[1]: https://github.com/googleapis/googleapis/blob/3afba2fd062df0c89ecd62d97f912192b8e0e0ae/google/datastore/v1/entity.proto#L203
[1]: https://github.com/GoogleCloudPlatform/datastore-ndb-python/issues/175
[2]: https://github.com/googleapis/google-cloud-python/issues/6317
[3]: https://github.com/googleapis/googleapis/blob/3afba2fd062df0c89ecd62d97f912192b8e0e0ae/google/datastore/v1/entity.proto#L203
4 changes: 0 additions & 4 deletions src/google/cloud/ndb/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@ class Property(ModelAttribute):
_verbose_name = None
_write_empty_list = False
# Non-public class attributes.
_CREATION_COUNTER = 0
_FIND_METHODS_CACHE = {}

def __init__(
Expand Down Expand Up @@ -385,9 +384,6 @@ def __init__(
self._verbose_name = verbose_name
if write_empty_list is not None:
self._write_empty_list = write_empty_list
# Keep a unique creation counter. Note that this is not threadsafe.
Property._CREATION_COUNTER += 1
self._creation_counter = Property._CREATION_COUNTER

@staticmethod
def _verify_name(name):
Expand Down
38 changes: 7 additions & 31 deletions tests/unit/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,25 +330,12 @@ def test___hash__():
hash(wrapped)


@pytest.fixture
def zero_prop_counter():
counter_val = model.Property._CREATION_COUNTER
model.Property._CREATION_COUNTER = 0
try:
yield
finally:
model.Property._CREATION_COUNTER = counter_val


class TestProperty:
@staticmethod
def test_constructor_defaults(zero_prop_counter):
def test_constructor_defaults():
prop = model.Property()
# Check that the creation counter was updated.
assert model.Property._CREATION_COUNTER == 1
assert prop._creation_counter == 1
# Check that none of the constructor defaults were used.
assert prop.__dict__ == {"_creation_counter": 1}
assert prop.__dict__ == {}

@staticmethod
def _example_validator(prop, value):
Expand All @@ -360,7 +347,7 @@ def test__example_validator(self):
assert validated == "abcde"
assert self._example_validator(None, validated) == "abcde"

def test_constructor_explicit(self, zero_prop_counter):
def test_constructor_explicit(self):
prop = model.Property(
name="val",
indexed=False,
Expand All @@ -381,43 +368,32 @@ def test_constructor_explicit(self, zero_prop_counter):
assert prop._validator is self._example_validator
assert prop._verbose_name == "VALUE FOR READING"
assert not prop._write_empty_list
# Check that the creation counter was updated.
assert model.Property._CREATION_COUNTER == 1
assert prop._creation_counter == 1

@staticmethod
def test_constructor_invalid_name(zero_prop_counter):
def test_constructor_invalid_name():
with pytest.raises(TypeError):
model.Property(name=["not", "a", "string"])
with pytest.raises(ValueError):
model.Property(name="has.a.dot")
# Check that the creation counter was not updated.
assert model.Property._CREATION_COUNTER == 0

@staticmethod
def test_constructor_repeated_not_allowed(zero_prop_counter):
def test_constructor_repeated_not_allowed():
with pytest.raises(ValueError):
model.Property(name="a", repeated=True, required=True)
with pytest.raises(ValueError):
model.Property(name="b", repeated=True, default="zim")
# Check that the creation counter was not updated.
assert model.Property._CREATION_COUNTER == 0

@staticmethod
def test_constructor_invalid_choices(zero_prop_counter):
def test_constructor_invalid_choices():
with pytest.raises(TypeError):
model.Property(name="a", choices={"wrong": "container"})
# Check that the creation counter was not updated.
assert model.Property._CREATION_COUNTER == 0

@staticmethod
def test_constructor_invalid_validator(zero_prop_counter):
def test_constructor_invalid_validator():
with pytest.raises(TypeError):
model.Property(
name="a", validator=unittest.mock.sentinel.validator
)
# Check that the creation counter was not updated.
assert model.Property._CREATION_COUNTER == 0

def test_repr(self):
prop = model.Property(
Expand Down

0 comments on commit da6b75e

Please sign in to comment.