Just add the comment about the subtlety with the IdentityHashMap, and I
think we're good to go.
https://gwt-code-reviews.appspot.com/1646803/diff/1/user/src/com/google/web/bindery/requestfactory/server/Resolver.java
File user/src/com/google/web/bindery/requestfactory/server/Resolver.java
(right):
https://gwt-code-reviews.appspot.com/1646803/diff/1/user/src/com/google/web/bindery/requestfactory/server/Resolver.java#newcode365
user/src/com/google/web/bindery/requestfactory/server/Resolver.java:365:
new IdentityHashMap<BaseProxy, Resolution>();
On 2012/04/10 08:05:33, tbroyer wrote:
On 2012/04/09 15:44:43, rdayal wrote:
> Looks good, but I must say that I dont' quite understand how/why
this fixes
the
> problem. Can you point out how the code execution in this class
would have
> resulted in a breakage with a HashMap vs. an IdentityHashMap?
At line 632, a collection is populated with the result of
resolveClientValue(Object,Type), which is always an empty proxy
(properties are
populated later) –modulo reuse of a proxy if it has already been
resolved–. This
isn't an issue with EntityProxies, as each proxy as at a minimum a
stableId that
keys it to a domain object and distinguishes it from other proxies
(for other
domain objects), but ValueProxies compare equals() to each other by
their
properties' values only, independently of the underlying domain
object.
So, when you resolve a collection of ValueProxies, the first iteration
of the
loop (at line 632) creates a new, empty ValueProxy and puts it in the
clientObjectsToResolutions map. In the second iteration,
resolveClientValue will
create another ValueProxy, but will return the same Resolution because
the
ValueProxy compares equals() to the previous one, so getting the
Resolution from
the map will return the one for the first ValueProxy.
Using an IdentityHashMap fixes that lookup, so that we have one
Resolution
instance per proxy instance.
Because we have other guards for EntityProxies (keyed by stableId in
state.getBeansForPayload(), via resolveClientProxy), that change is
safe: the
goal is to have a most one proxy per domain object (which is
guaranteed in
RequestState for EntityProxies)
Ok, thanks for the explanation, that helps a lot. Two things I'd say:
-we need to document this somewhere, because it's pretty subtle
-it seems bad that we can get into states where ValueProxy.equals() is
doing "the wrong thing", but I'm not familiar enough with this code to
propose a good solution. I'd just say that it seems like a pitfall if we
need to have an IdentityHashMap to work around issues like this..
https://gwt-code-reviews.appspot.com/1646803/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):
https://gwt-code-reviews.appspot.com/1646803/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode587
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:587:
simpleFooRequest().returnValueProxies().with("simpleFoo.fooField").fire(new
Receiver<List<SimpleValueProxy>>() {
On 2012/04/10 08:05:33, tbroyer wrote:
On 2012/04/09 15:44:43, rdayal wrote:
> Sorry if this is naive question, but since the returnValueProxies
method
> implementation always fills in all of the properties (such as
fooField),
doesn't
> the with("simpleFoo.fooField") become redundant? Based on the
implementation
of
> returnValueProxies(), it seems that simpleFoo.fooField will always
be filled
in,
> regardless of whether or not you use the 'with' statement.
The with() is not passed down to the domain method (which is another
issue:
https://wave.google.com/wave/waveref/googlewave.com/w+WU4iAICkI/%7E/conv+root/b+QDorc1lYB
), it's a wire-protocol thing (it's not even sent to the server when
using the
JsonRpc dialect).
By default, for EntityProxies, only "value-type" properties are
populated (those
that can be encoded by ValueCodex; and lists of those). If you want
any other
kind of object to be serialized and sent to the client, it has to be
asked for
explicitly using with(); independently of whether the domain method
will
populate the property on the server-side.
AH!!! I did not know that. Thanks again. BTW, I can't read that wave -
can you add me to it?
https://gwt-code-reviews.appspot.com/1646803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
