Re: Fix for issue 5952: RequestContext#isChanged. (issue1601806)

[rda...@google.com]

LGTM.


http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode624
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:624:
* resolving property values, won't automatically edit proxies that
weren't
On 2012/04/10 11:15:27, tbroyer wrote:
> On 2012/04/09 15:12:50, rdayal wrote:
> > I'm an autobean newbie, but shouldn't we change the diffing
algorithm so that
> it
> > doesn't actually cause edits to proxies? I mean, when you a do a
diff, that
> > should never happen, right?

> The problem is that autobean has no notion of read only vs. editable
beans.
> That's something specific to proxies and request factory, where
getting a
> property (including from a visitor) will automatically edit its value
if it's a
> proxy.



Ugh. Ok, then we probably need to fix something with respect to getting
a property from a proxy - I wonder why getting a value from a proxy
implies an edit? Anyway, I'm just musing to myself here - whenever we
have to do tricks like this, it seems that there's an underlying problem
in the API.

http://gwt-code-reviews.appspot.com/1601806/diff/9001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/9001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode622
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:622:
setEditedProxiesMutable(false);
Nice, I like this.

http://gwt-code-reviews.appspot.com/1601806/diff/9001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode1281
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:1281:
assert bean.getTag(REQUEST_CONTEXT_STATE) == expectedState;
Nit: put parens around the assert expression.

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

[t.br...@gmail.com]

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode624
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:624:
* resolving property values, won't automatically edit proxies that
weren't

On 2012/04/27 14:52:12, rdayal wrote:
> Ugh. Ok, then we probably need to fix something with respect to
getting a
> property from a proxy - I wonder why getting a value from a proxy
implies an
> edit?

Otherwise, you have to pass the RequestContext along with the proxies
everywhere. With this auto-edit, you can simply edit the "top-level"
proxy and then use it like any other bean.

e.g.
proxy = ctx.edit(proxy);
proxy.getFoo().setBar("foo bar"); // auto-edit
vs.
proxy = ctx.edit(proxy);
ctx.edit(proxy.getFoo().setBar("foo bar");

FYI: to fix the TODO about reducing the payload while preserving the
"no-op request, validate proxies only", I was thinking about tracking
those proxies that are auto-edited vs. those that have been explicitly
edited, so that only the latter are unconditionally sent to the server;
the former being sent only if they contain changes.
The issue here is that for now the RequestFactoryEditorDriver explicitly
edits all proxies, instead of editing the top-level proxy and taking
advantage of the auto-edit feature (see issue 5981 which is related; I
do believe it's better that way though: editing proxies in CellTables
would break the edit/flush workflow; and that could be mitigated by
introducing "read-only editors" (that wouldn't edit the subproxy)).

...I wouldn't mind personally removing that auto-edit feature, even if
that's a "big breaking" change: it seems like overall it causes more
harm than good.

http://gwt-code-reviews.appspot.com/1601806/diff/9001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/9001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode1281
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:1281:
assert bean.getTag(REQUEST_CONTEXT_STATE) == expectedState;

On 2012/04/27 14:52:12, rdayal wrote:
> Nit: put parens around the assert expression.

Done.

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

[rda...@google.com]

Hey Thomas,

When I ran the tests after applying your change, I saw a failure in
EditorTest.test:

aused by: junit.framework.AssertionFailedError: expected=EditorBarTest
actual=FOO
at
com.google.web.bindery.requestfactory.gwt.client.ui.EditorTest.onSuccess(EditorTest.java:164)
at
com.google.web.bindery.requestfactory.gwt.client.ui.EditorTest.onSuccess(EditorTest.java:1)
at
com.google.web.bindery.requestfactory.shared.impl.AbstractRequest.onSuccess(AbstractRequest.java:129)
at
com.google.web.bindery.requestfactory.shared.impl.AbstractRequestContext.processPayload(AbstractRequestContext.java:377)
... 28 more


Are you seeing the same thing?

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

[t.br...@gmail.com]

On my local branch (off of r10773), "ant -f user/build.xml
test.dev.htmlunit
-Dgwt.junit.testcase.includes=com/google/web/bindery/requestfactory/gwt/client/ui/EditorTest.class"
passes:

[junit] Running
com.google.web.bindery.requestfactory.gwt.client.ui.EditorTest
[junit] Tests run: 7, Failures: 0, Errors: 0, Time elapsed: 49,035
sec

I'll rebase and see if that changes anything.

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

[t.br...@gmail.com]

Rebased upon r10958, "ant clean dist-dev" and then ran test.dev.htmlunit
and test.web.htmlunit, with the same BUILD SUCCESSFUL results.

I tried with OpenJDK (6u24) and Oracle JDK (6u27)
(with the clean/dist-dev being run with OpenJDK; only the test being run
with both JDKs)

Also tried with OpenJDK 7u3 (clean dist-dev, then test.dev.htmlunit;
using "JAVA_HOME=/usr/lib/jvm/java-7-openjdk-amd64/ ant ..." on the
command line; I'm not really sure the JAVA_HOME honored everywhere using
this technique); same (successful) results.

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

[Rajeev Dayal]

Thanks, Thomas. I'll take a look and see what's going on in our environment. I wonder if it's an order-of-execution failure (implying that there's statefulness between test runs that's not getting cleared).

[Freeland Abbott]

Depending on which targets are failing, we also run with FF3, which Thomas most likely doesn't....

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Rajeev Dayal]

Ah, thanks - though it's interesting that there'd be a browser-specific failure in response to this type of change. Thanks for pointing out the difference though; I'll double-check.

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Freeland Abbott]

Not saying there is, only that you should include it in the list of possibilities.  I sounds like Broyer only tested against HTMLUnit, and if you're doing the "normal" things, you'll have also tested with a real browser.

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Rajeev Dayal]

