tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hls...@apache.org
Subject [2/2] git commit: TAP5-2269: Client persistence strategy does not detect changes to internal state of mutable objects
Date Mon, 01 Sep 2014 18:30:00 GMT
TAP5-2269: Client persistence strategy does not detect changes to internal state of mutable
objects


Project: http://git-wip-us.apache.org/repos/asf/tapestry-5/repo
Commit: http://git-wip-us.apache.org/repos/asf/tapestry-5/commit/021b23e1
Tree: http://git-wip-us.apache.org/repos/asf/tapestry-5/tree/021b23e1
Diff: http://git-wip-us.apache.org/repos/asf/tapestry-5/diff/021b23e1

Branch: refs/heads/master
Commit: 021b23e18c1b67b0a932d19dc0080b82a82e4902
Parents: 57ced86
Author: Howard M. Lewis Ship <hlship@apache.org>
Authored: Mon Sep 1 11:30:07 2014 -0700
Committer: Howard M. Lewis Ship <hlship@apache.org>
Committed: Mon Sep 1 11:30:07 2014 -0700

----------------------------------------------------------------------
 54_RELEASE_NOTES.md                             |   7 +
 .../ClientPersistentFieldStorageImpl.java       |  44 ++++---
 .../ClientPersistentFieldStorageImplTest.java   | 128 +++++++++++++++----
 3 files changed, 133 insertions(+), 46 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/021b23e1/54_RELEASE_NOTES.md
----------------------------------------------------------------------
diff --git a/54_RELEASE_NOTES.md b/54_RELEASE_NOTES.md
index 93d0646..19a598e 100644
--- a/54_RELEASE_NOTES.md
+++ b/54_RELEASE_NOTES.md
@@ -448,3 +448,10 @@ The automatic ValueEncoder from String to any Number type, or ti Boolean
have
 changed slightly. An empty input string is encoded to a null rather than being passed through
 the type coercer. This reflects the desire that a submitted value (in a URL or a form) that
is blank
 is the same as no value: null.
+
+## Client Persistent Fields
+
+Tapestry now determines if a client-persistent field is mutable and contains changes, using
the same
+checks as for session-persistent objects (the OptimizedSessionPersistedObject interface and
so forth).
+Previously, Tapestry would load a mutable object into a field, but a change to the internal
state
+of that mutable field would not always be preserved in the serialized object data attached
to links.

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/021b23e1/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImpl.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImpl.java
b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImpl.java
index 2d95767..a94ac4f 100644
--- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImpl.java
+++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImpl.java
@@ -1,5 +1,3 @@
-// Copyright 2007, 2008, 2009, 2011 The Apache Software Foundation
-//
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -19,10 +17,7 @@ import org.apache.tapestry5.ioc.ScopeConstants;
 import org.apache.tapestry5.ioc.annotations.Scope;
 import org.apache.tapestry5.ioc.internal.util.CollectionFactory;
 import org.apache.tapestry5.ioc.internal.util.InternalUtils;
-import org.apache.tapestry5.services.ClientDataEncoder;
-import org.apache.tapestry5.services.ClientDataSink;
-import org.apache.tapestry5.services.PersistentFieldChange;
-import org.apache.tapestry5.services.Request;
+import org.apache.tapestry5.services.*;
 
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
@@ -66,7 +61,7 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
         public PersistentFieldChange toChange(Object value)
         {
             return new PersistentFieldChangeImpl(componentId == null ? "" : componentId,
-                                                 fieldName, value);
+                    fieldName, value);
         }
 
         @Override
@@ -102,8 +97,7 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
             if (componentId == null)
             {
                 if (other.componentId != null) return false;
-            }
-            else if (!componentId.equals(other.componentId)) return false;
+            } else if (!componentId.equals(other.componentId)) return false;
 
             return true;
         }
@@ -111,15 +105,18 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
 
     private final ClientDataEncoder clientDataEncoder;
 
+    private final SessionPersistedObjectAnalyzer analyzer;
+
     private final Map<Key, Object> persistedValues = CollectionFactory.newMap();
 
     private String clientData;
 
     private boolean mapUptoDate = false;
 
