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/05/29 18:41:50, rdayal wrote:
On 2012/05/29 17:08:02, rdayal wrote: > On 2012/05/27 08:41:35, tbroyer wrote: > > On 2012/05/23 01:08:29, rdayal wrote: > > > Sorry for the long delay on this, but doing a bit of thinking -
it seems
> > > incorrect to add an empty ValueProxy object to the > clientObjectToResolutionMap > > > when there's no representation of it. It seems that items should
be added
to > > > this list when they're actually resolved (i.e filled in; not
empty).
> > > > > > Can we come up with a solution that satisfies this? It may
require the
> > addition > > > of another "unresolved" List, and then migration over to the map
when
items > > are > > > filled in. > > > > I'll have to think more about it... > > > > > Though it's going to add more code, I think the workaround we
have here is
a > > bit > > > of a hack. I think it's an elegant hack, but it just feels wrong
to be
> working > > > around ValueProxy.equals(....) because we're trying to add empty
value
> proxies > > > to a "resolved" map when they really haven't been totally
resolved yet.
> > > > > > Thoughts? > > > > I really think we _need_ an IdentityHashMap here, whether we add
an
> "unresolved" > > list or not. We want to map proxy instances to their "resolution"
object, we
> > know we will be modifying the proxies, so a Map that uses equals()
is a
> mistake: > > a) you shouldn't use mutable objects as keys, and b) we don't care
about
> objects > > that compare equal, we want to map a very specific instance (be it
an entity
> > proxy or value proxy, it really doesn't matter) to a Resolution
object.
> > > > Your justification to use an IdentityHashMap does make sense. It
just strikes
me > as odd that we're using a Map where the key is mutable (in general,
regardless
> of whether or not this is an IdentityHashMap). It just seems that
there must
be > a better way to do this. I'd be happier if the key was some sort of
synthetic
ID > that was generated at creation time (of the ValueProxy or
EntityProxy), and
that > key mapped to both the resolution object and the proxy object.
In the interest of expediency, we can defer "fixing" this issue. I'd
request
that we add some doc to your existing comment indicating that this map
is
strange in that the key itself is mutated.
Done. I've actually removed the comment from patch-set #2, as this is unnecessary details: as I said previously, we're mapping instances to Resolution objects, independently of their "internal state" (and thus independently of their hashCode value and equals behavior), so IdentityHashMap is what should have been used from the beginning. The fact that RequestState guarantees there's at most one EntityProxy per domain object only adds confusion. https://gwt-code-reviews.appspot.com/1646803/diff/12001/user/src/com/google/web/bindery/requestfactory/server/RequestState.java File user/src/com/google/web/bindery/requestfactory/server/RequestState.java (right): https://gwt-code-reviews.appspot.com/1646803/diff/12001/user/src/com/google/web/bindery/requestfactory/server/RequestState.java#newcode49 user/src/com/google/web/bindery/requestfactory/server/RequestState.java:49: private final IdentityHashMap<Object, SimpleProxyId<?>> domainObjectsToId; domainObjectsToId as only ever assigned an IdentityHashMap instance; and given that an IdentityHashMap breaks the Map contract, I think it's better to use IdentityHashMap here to make it explicit what the behavior of the map is without having to look for the value that has been assigned to the field. https://gwt-code-reviews.appspot.com/1646803/diff/12001/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java File user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java (right): https://gwt-code-reviews.appspot.com/1646803/diff/12001/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java#newcode78 user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java:78: * ID to a persisted ID in Resolver#resolveClientProxy) in a way that can Note that there's a small bug in resolveClientProxy in that it calls setServerId on the SimpleProxyId instead of calling IdFatory#getId. getId updates an existing ID in a similar way, but Id Factory also keeps track of ephemeral IDs, and getId updates the internal bookmarking in addition to calling setServerId. This causes a SimpleProxyId to *not* be reused (and therefore a new EntityProxy to be created) when having 2 distinct domain-object instances representing the same entity. This is part of http://code.google.com/p/google-web-toolkit/issues/detail?id=7341 but is not enough to fix it. https://gwt-code-reviews.appspot.com/1646803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
