Allow detached JDO objects to be transferred via RPC

[ri...@google.com]

Reviewers: bobvawter_google.com,

Description:
Attempting to transfer objects marked as
@PersistenceCapable(detachable="true") via RPC currently fails due to
mismatched class bytecode between the server and client. This patch
detects such objects and manually serializes the extra information
required to maintain the identity of the object should it be transferred
from aJDO-enabled server to a client and back to the server. The object
must be in the Detached state in order for the transfer to be meaningful
-- in general, applications sending persistent objects over the wire
need to follow the same rules they would in order to successfully
serialize and deserialize such objects.

Future work:

1) Deal with classes whose persistence is specified via an xml
configuration file rather than via annotations.
2) Detect if an attempt is made to transfer an object that is not in the
Detached state.

Please review this at http://gwt-code-reviews.appspot.com/47807

Affected files:
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
user/src/com/google/gwt/user/client/rpc/WeakMapping.java
user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java
user/src/com/google/gwt/user/server/rpc/impl/Base64Utils.java
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java

user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java

user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
user/super/com/google/gwt/emul/java/lang/Object.java

user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/WeakMapping.java
user/test/com/google/gwt/user/server/rpc/RPCTest.java

[cromw...@gmail.com]

On 2009/06/26 19:57:28, Dan Rice wrote:

Two general comments:
1) What are the implications of stashing an extra field on
java.lang.Object, is there any impact on output size?

2) I've noticed that the JDO enhanced classes seem to track field
mutations and update the jdoDetachedState. This patch just seems to
preserve the original jdoDetachedState on the client by tunneling it on
an expando. What happens if an object sent from the server to the
client, has modifications performed (mutations), and then is sent back
to the server for a PersistenceManager.makePersistent() or
EntityManager.merge()? Will the PMF/EMF just decide that nothing has
changed and treat it as a no-op?

http://gwt-code-reviews.appspot.com/47807

[Daniel Rice (דניאל רייס)‎]

1) According to Bob, the extra field shouldn't cost anything unless it's used.

2) The code sets the 'modified' bits when the data is deserialized on the server, i.e., it marks all the fields as dirty so the object will be fully updated when the user calls makePersistent on it.

I think this is sufficent, because:

a) It's not practical to track changes on the client (for example, JSNI methods could easily bypass any mechanism we put in place), and

b) AppEngine doesn't do partial updates anyway, so there's no performance advantage in that environment for trying to do more.

By the way, the design is largely based on one of the emails you sent to the gwt mailing list, so thanks for helping me get some clarity on what needs to happen,

Dan

[bruno]

Hi all,

I did similar development with Gilead adapter for GAE (available on
Gilead site : http://gilead.sourceforge.net), so I guess it would be a
good idea to share our knwoledge about it.
I modified nearly the same files, but I more generic concepts
(ISerializationFilter and ISerializationTransformer) to allow third
parties serialization tuning (as for Hibernate for example).

I detailed it on the following post :
http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/f6bc9f5558684c1

Regards
Bruno

On 27 juin, 05:57, Daniel Rice (דניאל רייס) <r...@google.com> wrote:
> 1) According to Bob, the extra field shouldn't cost anything unless it's
> used.
> 2) The code sets the 'modified' bits when the data is deserialized on the
> server, i.e., it marks all the fields as dirty so the object will be fully
> updated when the user calls makePersistent on it.
> I think this is sufficent, because:
>
> a) It's not practical to track changes on the client (for example, JSNI
> methods could easily bypass any mechanism we put in place), and
> b) AppEngine doesn't do partial updates
> anyway, so there's no performance advantage in that environment for trying
> to do more.
>
> By the way, the design is largely based on one of the emails you sent to the
> gwt mailing list, so thanks for helping me get some clarity on what needs to
> happen,
>
> Dan
>

[bo...@google.com]

Please extract the expando work to the gwt.core.client package and add
test cases for it, as it's a generally-useful facility to offer.

Needs tests of the JDO testcases.

@Cromwellian,
The expando field just reserves a bit of the Object-field namespace
since it's lazily initialized.

http://gwt-code-reviews.appspot.com/47807/diff/1/9
File user/src/com/google/gwt/user/client/rpc/WeakMapping.java (right):

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode16
Line 16: package com.google.gwt.user.client.rpc;
Move this to the core.client package, since it's not specific to the rpc
system.

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode22
Line 22: * A class associating a (key -> value) map with arbitrary
sourceobjects. A WeakHashMap
source objects

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode24
Line 24: * garbage collected, the map associated with it will be
collected as well.
This javadoc should accurately describe both hosted and web-mode code.