On Sun Apr 29 07:17:38 GMT-400 2012, <t.br...@gmail.com> wrote:

Ok, more information - I'm only getting the failure when I run the entire RequestFactorySuite. Still digging, but can you tell me if you see a failure if you run the entire suite?

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

[t.br...@gmail.com]

On 2012/05/04 22:05:23, rdayal wrote:

> Ok, more information - I'm only getting the failure when I run the
entire
> RequestFactorySuite. Still digging, but can you tell me if you see a
> failure if you run the entire suite?

Reproduced :-(

I also have a timeout in RequestFactoryTest#testChangedEdit, and (maybe
related) a server-side error:

5 mai 2012 00:58:11
com.google.web.bindery.requestfactory.server.RequestFactoryServlet
doPost
GRAVE: Unexpected error
java.lang.StringIndexOutOfBoundsException: String index out of range: 0
at java.lang.String.charAt(String.java:694)
at
com.google.web.bindery.autobean.vm.impl.JsonSplittable.create(JsonSplittable.java:45)
at
com.google.web.bindery.autobean.shared.impl.StringQuoter.split(StringQuoter.java:73)
at
com.google.web.bindery.autobean.shared.AutoBeanCodex.decode(AutoBeanCodex.java:54)
at
com.google.web.bindery.requestfactory.server.SimpleRequestProcessor.process(SimpleRequestProcessor.java:124)
at
com.google.web.bindery.requestfactory.server.RequestFactoryServlet.doPost(RequestFactoryServlet.java:133)


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

[t.br...@gmail.com]

OK, found it: the issue in EditorTest is that the SimpleBar that the
setUserName is applied to and the one in the SimpleFoo's barField are
not the same instances!

The instances are initialized in response to the finishTestAndReset call
from RequestBatcherTest, which explains the differing behavior when this
test is commented out (or moved to run later): in this case, everything
is initialized from the call to findSimpleFooById at the beginning of
EditorTest#test.

The real issue is actually that finisTestAndReset resets SimpleFoo
before resetting SimpleBar, so when SimpleFoo asks for
SimpleBar.getSingleton() it gets the "previous instance", and then the
SimpleBars are reinitialized.
When everything is initialized in EditorTest#test() through
findSimpleFooById, the SimpleFoo's barField and SimpleBar are the same
instances. It might be that they differ in subsequent tests but that
just does not seem to be an issue then.