-    public ClientPersistentFieldStorageImpl(Request request, ClientDataEncoder clientDataEncoder)
+    public ClientPersistentFieldStorageImpl(Request request, ClientDataEncoder clientDataEncoder,
SessionPersistedObjectAnalyzer analyzer)
     {
         this.clientDataEncoder = clientDataEncoder;
+        this.analyzer = analyzer;
 
         // This, here, is the problem of TAPESTRY-2501; this call can predate
         // the check to set the character set based on meta data of the page.
@@ -233,12 +230,10 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
 
                 persistedValues.put(key, value);
             }
-        }
-        catch (Exception ex)
+        } catch (Exception ex)
         {
             throw new RuntimeException("Serialized client state was corrupted. This may indicate
that too much state is being stored, which can cause the encoded string to be truncated by
the client web browser.", ex);
-        }
-        finally
+        } finally
         {
             InternalUtils.close(in);
         }
@@ -246,6 +241,21 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
 
     private void refreshClientData()
     {
+        // TAP5-2269: Even in the absense of a change to a persistent field, a mutable persistent
object
+        // may have changed.
+
+        if (clientData != null)
+        {
+            for (Object value : persistedValues.values())
+            {
+                if (analyzer.checkAndResetDirtyState(value))
+                {
+                    clientData = null;
+                    break;
+                }
+            }
+        }
+
         // Client data will be null after a change to the map, or if there was no client
data in the
         // request. In any other case where the client data is non-null, it is by definition
         // up-to date (since it is reset to null any time there's a change to the map).
@@ -276,12 +286,10 @@ public class ClientPersistentFieldStorageImpl implements ClientPersistentFieldSt
                 os.writeObject(e.getKey());
                 os.writeObject(e.getValue());
             }
-        }
-        catch (Exception ex)
+        } catch (Exception ex)
         {
             throw new RuntimeException(ex.getMessage(), ex);
-        }
-        finally
+        } finally
         {
             InternalUtils.close(os);
         }

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/021b23e1/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImplTest.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImplTest.java
b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImplTest.java
index 0b24b38..3cd0ef9 100644
--- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImplTest.java
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientPersistentFieldStorageImplTest.java
@@ -1,5 +1,3 @@
-// Copyright 2007-2013 The Apache Software Foundation
-//
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -17,14 +15,19 @@ package org.apache.tapestry5.internal.services;
 import org.apache.tapestry5.Link;
 import org.apache.tapestry5.internal.test.InternalBaseTestCase;
 import org.apache.tapestry5.internal.util.Holder;
+import org.apache.tapestry5.ioc.internal.util.CollectionFactory;
 import org.apache.tapestry5.services.ClientDataEncoder;
 import org.apache.tapestry5.services.PersistentFieldChange;
 import org.apache.tapestry5.services.Request;
+import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer;
 import org.easymock.EasyMock;
 import org.easymock.IAnswer;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 import static org.apache.tapestry5.ioc.internal.util.CollectionFactory.newList;
@@ -35,10 +38,13 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 {
     private ClientDataEncoder clientDataEncoder;
 
+    private SessionPersistedObjectAnalyzer analyzer;
+
     @BeforeClass
     public void setup()
     {
         clientDataEncoder = getService(ClientDataEncoder.class);
+        analyzer = getService(SessionPersistedObjectAnalyzer.class);
     }
 
     @Test
@@ -49,7 +55,7 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         // Should do nothing.
 
@@ -58,21 +64,10 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
         verify();
     }
 
-    @SuppressWarnings("unchecked")
-    @Test
-    public void store_and_restore_a_change()
+    private Holder<String> captureLinkModification(Link link)
     {
-        Request request = mockRequest(null);
-        Link link = mockLink();
         final Holder<String> holder = Holder.create();
 
-        String pageName = "Foo";
-        String componentId = "bar.baz";
-        String fieldName = "biff";
-        Object value = 99;
-
-        // Use an IAnswer to capture the value.
-
         link.addParameter(eq(ClientPersistentFieldStorageImpl.PARAMETER_NAME), isA(String.class));
         setAnswer(new IAnswer<Void>()
         {
@@ -86,9 +81,26 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
             }
         });
 