The specifics of the implementation are usually documented as a regular
block comment just below the class declaration.

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode31
Line 31: public static Object get(Object instance, String key) {
Needs documentation, especially on the namespace of the key.

http://gwt-code-reviews.appspot.com/47807/diff/1/8
File user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/8#newcode159
Line 159: Class annotationClass =
Class.forName("javax.jdo.annotations.PersistenceCapable");
Raw type

http://gwt-code-reviews.appspot.com/47807/diff/1/5
File user/src/com/google/gwt/user/server/rpc/impl/Base64Utils.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/5#newcode2
Line 2: * Copyright 2008 Google Inc.
Copyright date should be current for new files.

http://gwt-code-reviews.appspot.com/47807/diff/1/5#newcode19
Line 19: * A utility to decode and encode byte arrays as Strings, using
only "safe" characters.
Formatting looks off.

http://gwt-code-reviews.appspot.com/47807/diff/1/7
File
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/7#newcode315
Line 315: * Returns true if the given field is named 'jdoDetachedState'
and the enclosing
Formatting

http://gwt-code-reviews.appspot.com/47807/diff/1/7#newcode322
Line 322: Class[] interfaces =
field.getDeclaringClass().getInterfaces();
If

A extends B;
B implements I;

does A.class.getInterfaces() include I?

Would it be easier to have a static field that's initialized with a
Class.forName and Class.isInstance() used instead?

http://gwt-code-reviews.appspot.com/47807/diff/1/4
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/4#newcode611
Line 611: // Check for the presence of the javax.jdo.spi.Detachable
interface. If it is present,
Use a block comment so the formatter will format this correctly.

http://gwt-code-reviews.appspot.com/47807/diff/1/4#newcode614
Line 614: Class[] interfaces = instanceClass.getInterfaces();
Would a speculatively-initialized Class literal field and
Class.isInstance() work more correctly?

http://gwt-code-reviews.appspot.com/47807/diff/1/4#newcode676
Line 676: assert version == 1;
This should throw a runtime exception, since end-users likely won't have
assertions turned on.

http://gwt-code-reviews.appspot.com/47807/diff/1/4#newcode682
Line 682: case 'N':
What do these values mean?

http://gwt-code-reviews.appspot.com/47807/diff/1/4#newcode689
Line 689: Class c = Class.forName(className);
Use of raw types should be flagged by eclipse.

Class<? extends Externalizable> c =
Class.forName(className).asSubclass(Externalizable.class)

Add a ClassCastException to the catch block below.

http://gwt-code-reviews.appspot.com/47807/diff/1/6
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/6#newcode666
Line 666: // Check for the presence of the javax.jdo.spi.Detachable
interface. If it is present,
Block comment and formatting.

http://gwt-code-reviews.appspot.com/47807/diff/1/6#newcode669
Line 669: Class[] interfaces = instanceClass.getInterfaces();
Class.isInstance

http://gwt-code-reviews.appspot.com/47807/diff/1/6#newcode713
Line 713: * null is indicated by 'N', an externalizable field is
indicated by an 'E'
Use static final fields for these values, and reference them with the
@value javadoc tag.

http://gwt-code-reviews.appspot.com/47807/diff/1/6#newcode734
Line 734: out.writeInt(1);
Extract the number 1 to a constant.

http://gwt-code-reviews.appspot.com/47807/diff/1/10
File user/super/com/google/gwt/emul/java/lang/Object.java (right):

http://gwt-code-reviews.appspot.com/47807/diff/1/10#newcode45
Line 45: * an expando containing a String -> Object mapping on arbitrry
Objects.
arbitrary

http://gwt-code-reviews.appspot.com/47807/diff/1/10#newcode50
Line 50: private transient Object expando;
Sort order isn't enforced by checkstyle, but fields should generally be
sorted.

This field should be declared as JavaScriptObject type.

http://gwt-code-reviews.appspot.com/47807/diff/1/11
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/WeakMapping.java
(right):

http://gwt-code-reviews.appspot.com/47807/diff/1/11#newcode21
Line 21: public class WeakMapping {
Needs javadoc. The eclipse checkstyle rules should have put a warning
here.

http://gwt-code-reviews.appspot.com/47807/diff/1/11#newcode27
Line 27: instance.@java.lang.Object::expando[key] = value;
You need to prefix the key in order to prevent conflicts with builtin
properties; typicially expando[':' + key]

http://gwt-code-reviews.appspot.com/47807/diff/1/3
File user/test/com/google/gwt/user/server/rpc/RPCTest.java (right):

http://gwt-code-reviews.appspot.com/47807/diff/1/3#newcode167
Line 167: public static void testBase64Utils() {
Move these to their own test class.

http://gwt-code-reviews.appspot.com/47807

[ri...@google.com]

http://gwt-code-reviews.appspot.com/47807/diff/1/9
File user/src/com/google/gwt/user/client/rpc/WeakMapping.java (right):

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode31

Line 31: public static Object get(Object instance, String key) {

I'm not sure what can be said about the namespace. What do you have in
mind?

[bo...@google.com]

http://gwt-code-reviews.appspot.com/47807/diff/1/9
File user/src/com/google/gwt/user/client/rpc/WeakMapping.java (right):

http://gwt-code-reviews.appspot.com/47807/diff/1/9#newcode31
Line 31: public static Object get(Object instance, String key) {

On 2009/06/29 15:34:02, Dan Rice wrote:
> I'm not sure what can be said about the namespace. What do you have
in mind?

"Note that the key space is module-wide, so some care should be taken to
choose sufficiently unique identifiers."

http://gwt-code-reviews.appspot.com/47807

[bruno]

Hi Bod and Dan,

Very happy to see that this issue is on its way to be solved :)
Nevertheless, I think it would be useful if JDO management code and
RPC serialization can be separated : instead of having

for (Class iface : interfaces) {
if ("javax.jdo.spi.Detachable".equals(iface.getName())) {
// JDO specific code
}
}

a code like this would be better :

for (Class iface : interfaces) {
if (this.serializationTransformer.isTransformable(iface)) {
serializationTransformer.transform(instanceClass);
}
}

where JDO specific code is encapsulated in a specific code.
As another side benefit, this mechanism could be used to enhance GWT
serialization process for any other persistence library.

HTH
Bruno

[Daniel Rice (דניאל רייס)‎]

Hi Bruno -

  I've uploaded a new patch that's more along these lines.  Hopefully it will do what you are looking for.  Thanks,

Dan