Inverting the order of the reset calls in finishAndResetTest fixes the
issue.

I also simplified the changes to AbstractRequestContext (adding a
"diff'ing" state to the AbstractRequestContext/RequestState that's then
used in the various AutoBean Categories to
a) avoid auto-editing proxies (what was done by temporarily locking the
context in patchsets 1 to 6)
b) change the equals() behavior for EntityProxies (fixing issue 5952)

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

[t.br...@gmail.com]

I regularly (if not always) have a timeout in
RequestFactorytest#testChangedEdit, so we might have to bump
DELAY_TEST_FINISH (or refactory the test), but otherwise everything's OK
(running RequestFactorySuite in Eclipse) with patch-set #7.

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

[Rajeev Dayal]

Hey Thomas, 

Good find! I thought there was something funny with finishTestAndReset, but I wouldn't have spotted that :).

Replies inline:

On Fri May 04 22:08:03 GMT-400 2012, <t.br...@gmail.com> wrote:

OK, found it: the issue in EditorTest is that the SimpleBar that the
setUserName is applied to and the one in the SimpleFoo's barField are
not the same instances!

The instances are initialized in response to the finishTestAndReset call
from RequestBatcherTest, which explains the differing behavior when this
test is commented out (or moved to run later): in this case, everything
is initialized from the call to findSimpleFooById at the beginning of
EditorTest#test.

Ok, I see.

 

The real issue is actually that finisTestAndReset resets SimpleFoo
before resetting SimpleBar, so when SimpleFoo asks for
SimpleBar.getSingleton() it gets the "previous instance", and then the
SimpleBars are reinitialized.
When everything is initialized in EditorTest#test() through
findSimpleFooById, the SimpleFoo's barField and SimpleBar are the same
instances. It might be that they differ in subsequent tests but that
just does not seem to be an issue then.

Ugh, that sucks - fortunately, it's just an artifact of testing. A better fix might be to change the reset(...) in SimpleFoo to automatically reset the SimpleBar entity? What do you think?

 

Inverting the order of the reset calls in finishAndResetTest fixes the
issue.

I also simplified the changes to AbstractRequestContext (adding a
"diff'ing" state to the AbstractRequestContext/RequestState that's then
used in the various AutoBean Categories to
a) avoid auto-editing proxies (what was done by temporarily locking the
context in patchsets 1 to 6)
b) change the equals() behavior for EntityProxies (fixing issue 5952)

Ok, I'll take a detailed look.

 

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

[rda...@google.com]

Ok, the original failure we were looking at is gone! Yay! Now, two more
to deal with:

