You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@myfaces.apache.org by ma...@apache.org on 2009/01/30 15:15:08 UTC
svn commit: r739284 - in /myfaces/trinidad/trunk_1.2.x:
trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/
trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/
Author: matzew
Date: Fri Jan 30 14:15:07 2009
New Revision: 739284
URL: http://svn.apache.org/viewvc?rev=739284&view=rev
Log:
TRINIDAD-1356 - Application of changes by the ChangeManager is extremely slow
TRINIDAD-1375 - Increase strength of viewState token
thanks to Blake Sullivan for the patches
Modified:
myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/MoveChildComponentChange.java
myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/SessionChangeManager.java
myfaces/trinidad/trunk_1.2.x/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/TokenCache.java
Modified: myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/MoveChildComponentChange.java
URL: http://svn.apache.org/viewvc/myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/MoveChildComponentChange.java?rev=739284&r1=739283&r2=739284&view=diff
==============================================================================
--- myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/MoveChildComponentChange.java (original)
+++ myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/MoveChildComponentChange.java Fri Jan 30 14:15:07 2009
@@ -1,5 +1,6 @@
package org.apache.myfaces.trinidad.change;
+import javax.faces.component.NamingContainer;
import javax.faces.component.UIComponent;
import javax.faces.context.FacesContext;
@@ -92,6 +93,7 @@
_commonParent);
_destinationContainerScopedId =
ComponentUtils.getScopedIdForComponent(destinationContainer, _commonParent);
+
_commonParentScopedId =
ComponentUtils.getScopedIdForComponent(_commonParent, null);
@@ -102,11 +104,58 @@
throw new IllegalArgumentException(
_LOG.getMessage("MOVE_PARTICIPANTS_WITHOUT_ID"));
+ // calculate the absolute scoped ids for the source and destination so that we can
+ // handle remapping scoped ids in the SessionChangeManager
+ String commonParentPrefix = _getScopedIdPrefix(_commonParent, _commonParentScopedId);
+
+ _sourceAbsoluteScopedId = (commonParentPrefix != null)
+ ? new StringBuilder(commonParentPrefix).
+ append(NamingContainer.SEPARATOR_CHAR).
+ append(_movableChildScopedId).toString()
+ : _movableChildScopedId;
+
+ // calculate the absolute scoped id of the destination
+ String destinationContainerPrefix = _getScopedIdPrefix(destinationContainer,
+ _destinationContainerScopedId);
+
+ StringBuilder destinationScopedIdBuilder = new StringBuilder();
+
+ if (commonParentPrefix != null)
+ {
+ destinationScopedIdBuilder.append(commonParentPrefix).append(NamingContainer.SEPARATOR_CHAR);
+ }
+
+ if (destinationContainerPrefix != null)
+ {
+ destinationScopedIdBuilder.append(destinationContainerPrefix).append(NamingContainer.SEPARATOR_CHAR);
+ }
+
+ _destinationAbsoluteScopedId = destinationScopedIdBuilder.append(movableChild.getId()).toString();
+
// For insertBeforeComponent, we do not care to obtain scoped id.
_insertBeforeId = (insertBeforeComponent == null) ?
null:insertBeforeComponent.getId();
}
+ private String _getScopedIdPrefix(UIComponent component, String scopedId)
+ {
+ if (component instanceof NamingContainer)
+ return scopedId;
+ else
+ {
+ // remove the component's id from the end
+ int separatorIndex = scopedId.lastIndexOf(NamingContainer.SEPARATOR_CHAR);
+
+ if (separatorIndex >= 0)
+ return scopedId.substring(0, separatorIndex);
+ else
+ {
+ // component was at top level
+ return null;
+ }
+ }
+ }
+
/**
* Convenience method to add this MoveChildComponentChange to the supplied
* ChangeManager. The change will be registered against a parent component
@@ -388,6 +437,23 @@
}
/**
+ * Returns the absolute scopedId of the source component
+ */
+ public String getSourceScopedId()
+ {
+ return _sourceAbsoluteScopedId;
+ }
+
+
+ /**
+ * Returns the absolute scopedId of the source component at its destination
+ */
+ public String getDestinationScopedId()
+ {
+ return _destinationAbsoluteScopedId;
+ }
+
+ /**
* Returns the depth of a UIComponent in the tree.
* @param comp the UIComponent whose depth has to be calculated
* @return the depth of the passed in UIComponent
@@ -427,7 +493,8 @@
private final String _destinationContainerScopedId;
private final String _commonParentScopedId;
private final String _insertBeforeId;
-
+ private final String _sourceAbsoluteScopedId;
+ private final String _destinationAbsoluteScopedId;
private static final long serialVersionUID = 1L;
private static final TrinidadLogger _LOG = TrinidadLogger.createTrinidadLogger(
Modified: myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/SessionChangeManager.java
URL: http://svn.apache.org/viewvc/myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/SessionChangeManager.java?rev=739284&r1=739283&r2=739284&view=diff
==============================================================================
--- myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/SessionChangeManager.java (original)
+++ myfaces/trinidad/trunk_1.2.x/trinidad-api/src/main/java/org/apache/myfaces/trinidad/change/SessionChangeManager.java Fri Jan 30 14:15:07 2009
@@ -20,10 +20,15 @@
import java.io.Serializable;
+import java.util.ArrayList;
import java.util.Collections;
+import java.util.Iterator;
import java.util.List;
+import java.util.ListIterator;
import java.util.Map;
+import java.util.Queue;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.CopyOnWriteArrayList;
import javax.faces.component.UIComponent;
@@ -31,6 +36,7 @@
import javax.faces.context.FacesContext;
import org.apache.myfaces.trinidad.logging.TrinidadLogger;
+import org.apache.myfaces.trinidad.util.CollectionUtils;
import org.apache.myfaces.trinidad.util.ComponentUtils;
import org.w3c.dom.Document;
@@ -53,13 +59,15 @@
{
UIViewRoot viewRoot = facesContext.getViewRoot();
- List<QualifiedComponentChange> componentChangesForView =
- _getComponentChangesForView(facesContext, viewRoot.getViewId(), false);
+ // retrieve the ComponentChanges for this current viewid
+ ChangesForView changesForView = _getChangesForView(facesContext, viewRoot.getViewId(), false);
- for (QualifiedComponentChange qualifiedChange : componentChangesForView)
+ // loop through the viewId's changes, applying the changes
+ for (QualifiedComponentChange qualifiedChange : changesForView.getComponentChangesForView())
{
UIComponent targetComponent =
viewRoot.findComponent(qualifiedChange.getTargetComponentScopedId());
+
// Possible that the target component no more exists in the view
if (targetComponent != null)
{
@@ -92,15 +100,90 @@
{
String viewId = facesContext.getViewRoot().getViewId();
- List<QualifiedComponentChange> componentChangesForView =
- _getComponentChangesForView(facesContext, viewId, true);
+ // get the ComponentChanges for the current viewId
+ ChangesForView changesForView = _getChangesForView(facesContext, viewId, true);
+ // get the absolute scopedId for the target component so that we have a unique identifier
+ // to compare
String scopedIdForTargetComponent =
- ComponentUtils.getScopedIdForComponent(targetComponent, null);
+ ComponentUtils.getScopedIdForComponent(targetComponent, null);
- componentChangesForView.add(
- new QualifiedComponentChange(scopedIdForTargetComponent,
- componentChange));
+ // try to collapse AttributeComponentChanges, handling component movement so that
+ // we can collapse any attribute change on the same component
+ if (componentChange instanceof AttributeComponentChange)
+ {
+ AttributeComponentChange attributeChange = (AttributeComponentChange)componentChange;
+ String attributeName = attributeChange.getAttributeName();
+
+ // would really rather use a Deque here and iterate backwards, which would also make
+ // handling the rename changes easier
+ Iterator<QualifiedComponentChange> changes =
+ changesForView.getComponentChangesForView().iterator();
+
+ // list of changes that have renamed the scoped id of this component. We need to
+ // handle this aliasing when traversing through the changes looking for matches
+ Iterator<MoveChildComponentChange> renameChanges =
+ changesForView.getRenameChanges(scopedIdForTargetComponent);
+
+ // we need to look through the rename list to map from the current names to
+ // the new names
+ MoveChildComponentChange nextRenameChange;
+ String currTargetScopedId;
+
+ if (renameChanges.hasNext())
+ {
+ // we have at least one rename change, so get it and find the name that this
+ // component was originally known by
+ nextRenameChange = renameChanges.next();
+ currTargetScopedId = nextRenameChange.getSourceScopedId();
+ }
+ else
+ {
+ nextRenameChange = null;
+ currTargetScopedId = scopedIdForTargetComponent;
+ }
+
+ // loop forward through the changes looking for AttributeChanges to collapse
+ while (changes.hasNext())
+ {
+ QualifiedComponentChange currQualifiedChange = changes.next();
+
+ if (currQualifiedChange.getComponentChange() == nextRenameChange)
+ {
+ // we got a match, so update the scoped id we should be looking for
+ currTargetScopedId = nextRenameChange.getDestinationScopedId();
+
+ nextRenameChange = (renameChanges.hasNext())
+ ? renameChanges.next()
+ : null;
+ }
+ else if (currQualifiedChange.getTargetComponentScopedId().equals(currTargetScopedId))
+ {
+ // found a change on the same component. Check if it's an AttributeChange
+ ComponentChange currChange = currQualifiedChange.getComponentChange();
+
+ if (currChange instanceof AttributeComponentChange)
+ {
+ AttributeComponentChange currAttributeChange = (AttributeComponentChange)currChange;
+
+ // Check if the AttributeChange is for the same attribute
+ if (attributeName.equals(currAttributeChange.getAttributeName()))
+ {
+ // the old AttributeChange is for the same attribute, so remove it since the
+ // new AttributeChange would eclipse it anyway.
+ changes.remove();
+ break;
+ }
+ }
+ }
+ }
+ }
+
+ QualifiedComponentChange newQualifiedChange = new QualifiedComponentChange(
+ scopedIdForTargetComponent,
+ componentChange);
+
+ changesForView.addChange(newQualifiedChange);
}
/**
@@ -118,11 +201,12 @@
* @param viewId The id of the view for which changes are required.
* @param createIfNecessary Indicates whether the underlying datastructures
* that store the component changes needs to be created if absent.
- * @return The in-order list of component changes for the supplied view. This
+ * @return The ChangesForView object containing information about the changes for the specified
+ * viewId, including in-order list of component changes for the supplied view. This
* will be in the same order in which the component changes were added through
* calls to <code>addComponentChange()</code>.
*/
- private List<QualifiedComponentChange> _getComponentChangesForView(
+ private ChangesForView _getChangesForView(
FacesContext facesContext,
String viewId,
boolean createIfNecessary)
@@ -130,25 +214,22 @@
Object session = facesContext.getExternalContext().getSession(true);
// Key is view id and value is list of component changes for that view
- ConcurrentHashMap<String, List<QualifiedComponentChange>>
- componentChangesMapForSession;
+ ConcurrentHashMap<String, ChangesForView> componentChangesMapForSession;
synchronized(session)
{
Map<String, Object> sessMap =
facesContext.getExternalContext().getSessionMap();
- componentChangesMapForSession =
- (ConcurrentHashMap<String, List<QualifiedComponentChange>>)
- (sessMap.get(_COMPONENT_CHANGES_MAP_FOR_SESSION_KEY));
+ componentChangesMapForSession = (ConcurrentHashMap<String, ChangesForView>)
+ sessMap.get(_COMPONENT_CHANGES_MAP_FOR_SESSION_KEY);
if (componentChangesMapForSession == null)
{
if (!createIfNecessary)
- return Collections.emptyList();
+ return _EMPTY_CHANGES;
- componentChangesMapForSession =
- new ConcurrentHashMap<String, List<QualifiedComponentChange>>();
+ componentChangesMapForSession = new ConcurrentHashMap<String, ChangesForView>();
sessMap.put(_COMPONENT_CHANGES_MAP_FOR_SESSION_KEY,
componentChangesMapForSession);
}
@@ -157,19 +238,124 @@
if (!componentChangesMapForSession.containsKey(viewId))
{
if (!createIfNecessary)
- return Collections.emptyList();
+ return _EMPTY_CHANGES;
- // Writes are per change addition, not very frequent, using
- // CopyOnWriteArrayList should suffice.
- componentChangesMapForSession.putIfAbsent(
- viewId,
- new CopyOnWriteArrayList<QualifiedComponentChange>());
+ componentChangesMapForSession.putIfAbsent(viewId, new ChangesForView(true));
}
return componentChangesMapForSession.get(viewId);
}
- private static final class QualifiedComponentChange implements Serializable
+ /**
+ * Tracks the component changes for a particular view as well as all the movement
+ * changes so that component aliasing can be tracked
+ */
+ private static final class ChangesForView implements Serializable
+ {
+ protected ChangesForView(boolean rw)
+ {
+ if (rw)
+ {
+ _componentChangesForView = new ConcurrentLinkedQueue<QualifiedComponentChange>();
+ _renameChanges = new CopyOnWriteArrayList<MoveChildComponentChange>();
+ }
+ else
+ {
+ _componentChangesForView = CollectionUtils.emptyQueue();
+ _renameChanges = null;
+ }
+ }
+
+ /**
+ * Returns the QualifiedComponentChanges for this viewId
+ */
+ protected Iterable<QualifiedComponentChange> getComponentChangesForView()
+ {
+ return _componentChangesForView;
+ }
+
+ /**
+ * Adds a change to the QualifiedComponentChanges for this viewId, handling
+ * MoveChildComponentChanges specially to handle cases where the clientId
+ * of a component changes as a result of a rename operation
+ */
+ protected void addChange(QualifiedComponentChange qualifiedChange)
+ {
+ _componentChangesForView.add(qualifiedChange);
+
+ ComponentChange componentChange = qualifiedChange.getComponentChange();
+
+ if (componentChange instanceof MoveChildComponentChange)
+ {
+ // we only need to remove moves that actually changed the absolute scoped id of the
+ // component
+ MoveChildComponentChange moveComponentChange = (MoveChildComponentChange)componentChange;
+
+ if (!moveComponentChange.getSourceScopedId().equals(moveComponentChange.getDestinationScopedId()))
+ {
+ _renameChanges.add(moveComponentChange);
+ }
+ }
+ }
+
+ /**
+ * Returns the Iterator of rename changes that affect the current scoped id in ComponentChange order
+ * @return
+ */
+ protected Iterator<MoveChildComponentChange> getRenameChanges(String targetScopedId)
+ {
+ if (_renameChanges != null)
+ {
+ String currTargetScopedId = targetScopedId;
+ List renameChanges = null;
+
+ // iterate from the back of the List determining the MoveChildComponentChange
+ // that are aliased to this scoped id
+ ListIterator<MoveChildComponentChange> moveChanges =
+ _renameChanges.listIterator(_renameChanges.size());
+
+ while (moveChanges.hasPrevious())
+ {
+ MoveChildComponentChange currMoveChange = moveChanges.previous();
+
+ if (currTargetScopedId.equals(currMoveChange.getDestinationScopedId()))
+ {
+ // lazily create the list the first time we need it
+ if (renameChanges == null)
+ renameChanges = new ArrayList<MoveChildComponentChange>();
+
+ renameChanges.add(currMoveChange);
+
+ // get the new id to search for
+ currTargetScopedId = currMoveChange.getSourceScopedId();
+ }
+ }
+
+ if (renameChanges != null)
+ {
+ if (renameChanges.size() > 1)
+ {
+ // reverse the list to match the order that we will see these items when traversing
+ // the changes from the forward direction
+ Collections.reverse(renameChanges);
+ }
+
+ return renameChanges.iterator();
+ }
+ }
+
+ return CollectionUtils.emptyIterator();
+ }
+
+ private final Queue<QualifiedComponentChange> _componentChangesForView;
+ private final List<MoveChildComponentChange> _renameChanges;
+
+ private static final long serialVersionUID = 1L;
+ }
+
+ private static final ChangesForView _EMPTY_CHANGES = new ChangesForView(false);
+
+ private static class QualifiedComponentChange implements Serializable
{
public QualifiedComponentChange(String targetComponentScopedId,
ComponentChange componentChange)
@@ -198,7 +384,7 @@
private static final long serialVersionUID = 1L;
}
-
+
private static final String _COMPONENT_CHANGES_MAP_FOR_SESSION_KEY =
"org.apache.myfaces.trinidadinternal.ComponentChangesMapForSession";
Modified: myfaces/trinidad/trunk_1.2.x/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/TokenCache.java
URL: http://svn.apache.org/viewvc/myfaces/trinidad/trunk_1.2.x/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/TokenCache.java?rev=739284&r1=739283&r2=739284&view=diff
==============================================================================
--- myfaces/trinidad/trunk_1.2.x/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/TokenCache.java (original)
+++ myfaces/trinidad/trunk_1.2.x/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/util/TokenCache.java Fri Jan 30 14:15:07 2009
@@ -18,19 +18,22 @@
*/
package org.apache.myfaces.trinidadinternal.util;
-import java.io.IOException;
-import java.io.ObjectInputStream;
import java.io.Serializable;
+import java.math.BigInteger;
+
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+
import javax.faces.context.ExternalContext;
import javax.faces.context.FacesContext;
-import javax.servlet.http.HttpSession;
-
import org.apache.myfaces.trinidad.logging.TrinidadLogger;
@@ -80,19 +83,8 @@
cache = (TokenCache) external.getSessionMap().get(cacheName);
if ((cache == null) && createIfNeeded)
{
- // Seed the token cache with the session ID (if available)
- int seed = 0;
- if (_USE_SESSION_TO_SEED_ID)
- {
- if (session instanceof HttpSession)
- {
- String id = ((HttpSession) session).getId();
- if (id != null)
- seed = id.hashCode();
- }
- }
-
- cache = new TokenCache(defaultSize, seed);
+ // create the TokenCache with the crytographically random seed
+ cache = new TokenCache(defaultSize, _getSeed());
external.getSessionMap().put(cacheName, cache);
}
@@ -100,6 +92,33 @@
return cache;
}
+
+ /**
+ * Returns a cryptographically secure random number to use as the TokenCache seed
+ */
+ private static long _getSeed()
+ {
+ SecureRandom rng;
+
+ try
+ {
+ // try SHA1 first
+ rng = SecureRandom.getInstance("SHA1PRNG");
+ }
+ catch (NoSuchAlgorithmException e)
+ {
+ // SHA1 not present, so try the default (which could potentially not be
+ // cryptographically secure)
+ rng = new SecureRandom();
+ }
+
+ // use 48 bits for strength and fill them in
+ byte[] randomBytes = new byte[6];
+ rng.nextBytes(randomBytes);
+
+ // convert to a long
+ return new BigInteger(randomBytes).longValue();
+ }
/**
@@ -107,28 +126,42 @@
*/
public TokenCache()
{
- this(_DEFAULT_SIZE, 0);
+ this(_DEFAULT_SIZE, 0L);
}
/**
- * Create a TokenCache that will store the last "size" entries.
+ * Create a TokenCache that will store the last "size" entries. This version should
+ * not be used if the token cache is externally accessible (since the seed always
+ * starts at 0). Use the
+ * constructor with the long seed instead.
*/
public TokenCache(int size)
{
- this(size, 0);
+ this(size, 0L);
}
/**
* Create a TokenCache that will store the last "size" entries,
* and begins its tokens based on the seed (instead of always
* starting at "0").
+ * @Deprecated Use version using a long size instead for greater security
*/
public TokenCache(int size, int seed)
{
+ this(size, (long)seed);
+ }
+
+ /**
+ * Create a TokenCache that will store the last "size" entries,
+ * and begins its tokens based on the seed (instead of always
+ * starting at "0").
+ */
+ public TokenCache(int size, long seed)
+ {
_cache = new LRU(size);
_pinned = new ConcurrentHashMap<String, String>(size);
- _count = seed;
+ _count = new AtomicLong(seed);
}
/**
@@ -285,19 +318,11 @@
private String _getNextToken()
{
- synchronized (_lock)
- {
- _count = _count + 1;
- return Integer.toString(_count, 16);
- }
- }
-
- private void readObject(ObjectInputStream in)
- throws ClassNotFoundException, IOException
- {
- in.defaultReadObject();
- // Re-create the lock, which was transient
- _lock = new Object();
+ // atomically increment the value
+ long nextToken = _count.incrementAndGet();
+
+ // convert using base 36 because it is a fast efficient subset of base-64
+ return Long.toString(nextToken, 36);
}
private class LRU extends LRUCache<String, String>
@@ -322,15 +347,14 @@
// stored, and the values are the tokens that are pinned. This is
// an N->1 ratio: the values may appear multiple times.
private final Map<String, String> _pinned;
- private int _count;
- private transient Object _lock = new Object();
+
+ // the current token value
+ private final AtomicLong _count;
// Hack instance parameter used to communicate between the LRU cache's
// removing() method, and the addNewEntry() method that may trigger it
private transient String _removed;
- static private final boolean _USE_SESSION_TO_SEED_ID = true;
-
static private final int _DEFAULT_SIZE = 15;
static private final long serialVersionUID = 1L;
static private final TrinidadLogger _LOG =