http://gwt-code-reviews.appspot.com/1622803/diff/6001/user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java File user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java (right):
http://gwt-code-reviews.appspot.com/1622803/diff/6001/user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java#newcode71 user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java:71: nextId = map.size(); On 2012/05/23 19:27:46, skybrian wrote:
I think there's some implicit contract between ProxyStore and its
caller that
isn't documented? Since nextId field is never serialized and put()
never needs
to be called, here's a simple test that wouldn't work with the current implementation:
- create new proxy - call nextId() - serialize - deserialize - call nextId()
The map will still have zero elements so it will return the same id
twice. If you call nextId twice, isn't it being incremented in each case (meaning the values would differ between different calls)? You're only creating DefaultProxyStore once, right? Or is your point that it's unclear as to whether or not nextId is unique for a given Proxy, regardless of the DefaultProxyStore instance that is backing it?
Initializing from map.size() might be a reasonable thing to for
backward
compatibility with a previous version of DefaultProxyStore, but for
the current
version, it seems like we should actually be serializing the nextId
field and
restoring it? So EXPECTED_VERSION probably needs to be bumped up and
the
protocol changed?
Or alternately, the contract on ProxyStore isn't correct and this
sequence of
calls isn't allowed. In that case we should probably update the
javadoc on
ProxyStore to explain what the contract actually is.
http://gwt-code-reviews.appspot.com/1622803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