1)
testChangedEdit(com.google.web.bindery.requestfactory.gwt.client.RequestFactoryTest)java.lang.RuntimeException:
Remote test failed at 127.0.0.1 / Mozilla/5.0 (X11; U; Linux i686
(x86_64); en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
at
com.google.gwt.junit.JUnitShell.processTestResult(JUnitShell.java:1283)
at com.google.gwt.junit.JUnitShell.runTestImpl(JUnitShell.java:1353)
at com.google.gwt.junit.JUnitShell.runTestImpl(JUnitShell.java:1304)
at com.google.gwt.junit.JUnitShell.runTest(JUnitShell.java:652)
at
com.google.gwt.junit.client.GWTTestCase.runTest(GWTTestCase.java:441)
at
com.google.testing.junit.runner.OutputRedirectingTestResult.protect(Output
...
(failed due to timeout exceeding 30s)


2)
testChangedEdit(com.google.web.bindery.requestfactory.gwt.client.RequestFactoryExceptionHandlerTest)java.lang.RuntimeException:
Remote test failed at 127.0.0.1 / Mozilla/5.0 (X11; U; Linux i686
(x86_64); en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
at
com.google.gwt.junit.JUnitShell.processTestResult(JUnitShell.java:1283)
at com.google.gwt.junit.JUnitShell.runTestImpl(JUnitShell.java:1353)
at com.google.gwt.junit.JUnitShell.runTestImpl(JUnitShell.java:1304)
at com.google.gwt.junit.JUnitShell.runTest(JUnitShell.java:652)
at
com.google.gwt.junit.client.GWTTestCase.runTest(GWTTestCase.java:441)
.........
(failed due to timeout exceeding 30s)

I also saw two errors on the server-side, are you seeing these?

java.lang.StringIndexOutOfBoundsException: String index out of range: 0
at java.lang.String.charAt(String.java:694)
at
com.google.web.bindery.autobean.vm.impl.JsonSplittable.create(JsonSplittable.java:45)
at
com.google.web.bindery.autobean.shared.impl.StringQuoter.split(StringQuoter.java:73)
at
com.google.web.bindery.autobean.shared.AutoBeanCodex.decode(AutoBeanCodex.java:54)
at
com.google.web.bindery.requestfactory.server.SimpleRequestProcessor.process(SimpleRequestProcessor.java:124)
at
com.google.web.bindery.requestfactory.server.RequestFactoryServlet.doPost(RequestFactoryServlet.java:133)

at javax.servlet.http.HttpServlet.service(HttpServlet.java:727)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:820)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:487)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:362)
at
com.google.gwt.thirdparty.org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:181)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:729)
at
com.google.gwt.thirdparty.org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.RequestLogHandler.handle(RequestLogHandler.java:49)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
at
com.google.gwt.thirdparty.org.mortbay.jetty.Server.handle(Server.java:324)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:505)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.headerComplete(HttpConnection.java:829)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpParser.parseNext(HttpParser.java:513)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpParser.parseAvailable(HttpParser.java:211)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:380)
at
com.google.gwt.thirdparty.org.mortbay.io.nio.SelectChannelEndPoint.run(SelectChannelEndPoint.java:395)
at
com.google.gwt.thirdparty.org.mortbay.thread.QueuedThreadPool.run(QueuedThreadPool.java:488)
[ERROR] 500 - POST
/com.google.web.bindery.requestfactory.gwt.RequestFactoryExceptionHandlerTest.JUnit/gwtRequest
(127.0.0.1) 1503 bytes
The following exception about an entity with a null version is expected
May 7, 2012 3:27:28 PM
com.google.web.bindery.requestfactory.server.RequestFactoryServlet
doPost
SEVERE: Unexpected error
com.google.web.bindery.requestfactory.server.UnexpectedException: The
persisted entity with id 1 has a null version
at
com.google.web.bindery.requestfactory.server.SimpleRequestProcessor.createReturnOperations(SimpleRequestProcessor.java:290)
at
com.google.web.bindery.requestfactory.server.SimpleRequestProcessor.process(SimpleRequestProcessor.java:232)
at
com.google.web.bindery.requestfactory.server.SimpleRequestProcessor.process(SimpleRequestProcessor.java:127)
at
com.google.web.bindery.requestfactory.server.RequestFactoryServlet.doPost(RequestFactoryServlet.java:133)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:727)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:820)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:487)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:362)
at
com.google.gwt.thirdparty.org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216)
at
com.google.gwt.thirdparty.org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:181)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:729)
at
com.google.gwt.thirdparty.org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.RequestLogHandler.handle(RequestLogHandler.java:49)
at
com.google.gwt.thirdparty.org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
at
com.google.gwt.thirdparty.org.mortbay.jetty.Server.handle(Server.java:324)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:505)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.content(HttpConnection.java:843)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpParser.parseNext(HttpParser.java:647)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpParser.parseAvailable(HttpParser.java:211)
at
com.google.gwt.thirdparty.org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:380)
at
com.google.gwt.thirdparty.org.mortbay.io.nio.SelectChannelEndPoint.run(SelectChannelEndPoint.java:395)
at
com.google.gwt.thirdparty.org.mortbay.thread.QueuedThreadPool.run(QueuedThreadPool.java:488)





http://gwt-code-reviews.appspot.com/1601806/diff/27001/user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java
File
user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/27001/user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java#newcode24
user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java:24:
public class RequestPayloadJreTest extends RequestPayloadTest {
javadoc

http://gwt-code-reviews.appspot.com/1601806/diff/27001/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java
File
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/27001/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java#newcode47
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java:47:
protected static class RecordingRequestTransport implements
RequestTransport {
javadoc

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