Skip to content

Commit

Permalink
The memo cache key needs to include the policy
Browse files Browse the repository at this point in the history
Externalizing the same object twice using different policies probably doesn't happen, but might.

A bug in the test was hiding the fact that it did not work.
  • Loading branch information
jamadden committed Aug 1, 2020
1 parent 87f9cc0 commit aa4b4e0
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 10 deletions.
8 changes: 5 additions & 3 deletions src/nti/externalization/externalization/externalizer.py
Expand Up @@ -257,11 +257,12 @@ def _to_external_object_state(obj, state, top_level=False):
assert obj is not None # caught by primitives already.

orig_obj_id = id(obj) # XXX: Relatively expensive on PyPy
cache_key = (orig_obj_id, state.policy)
if state.useCache:
value = state.memo.get(orig_obj_id, None)
value = state.memo.get(cache_key, None)
result = value[1] if value is not None else None
if result is None: # mark as in progress
state.memo[orig_obj_id] = (obj, _marker)
state.memo[cache_key] = (obj, _marker)
elif result is not _marker:
return result
else:
Expand Down Expand Up @@ -306,7 +307,7 @@ def _to_external_object_state(obj, state, top_level=False):
)

if state.useCache: # save result
state.memo[orig_obj_id] = (obj, result)
state.memo[cache_key] = (obj, result)
return result
except state.catch_components as t:
if top_level or state.catch_component_action is None:
Expand Down Expand Up @@ -399,6 +400,7 @@ def to_external_object(
FutureWarning
)


if policy is NotGiven:
if policy_name is not NotGiven:
policy = getUtility(IExternalizationPolicy, policy_name)
Expand Down
22 changes: 15 additions & 7 deletions src/nti/externalization/tests/test_externalization.py
Expand Up @@ -423,12 +423,14 @@ def child(self, new):

def toExternalObject(self, *args, **kwargs):
result = TestExternalizableInstanceDict.X.toExternalObject(self, *args, **kwargs)
# We deliberately don't pass the kwargs to the
# parent, in order to test the thread-local storage.
# (Or the policy_name)
if self.policy_name:
result['child'] = toExternalObject(self._x, policy_name=self.policy_name)
result['child'] = toExternalObject(self._x)
else:
# We deliberately don't pass the kwargs to the
# function, in order to test the thread-local storage.
# (Or the policy_name)
result['child'] = toExternalObject(self._x)
result['default_child'] = toExternalObject(self._x)
return result

def test_simple_roundtrip(self):
Expand Down Expand Up @@ -500,9 +502,15 @@ def test_registered_policy_name(self):
assert_that(ext['Last Modified'], is_(x.lastModified))

# child used ISO policy
ext = ext['child']
assert_that(ext['CreatedTime'], is_(self.created_string))
assert_that(ext['Last Modified'], is_(self.modified_string))
child = ext['child']
assert_that(child['CreatedTime'], is_(self.created_string))
assert_that(child['Last Modified'], is_(self.modified_string))

# next externalization used the default again
child = ext['default_child']
assert_that(child['CreatedTime'], is_(x.createdTime))
assert_that(child['Last Modified'], is_(x.lastModified))

finally:
component.getSiteManager().unregisterUtility(iso_policy, name='iso_policy')

Expand Down

0 comments on commit aa4b4e0

Please sign in to comment.