[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
LGTM
On Tue, Oct 5, 2010 at 4:21 PM, Amit Manjhi wrote:
>
>
> On Tue, Oct 5, 2010 at 4:10 PM, wrote:
>
>>
>> http://gwt-code-reviews.appspot.com/959801/diff/5004/20001
>>
>> File
>> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
>> (right):
>>
>> http://gwt-code-reviews.appspot.com/959801/diff/5004/20001#newcode77
>>
>> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:77:
>> this.version = (Integer)
>> (jsonObject.has(Constants.ENCODED_VERSION_PROPERTY)
>> Why are you hard coding this requirement that the thing be an integer?
>>
>>
> The version property is currently hard coded to be an Integer. I'm just
> using that here.
>
>
>> http://gwt-code-reviews.appspot.com/959801/diff/5004/20002
>>
>> File
>>
>> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
>> (right):
>>
>> http://gwt-code-reviews.appspot.com/959801/diff/5004/20002#newcode200
>>
>> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:200:
>>
>> public void testEndToEndSmartDiff_NoChange_NoVersion() throws Exception
>> {
>> Why keep the test that tests surviving what would be a client bug?
>>
>>
>> http://gwt-code-reviews.appspot.com/959801/show
>>
>
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
On Tue, Oct 5, 2010 at 4:10 PM, wrote:
>
> http://gwt-code-reviews.appspot.com/959801/diff/5004/20001
>
> File
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/5004/20001#newcode77
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:77:
> this.version = (Integer)
> (jsonObject.has(Constants.ENCODED_VERSION_PROPERTY)
> Why are you hard coding this requirement that the thing be an integer?
>
>
The version property is currently hard coded to be an Integer. I'm just
using that here.
> http://gwt-code-reviews.appspot.com/959801/diff/5004/20002
>
> File
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/5004/20002#newcode200
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:200:
>
> public void testEndToEndSmartDiff_NoChange_NoVersion() throws Exception
> {
> Why keep the test that tests surviving what would be a client bug?
>
>
> http://gwt-code-reviews.appspot.com/959801/show
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
Oops, nevermind on the "Why keep the test" comment, I was confused. http://gwt-code-reviews.appspot.com/959801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/diff/5004/20001
File
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/5004/20001#newcode77
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:77:
this.version = (Integer)
(jsonObject.has(Constants.ENCODED_VERSION_PROPERTY)
Why are you hard coding this requirement that the thing be an integer?
http://gwt-code-reviews.appspot.com/959801/diff/5004/20002
File
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/5004/20002#newcode200
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:200:
public void testEndToEndSmartDiff_NoChange_NoVersion() throws Exception
{
Why keep the test that tests surviving what would be a client bug?
http://gwt-code-reviews.appspot.com/959801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
On Tue, Oct 5, 2010 at 2:21 PM, wrote:
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
> File
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode78
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:78:
> this.version = givenVersion;
> Not done.
>
Finally done.
>
> http://gwt-code-reviews.appspot.com/959801/diff/12002/13002
>
> File
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/12002/13002#newcode199
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:199:
> public void testEndToEndSmartDiff_NoChange_NoVersion() throws Exception
> {
> If I read this right, you're testing a config that has nothing to do
> with
> reality — the domain object has a version number, but the browser has
> dropped
> it. The interesting test is of a domain object that has no version
> number, or at
> least no version change. Since you have properties that don't tickle the
> version, that shouldn't be hard to write.
>
> Am I confused?
>
>
The test above simulated a case where the domain object has no version
number. But as you note above, there could be a scenario where the domain
object has a version number, but the client version and the server version
is the same. Added another test for that.
>
> http://gwt-code-reviews.appspot.com/959801/show
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
Updated patch incorporates the feedback, including adding the additional test. On Tue, Oct 5, 2010 at 2:14 PM, wrote: > http://gwt-code-reviews.appspot.com/959801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
File
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode78
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:78:
this.version = givenVersion;
Not done.
http://gwt-code-reviews.appspot.com/959801/diff/12002/13002
File
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/12002/13002#newcode199
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:199:
public void testEndToEndSmartDiff_NoChange_NoVersion() throws Exception
{
If I read this right, you're testing a config that has nothing to do
with
reality — the domain object has a version number, but the browser has
dropped
it. The interesting test is of a domain object that has no version
number, or at
least no version change. Since you have properties that don't tickle the
version, that shouldn't be hard to write.
Am I confused?
http://gwt-code-reviews.appspot.com/959801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
Done everything.
On Tue, Oct 5, 2010 at 1:32 PM, wrote:
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
> File
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1381
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1381:
> * send an UPDATE if the client is at a version different than that of
> the
> s/s/S/
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1382
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1382:
> * server or if the server version has changed.
> or if the server version has changed after invoking the domain method.
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1385
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1385:
> assert entityInstanceAfterOperation != null;
> Unnecessary assert.
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1445
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1445:
> SerializedEntity beforeEntity = beforeDataMap.get(entityKey);
> What would a null beforeEntity imply? That the key was just created on
> the server?
Yes, it would imply that the beforeEntity was just created on the server.
>
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5002
> File
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5002#newcode150
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:150:
> public void testEndToEndSmartDiff_NoChange() throws Exception {
> Install eclemma and verify coverage before submitting.
>
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003
> File user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode283
> user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:283: *
> increment version numbers.
> No plans to implement a server version of AutoBeans at present.
>
>
> http://gwt-code-reviews.appspot.com/959801/show
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
On Tue, Oct 5, 2010 at 1:26 PM, wrote:
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
> File
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode78
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:78:
> this.version = givenVersion;
> version = jsonObject.has(Constants.ENCODED_VERSION_PROPERTY) ?
> jsonObject.getString(Constants.ENCODED_VERSION_PROPERTY) : null;
>
> For that matter, do you even need the has() call? Will getString return
> null if it isn't set?
>
If the property is absent, getString(..) returns an JSONException.
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1371
>
> user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1371:
> // handle DELETE
> What are you fixing by getting rid of the write operation check?
>
> The objective was not to get rid of the write operation check, but just to
refactor the code so that the flow is cleaner. After refactoring, a boolean
was all that was necessary.
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5002
> File
>
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5002#newcode150
>
> user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:150:
> public void testEndToEndSmartDiff_NoChange() throws Exception {
> It looks like you're testing the version code path, but no longer
> testing the non-version code path? Until we delete the latter, we should
> maintain its coverage.
>
> Fixed in the followup patch.
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003
> File user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
> (right):
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode200
> user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:200:
> s1.isChanged = false;
> This is pointless, it defaults to false.
>
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode206
> user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:206:
> s2.isChanged = false;
> ditto
>
> Done.
> http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode283
> user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:283: *
> increment version numbers.
> Won't be able to use autobean, this is a JRE class. Wasn't thinking.
>
> Document that it is only set by setUserName and the other one.
>
> Done.
>
> http://gwt-code-reviews.appspot.com/959801/show
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
File
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1381
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1381:
* send an UPDATE if the client is at a version different than that of
the
s/s/S/
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1382
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1382:
* server or if the server version has changed.
or if the server version has changed after invoking the domain method.
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1385
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1385:
assert entityInstanceAfterOperation != null;
Unnecessary assert.
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1445
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1445:
SerializedEntity beforeEntity = beforeDataMap.get(entityKey);
What would a null beforeEntity imply? That the key was just created on
the server?
http://gwt-code-reviews.appspot.com/959801/diff/4001/5002
File
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5002#newcode150
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:150:
public void testEndToEndSmartDiff_NoChange() throws Exception {
Install eclemma and verify coverage before submitting.
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003
File user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode283
user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:283: *
increment version numbers.
No plans to implement a server version of AutoBeans at present.
http://gwt-code-reviews.appspot.com/959801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001
File
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode78
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:78:
this.version = givenVersion;
version = jsonObject.has(Constants.ENCODED_VERSION_PROPERTY) ?
jsonObject.getString(Constants.ENCODED_VERSION_PROPERTY) : null;
For that matter, do you even need the has() call? Will getString return
null if it isn't set?
http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1371
user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1371:
// handle DELETE
What are you fixing by getting rid of the write operation check?
http://gwt-code-reviews.appspot.com/959801/diff/4001/5002
File
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5002#newcode150
user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:150:
public void testEndToEndSmartDiff_NoChange() throws Exception {
It looks like you're testing the version code path, but no longer
testing the non-version code path? Until we delete the latter, we should
maintain its coverage.
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003
File user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
(right):
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode200
user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:200:
s1.isChanged = false;
This is pointless, it defaults to false.
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode206
user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:206:
s2.isChanged = false;
ditto
http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode283
user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:283: *
increment version numbers.
Won't be able to use autobean, this is a JRE class. Wasn't thinking.
Document that it is only set by setUserName and the other one.
http://gwt-code-reviews.appspot.com/959801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
The updated patch incorporates the feedback. On Tue, Oct 5, 2010 at 11:59 AM, wrote: > http://gwt-code-reviews.appspot.com/959801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
http://gwt-code-reviews.appspot.com/959801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Send an update notification to a stale client by comparing the incoming version (issue959801)
Per our offline discussion, you should do the version check before bothering with any diff checks — if the client is stale, we know it needs to update the entity regardless of what happens when the method fires. It's tempting to rip out the diff mechanism entirely, but this is not the the time. On Tue, Oct 5, 2010 at 10:11 AM, wrote: > Reviewers: rjrjr, robertvawter, > > Description: > Send an update notification to a stale client by comparing the incoming > version > number, with the version number on the server. This code path will only > be > taken if the ORM system has not flagged any errors. > > Hacked the test harness so that the version number is only incremented > on a > real change. Will fix the test harness shortly in a follow-up patch. > > Patch by: amitmanjhi > Review by: rjrjr, robertvawter > > > Please review this at http://gwt-code-reviews.appspot.com/959801/show > > Affected files: > M user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java > M > user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java > M user/test/com/google/gwt/requestfactory/server/SimpleFoo.java > > > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
