Author: chris.laprun(a)jboss.com Date: 2010-02-13 06:18:05 -0500 (Sat, 13 Feb 2010) New Revision: 1669 Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationProperty.java components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationInfoTestCase.java components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationPropertyTestCase.java Log: - Moved isRegistered to RegistrationInfo. - Only modifyRegistration if we need to. - Fixed propertyValueChanged so that it doesn't trigger a need to modify the registration if the property that has changed didn't exist or was unchecked. - Added and fixed test cases. Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java 2010-02-13 03:41:18 UTC (rev 1668) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/ProducerInfo.java 2010-02-13 11:18:05 UTC (rev 1669) @@ -241,15 +241,7 @@ public boolean isRegistered() { - Boolean valid = persistentRegistrationInfo.isRegistrationValid(); - if (valid == null) - { - return persistentRegistrationInfo.getRegistrationHandle() != null; - } - else - { - return valid; - } + return persistentRegistrationInfo.isRegistered(); } public boolean isRegistrationRequired() @@ -1074,40 +1066,43 @@ { persistentEndpointInfo.refresh(); - try + if (isModifyRegistrationRequired()) { - RegistrationContext registrationContext = getRegistrationContext(); - Holder<byte[]> registrationState = new Holder<byte[]>(); + try + { + RegistrationContext registrationContext = getRegistrationContext(); + Holder<byte[]> registrationState = new Holder<byte[]>(); - // invocation - persistentEndpointInfo.getRegistrationService().modifyRegistration( - registrationContext, - persistentRegistrationInfo.getRegistrationData(), - registrationState, - new Holder<List<Extension>>()); + // invocation + persistentEndpointInfo.getRegistrationService().modifyRegistration( + registrationContext, + persistentRegistrationInfo.getRegistrationData(), + registrationState, + new Holder<List<Extension>>()); - // force refresh of internal RegistrationInfo state - persistentRegistrationInfo.setRegistrationValidInternalState(); + // force refresh of internal RegistrationInfo state + persistentRegistrationInfo.setRegistrationValidInternalState(); - // registration is not modified anymore :) - setModifyRegistrationRequired(false); + // registration is not modified anymore :) + setModifyRegistrationRequired(false); - // update state - persistentRegistrationInfo.setRegistrationState(registrationState.value); + // update state + persistentRegistrationInfo.setRegistrationState(registrationState.value); - log.info("Consumer with id '" + persistentId + "' sucessfully modified its registration."); + log.info("Consumer with id '" + persistentId + "' sucessfully modified its registration."); - // reset cache to be able to see new offered portlets on the next refresh - invalidateCache(); + // reset cache to be able to see new offered portlets on the next refresh + invalidateCache(); + } + catch (Exception e) + { + throw new PortletInvokerException("Couldn't modify registration with producer '" + persistentId + "'", e); + } + finally + { + registry.updateProducerInfo(this); + } } - catch (Exception e) - { - throw new PortletInvokerException("Couldn't modify registration with producer '" + persistentId + "'", e); - } - finally - { - registry.updateProducerInfo(this); - } } else { Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java 2010-02-13 03:41:18 UTC (rev 1668) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationInfo.java 2010-02-13 11:18:05 UTC (rev 1669) @@ -316,8 +316,6 @@ // todo: deal with language more appropriately prop = new RegistrationProperty(name, value, WSRPUtils.toString(Locale.getDefault()), this); getOrCreateRegistrationPropertiesMap(false).put(name, prop); - setModifiedSinceLastRefresh(true); - setModifyRegistrationNeeded(true); } return prop; @@ -689,7 +687,7 @@ public boolean isModifyRegistrationNeeded() { - return modifyRegistrationNeeded || isModifiedSinceLastRefresh(); + return modifyRegistrationNeeded; } public boolean isModifiedSinceLastRefresh() @@ -702,11 +700,14 @@ this.modifiedSinceLastRefresh = modifiedSinceLastRefresh; } - /** todo: Should be package-only, public for tests... */ - public void propertyValueChanged(RegistrationProperty property, Object oldValue, Object newValue) + public void propertyValueChanged(RegistrationProperty property, RegistrationProperty.Status previousStatus, Object oldValue, Object newValue) { setModifiedSinceLastRefresh(true); - setModifyRegistrationNeeded(true); + + if (previousStatus != null && !RegistrationProperty.Status.MISSING_VALUE.equals(previousStatus) && !RegistrationProperty.Status.UNCHECKED_VALUE.equals(previousStatus)) + { + setModifyRegistrationNeeded(true); + } } private void setModifyRegistrationNeeded(boolean modifyRegistrationNeeded) @@ -714,6 +715,19 @@ this.modifyRegistrationNeeded = modifyRegistrationNeeded; } + public boolean isRegistered() + { + Boolean valid = isRegistrationValid(); + if (valid == null) + { + return getRegistrationHandle() != null; + } + else + { + return valid; + } + } + public class RegistrationRefreshResult extends RefreshResult { private Map<String, RegistrationProperty> registrationProperties; Modified: components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationProperty.java =================================================================== --- components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationProperty.java 2010-02-13 03:41:18 UTC (rev 1668) +++ components/wsrp/trunk/consumer/src/main/java/org/gatein/wsrp/consumer/RegistrationProperty.java 2010-02-13 11:18:05 UTC (rev 1669) @@ -181,6 +181,7 @@ if ((persistentValue != null && !persistentValue.equals(stringValue)) || (persistentValue == null && stringValue != null)) { String oldValue = persistentValue; + Status oldStatus = status; persistentValue = stringValue; if (persistentValue == null) { @@ -192,7 +193,7 @@ } // notify listeners - notifyListener(oldValue, persistentValue); + notifyListener(oldValue, persistentValue, oldStatus); } } @@ -217,13 +218,10 @@ this.status = status; } - private void notifyListener(String oldValue, String newValue) + private void notifyListener(String oldValue, String newValue, Status oldStatus) { - // listener can be null, especially when props are unfrozen from Hibernate - if (listener != null) - { - listener.propertyValueChanged(this, oldValue, newValue); - } + ParameterValidation.throwIllegalArgExceptionIfNull(listener, "PropertyChangeListener"); + listener.propertyValueChanged(this, oldStatus, oldValue, newValue); } public void setListener(PropertyChangeListener listener) @@ -233,6 +231,14 @@ static interface PropertyChangeListener { - void propertyValueChanged(RegistrationProperty property, Object oldValue, Object newValue); + /** + * Only called if an actual change occurred, i.e. oldvalue is guaranteed to be different from newValue + * + * @param property + * @param previousStatus + * @param oldValue + * @param newValue + */ + void propertyValueChanged(RegistrationProperty property, Status previousStatus, Object oldValue, Object newValue); } } Modified: components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationInfoTestCase.java =================================================================== --- components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationInfoTestCase.java 2010-02-13 03:41:18 UTC (rev 1668) +++ components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationInfoTestCase.java 2010-02-13 11:18:05 UTC (rev 1669) @@ -24,6 +24,7 @@ package org.gatein.wsrp.consumer; import junit.framework.TestCase; +import org.gatein.wsrp.WSRPConstants; import org.gatein.wsrp.WSRPTypeFactory; import org.gatein.wsrp.test.protocol.v1.ServiceDescriptionBehavior; import org.oasis.wsrp.v1.Property; @@ -44,6 +45,7 @@ { private RegistrationInfo info; private static final String producerId = "test"; + private static final RegistrationContext FAKE_REGISTRATION_CONTEXT = WSRPTypeFactory.createRegistrationContext("handle"); protected void setUp() throws Exception { @@ -67,6 +69,8 @@ assertNull(info.isRegistrationRequired()); // and we don't know if the registration is valid assertNull(info.isRegistrationValid()); + // and we are not registered + assertFalse(info.isRegistered()); assertFalse(info.isModifiedSinceLastRefresh()); assertFalse(info.isModifyRegistrationNeeded()); @@ -90,6 +94,56 @@ } } + public void testRegistration() + { + register(); + + assertEquals(FAKE_REGISTRATION_CONTEXT.getRegistrationHandle(), info.getRegistrationHandle()); + assertEquals(FAKE_REGISTRATION_CONTEXT.getRegistrationState(), info.getRegistrationState()); + + assertTrue(info.isRegistered()); + assertFalse(info.isModifiedSinceLastRefresh()); + assertFalse(info.isModifyRegistrationNeeded()); + assertTrue(info.isConsistentWithProducerExpectations()); + + assertFalse(info.isRefreshNeeded()); + + assertTrue(info.isRegistrationRequired()); + assertTrue(info.isRegistrationValid()); + assertFalse(info.isRegistrationDeterminedNotRequired()); + assertTrue(info.isRegistrationDeterminedRequired()); + assertFalse(info.isUndetermined()); + } + + public void testIsModifyRegistrationNeeded() + { + String key = "foo"; + + // if we're not registered, then modifying registration is never needed + assertFalse(info.isRegistered()); + info.setRegistrationPropertyValue(key, "bar"); + assertFalse(info.isModifyRegistrationNeeded()); + + register(); + + // if we try to set the property to the same value, nothing should change + info.setRegistrationPropertyValue(key, "bar"); + assertFalse(info.isModifiedSinceLastRefresh()); + assertFalse(info.isModifyRegistrationNeeded()); + + // but if we set the property to a different value, then we need to modify the registration + info.setRegistrationPropertyValue(key, "new"); + assertTrue(info.isModifiedSinceLastRefresh()); + assertTrue(info.isModifyRegistrationNeeded()); + } + + private void register() + { + // setting a registration context should simulate a successful registration + info.setRegistrationContext(FAKE_REGISTRATION_CONTEXT); + info.setConsumerName(WSRPConstants.DEFAULT_CONSUMER_NAME); + } + public void testSimpleSetGetRegistrationProperty() { String key = "foo"; @@ -98,7 +152,7 @@ // check status assertNull(info.isConsistentWithProducerExpectations()); assertTrue(info.isModifiedSinceLastRefresh()); - assertTrue(info.isModifyRegistrationNeeded()); + assertFalse(info.isModifyRegistrationNeeded()); // since we were not registered, modification of registration shouldn't be needed Map properties = info.getRegistrationProperties(); assertFalse(properties.isEmpty()); @@ -133,6 +187,8 @@ assertNull("Property value has changed since last refresh, status should be unknown", prop.isInvalid()); assertEquals("Property value has changed since last refresh, status should be unknown", RegistrationProperty.Status.UNCHECKED_VALUE, prop.getStatus()); + assertTrue(info.isModifiedSinceLastRefresh()); + assertTrue(info.isModifyRegistrationNeeded()); } public void testRefreshNoRegistration() @@ -203,12 +259,11 @@ public void testRefreshRegistrationDefaultRegistrationExtraLocalInfoWhileRegistered() { + register(); + // set a registration property info.setRegistrationPropertyValue("foo", "bar"); - // simulate being registered - info.setRegistrationHandle("blah"); - // we were registered so we need to call modifyRegistration, force check of extra props RegistrationInfo.RegistrationRefreshResult result = info.refresh( createServiceDescription(true, 0), producerId, false, false, true); @@ -257,13 +312,12 @@ public void testRefreshRegistrationRegistrationNoLocalInfoWhileRegistered() { + register(); + // producer requests 2 registration properties ServiceDescription sd = createServiceDescription(true, 2); - // simulate registration - info.setRegistrationHandle("blah"); - - RegistrationInfo.RegistrationRefreshResult result = info.refresh(sd, producerId, false, false, false); + RegistrationInfo.RegistrationRefreshResult result = info.refresh(sd, producerId, false, true, false); assertNotNull(result); assertTrue(result.hasIssues()); assertEquals(RefreshResult.Status.MODIFY_REGISTRATION_REQUIRED, result.getStatus()); @@ -368,15 +422,18 @@ info.setRegistrationPropertyValue("prop0", "value0"); assertTrue(info.isModifiedSinceLastRefresh()); - assertTrue(info.isModifyRegistrationNeeded()); + assertFalse(info.isModifyRegistrationNeeded()); // not registered, so modifying registration is not needed + RegistrationData registrationData = info.getRegistrationData(); checkRegistrationData(registrationData, "value0"); // check that setRegistrationValidInternalState properly updates RegistrationData if required info.setRegistrationPropertyValue("prop0", "value1"); assertTrue(info.isModifiedSinceLastRefresh()); - assertTrue(info.isModifyRegistrationNeeded()); - info.setRegistrationValidInternalState(); + assertFalse(info.isModifyRegistrationNeeded()); + + register(); + assertFalse(info.isModifiedSinceLastRefresh()); assertFalse(info.isModifyRegistrationNeeded()); List<Property> properties = info.getRegistrationData().getRegistrationProperties(); @@ -401,7 +458,7 @@ info.refresh(createServiceDescription(true, 1), producerId, true, true, false); // simulate successful registration - info.setRegistrationContext(WSRPTypeFactory.createRegistrationContext("handle")); + info.setRegistrationContext(FAKE_REGISTRATION_CONTEXT); assertTrue(info.isRegistrationRequired()); assertTrue(info.isRegistrationValid()); @@ -422,7 +479,7 @@ public void testRefreshRegisteredWithoutPropsAndProducerNowSendsProps() { // simulate successful registration - info.setRegistrationContext(WSRPTypeFactory.createRegistrationContext("handle")); + info.setRegistrationContext(FAKE_REGISTRATION_CONTEXT); assertTrue(info.isRegistrationRequired()); assertTrue(info.isRegistrationValid()); Modified: components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationPropertyTestCase.java =================================================================== --- components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationPropertyTestCase.java 2010-02-13 03:41:18 UTC (rev 1668) +++ components/wsrp/trunk/consumer/src/test/java/org/gatein/wsrp/consumer/RegistrationPropertyTestCase.java 2010-02-13 11:18:05 UTC (rev 1669) @@ -43,7 +43,7 @@ { boolean called; - public void propertyValueChanged(RegistrationProperty property, Object oldValue, Object newValue) + public void propertyValueChanged(RegistrationProperty property, RegistrationProperty.Status previousStatus, Object oldValue, Object newValue) { called = (prop == property) && VALUE.equals(oldValue) && NEW_VALUE.equals(newValue); }