+        return holder;
+    }
+
+    @SuppressWarnings("unchecked")
+    @Test
+    public void store_and_restore_a_change()
+    {
+        Request request = mockRequest(null);
+        Link link = mockLink();
+
+        String pageName = "Foo";
+        String componentId = "bar.baz";
+        String fieldName = "biff";
+        Object value = 99;
+
+        final Holder<String> holder = captureLinkModification(link);
+
         replay();
 
-        ClientPersistentFieldStorage storage1 = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage1 = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         storage1.postChange(pageName, componentId, fieldName, value);
 
@@ -98,8 +110,6 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         verify();
 
-        System.out.println(holder.get());
-
         assertEquals(changes1.size(), 1);
         PersistentFieldChange change1 = changes1.get(0);
 
@@ -113,7 +123,7 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage2 = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage2 = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         List<PersistentFieldChange> changes2 = newList(storage2.gatherFieldChanges(pageName));
 
@@ -142,7 +152,7 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         for (int k = 0; k < 3; k++)
         {
@@ -174,7 +184,7 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         storage.postChange(pageName, componentId, fieldName, 99);
         storage.postChange(pageName, componentId, fieldName, null);
@@ -201,7 +211,7 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         storage.postChange(pageName, componentId, fieldName, 99);
 
@@ -231,14 +241,13 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         try
         {
             storage.postChange("Foo", "bar.baz", "woops", badBody);
             unreachable();
-        }
-        catch (IllegalArgumentException ex)
+        } catch (IllegalArgumentException ex)
         {
             assertEquals(
                     ex.getMessage(),
@@ -248,6 +257,70 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
         verify();
     }
 
+    public static class MutableObject implements Serializable
+    {
+        public String mutableValue;
+    }
+
+    // So if an object is stored in a persistent field and is mutable, then the field should
not have to be modified
+    // to force a change, instead the value should be checked to see if it is dirty (assuming
true), and reserialized.
+    @Test
+    public void modified_mutable_objects_are_reserialized()
+    {
+        Request request = mockRequest(null);
+        Link link = mockLink();
+
+        MutableObject mo = new MutableObject();
+
+        mo.mutableValue = "initial state";
+
+        String pageName = "Foo";
+        String componentId = "bar.baz";
+        String fieldName = "biff";
+
+        final Holder<String> holder1 = captureLinkModification(link);
+
+        final Holder<String> holder2 = captureLinkModification(link);
+
+        replay();
+
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
+
+        storage.postChange(pageName, componentId, fieldName, mo);
+
+        storage.updateLink(link);
+
+        mo.mutableValue = "modified state";
+
+        storage.updateLink(link);
+
+        verify();
+
+        System.out.printf("holder1=%s%nholder2=%s%n", holder1.get(), holder2.get());
+
+        assertNotEquals(holder1.get(), holder2.get(), "encoded client data should be different");
+
+        // Now check that it de-serializes to the correct data.
+
+        request = mockRequest(holder2.get());
+
+        replay();
+
+        storage = new ClientPersistentFieldStorageImpl(request, clientDataEncoder, analyzer);
+
+        Collection<PersistentFieldChange> changes = storage.gatherFieldChanges(pageName);
+
+        assertEquals(changes.size(), 1);
+
+        PersistentFieldChange change = new ArrayList<PersistentFieldChange>(changes).get(0);
+
+        MutableObject mo2 = (MutableObject) change.getValue();
+
+        assertEquals(mo2.mutableValue, "modified state");
+
+        verify();
+    }
+
     @Test
     public void corrupt_client_data()
     {
@@ -256,14 +329,13 @@ public class ClientPersistentFieldStorageImplTest extends InternalBaseTestCase
 
         replay();
 
-        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder);
+        ClientPersistentFieldStorage storage = new ClientPersistentFieldStorageImpl(request,
clientDataEncoder, analyzer);
 
         try
         {
             storage.postChange("Foo", "bar.baz", "woops", 99);
             unreachable();
-        }
-        catch (RuntimeException ex)
+        } catch (RuntimeException ex)
         {
             assertEquals(ex.getMessage(), "Serialized client state was corrupted. This may
indicate that too much state is being stored, which can cause the encoded string to be truncated
by the client web browser.");
             assertNotNull(ex.getCause());


Mime
View raw message