> restriction for few Values to be dynamically modifable. From that > standpoint, Configuration declared which Values it thinks can be > dynamically configuratble (Values themselves are inert) and allows their > modification only via itself.
I think that it would be better from an encapsulation standpoint to put this logic into Value. Value is not just a simple data structure; it has complex behavior. This would become just one more piece of that. > I felt that way Configuration API is > explictly stating its capability to dynamic modification. I think that it is more in line with the rest of our configuration / value architecture for the Values to declare whether or not they are configurable. > Configuration > also now has to skip the dynamic Values from its hashCode. Thus > Configuration in one way or the other be aware that certain Values are > dynamic. This is actually a bug IMO -- we should not be skipping them, but rather using the original values. I think that that is a better proxy for equality comparison. > Even now, Value could be changed bypassing Configuration but such > pathway will negatively impact two assumptions a) Configuration is > frozen b) value-based hashCode is Configuration's identity. Exactly. Therefore, it seems like it'd be better to put the logic for preventing changing values closer to the values, instead of in the Configuration. -Patrick On 8/20/07, Pinaki Poddar <[EMAIL PROTECTED]> wrote: > Hi, > Configuration imposed the restrictions on its Values to be immutable > when configuration is frozen. Now Configuration is relaxing that > restriction for few Values to be dynamically modifable. From that > standpoint, Configuration declared which Values it thinks can be > dynamically configuratble (Values themselves are inert) and allows their > modification only via itself. I felt that way Configuration API is > explictly stating its capability to dynamic modification. Configuration > also now has to skip the dynamic Values from its hashCode. Thus > Configuration in one way or the other be aware that certain Values are > dynamic. > > Even now, Value could be changed bypassing Configuration but such > pathway will negatively impact two assumptions a) Configuration is > frozen b) value-based hashCode is Configuration's identity. > > > Pinaki Poddar > 972.834.2865 > > -----Original Message----- > From: Patrick Linskey [mailto:[EMAIL PROTECTED] > Sent: Monday, August 20, 2007 1:29 PM > To: [email protected] > Subject: Re: svn commit: r566494 - in /openjpa/trunk: > openjpa-kernel/src/main/java/org/apache/openjpa/conf/ > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/ > openjpa-lib/src/main/resources/org/apache/openjpa/lib/conf/ > openjpa-persistence-jdbc/src/test/ja > > Hi, > > I don't understand why the dynamicness of a setting needs to be exposed > to the Configuration. > > It seems like it would be more appropriate to move the logic for whether > or not something is dynamic to the Value type itself. Value could then > have a method called getOriginalString(). For non-dynamic settings, this > would be the same as getString(). > > Additionally, we could make set() or setString() do the logic to > validate if the Value's Configuration is read-only and is not dynamic. > It seems like this would be better from an encapsulation standpoint. > > With such an implementation, we would not need to create any new methods > in Configuration, and everything would Just Work, once we changed > Configuration's equals() method to use getOriginalString(). > Additionally, this would mean that instead of ignoring dynamic settings, > we'd be comparing to their original values, which seems like a better > proxy for equality. > > -Patrick > > On 8/15/07, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote: > > Author: ppoddar > > Date: Wed Aug 15 22:52:17 2007 > > New Revision: 566494 > > > > URL: http://svn.apache.org/viewvc?view=rev&rev=566494 > > Log: > > Adding support for change of configuration properties after the > configuration has been frozen. > > Three methods have been added to Configuration: > > i) Configuration.getDynamicValues() returns list of Values that are > dynamically modifiable. > > ii) Configuration.isDynamic(String property) affirms if the named > property is dynamically modifiable. > > iii) Configuration.modifyDynamic(String property, Object value) > modifies the named property value even when Configuration.isReadOnly(). > > > > Currently, OpenJPAConfigurationImpl.getDynamicValues() return 3 > > simple IntValue properties {dataCacheTimeout, fetchBatchSize, > > lockTimeout) > > > > > > > > Added: > > > > openjpa/trunk/openjpa-persistence-jdbc/src/test/java/org/apache/openjp > > a/conf/TestDynamicConfiguration.java > > Modified: > > > openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/conf/OpenJ > PAConfigurationImpl.java > > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Conf > iguration.java > > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Conf > igurationImpl.java > > > > openjpa/trunk/openjpa-lib/src/main/resources/org/apache/openjpa/lib/co > > nf/localizer.properties > > > > Modified: > > openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/conf/Ope > > nJPAConfigurationImpl.java > > URL: > > http://svn.apache.org/viewvc/openjpa/trunk/openjpa-kernel/src/main/jav > > a/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java?view=diff&rev= > > 566494&r1=566493&r2=566494 > > ====================================================================== > > ======== > > --- > > openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/conf/Ope > > nJPAConfigurationImpl.java (original) > > +++ openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/conf > > +++ /OpenJPAConfigurationImpl.java Wed Aug 15 22:52:17 2007 > > @@ -1437,4 +1437,9 @@ > > public Log getConfigurationLog() { > > return getLog(LOG_RUNTIME); > > } > > + > > + public Value[] getDynamicValues() { > > + return new Value[] > {dataCacheTimeout,fetchBatchSize,lockTimeout}; > > + } > > + > > } > > > > Modified: > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Co > > nfiguration.java > > URL: > > http://svn.apache.org/viewvc/openjpa/trunk/openjpa-lib/src/main/java/o > > rg/apache/openjpa/lib/conf/Configuration.java?view=diff&rev=566494&r1= > > 566493&r2=566494 > > ====================================================================== > > ======== > > --- > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Co > > nfiguration.java (original) > > +++ openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/con > > +++ f/Configuration.java Wed Aug 15 22:52:17 2007 > > @@ -223,4 +223,22 @@ > > * Return a copy of this configuration. > > */ > > public Object clone(); > > + > > + /** > > + * Modifies a <em>dynamic</em> property of this receiver even > when > > + * [EMAIL PROTECTED] #setReadOnly(boolean) frozen}. > > + */ > > + public void modifyDynamic(String property, Object newValue); > > + > > + /** > > + * Affirms if the given property can be modified > <em>dynamically</em> i.e. > > + * even after the receiver is [EMAIL PROTECTED] #setReadOnly(boolean) > frozen}. > > + */ > > + public boolean isDynamic(String property); > > + > > + /** > > + * Gets the values that can be modified <em>dynamically</em> i.e. > > + * even after the receiver is [EMAIL PROTECTED] #setReadOnly(boolean) > frozen}. > > + */ > > + public Value[] getDynamicValues(); > > } > > > > Modified: > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Co > > nfigurationImpl.java > > URL: > > http://svn.apache.org/viewvc/openjpa/trunk/openjpa-lib/src/main/java/o > > rg/apache/openjpa/lib/conf/ConfigurationImpl.java?view=diff&rev=566494 > > &r1=566493&r2=566494 > > ====================================================================== > > ======== > > --- > > openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Co > > nfigurationImpl.java (original) > > +++ openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/con > > +++ f/ConfigurationImpl.java Wed Aug 15 22:52:17 2007 > > @@ -803,7 +803,7 @@ > > ConfigurationImpl conf = (ConfigurationImpl) other; > > Map p1 = (_props == null) ? toProperties(false) : _props; > > Map p2 = (conf._props == null) ? conf.toProperties(false) : > conf._props; > > - return p1.equals(p2); > > + return excludeDynamic(p1).equals(excludeDynamic(p2)); > > } > > > > /** > > @@ -811,9 +811,8 @@ > > * [EMAIL PROTECTED] #toProperties}. > > */ > > public int hashCode() { > > - if (_props != null) > > - return _props.hashCode(); > > - return toProperties(false).hashCode(); > > + Map copy = (_props == null) ? toProperties(false) : _props; > > + return excludeDynamic(copy).hashCode(); > > } > > > > /** > > @@ -988,5 +987,36 @@ > > PluginListValue val = new PluginListValue(property); > > addValue(val); > > return val; > > + } > > + > > + public void modifyDynamic(String property, Object newValue) { > > + if (!isDynamic(property)) > > + throw new RuntimeException(_loc.get("not-dynamic", > property) > > + .toString()); > > + Value value = getValue(property); > > + value.setObject(newValue); > > + } > > + > > + public boolean isDynamic(String property) { > > + Value[] dynamicValues = getDynamicValues(); > > + for (int i=0; i<dynamicValues.length; i++) > > + if (dynamicValues[i].getProperty().equals(property)) > > + return true; > > + return false; > > + } > > + > > + public Value[] getDynamicValues() { > > + return new Value[0]; > > + } > > + > > + Map excludeDynamic(Map map) { > > + if (map == null) > > + return null; > > + Map copy = new HashMap(map); > > + Value[] dynamicValues = getDynamicValues(); > > + for (int i=0; i<dynamicValues.length; i++) { > > + > Configurations.removeProperty(dynamicValues[i].getProperty(), copy); > > + } > > + return copy; > > } > > } > > > > Modified: > > openjpa/trunk/openjpa-lib/src/main/resources/org/apache/openjpa/lib/co > > nf/localizer.properties > > URL: > > http://svn.apache.org/viewvc/openjpa/trunk/openjpa-lib/src/main/resour > > ces/org/apache/openjpa/lib/conf/localizer.properties?view=diff&rev=566 > > 494&r1=566493&r2=566494 > > ====================================================================== > > ======== > > --- > > openjpa/trunk/openjpa-lib/src/main/resources/org/apache/openjpa/lib/co > > nf/localizer.properties (original) > > +++ openjpa/trunk/openjpa-lib/src/main/resources/org/apache/openjpa/li > > +++ b/conf/localizer.properties Wed Aug 15 22:52:17 2007 > > @@ -110,3 +110,5 @@ > > Id-cat: General > > Id-displayorder: 50 > > Id-expert: true > > + > > +not-dynamic: Can not modify "{0}" to "{1}" because the property is > not dynamic. > > > > Added: > > openjpa/trunk/openjpa-persistence-jdbc/src/test/java/org/apache/openjp > > a/conf/TestDynamicConfiguration.java > > URL: > > http://svn.apache.org/viewvc/openjpa/trunk/openjpa-persistence-jdbc/sr > > c/test/java/org/apache/openjpa/conf/TestDynamicConfiguration.java?view > > =auto&rev=566494 > > ====================================================================== > > ======== > > --- > > openjpa/trunk/openjpa-persistence-jdbc/src/test/java/org/apache/openjp > > a/conf/TestDynamicConfiguration.java (added) > > +++ openjpa/trunk/openjpa-persistence-jdbc/src/test/java/org/apache/op > > +++ enjpa/conf/TestDynamicConfiguration.java Wed Aug 15 22:52:17 2007 > > @@ -0,0 +1,121 @@ > > +/* > > + * Licensed to the Apache Software Foundation (ASF) under one > > + * or more contributor license agreements. See the NOTICE file > > + * distributed with this work for additional information > > + * regarding copyright ownership. The ASF licenses this file > > + * to you 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 > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, > > + * software distributed under the License is distributed on an > > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > > + * KIND, either express or implied. See the License for the > > + * specific language governing permissions and limitations > > + * under the License. > > + */ > > +package org.apache.openjpa.conf; > > + > > +import javax.persistence.Persistence; import > > +org.apache.openjpa.persistence.test.*; > > +import org.apache.openjpa.conf.OpenJPAConfiguration; > > +import org.apache.openjpa.lib.conf.Value; > > +import org.apache.openjpa.persistence.OpenJPAEntityManagerFactory; > > +import org.apache.openjpa.persistence.OpenJPAPersistence; > > + > > +/** > > + * Tests dynamic modification of configuration property. > > + * > > + * @author Pinaki Poddar > > + * > > + */ > > +public class TestDynamicConfiguration extends SingleEMFTestCase { > > + > > + public void testConfigurationIsEqualByValueAndHashCode() { > > + OpenJPAEntityManagerFactory emf1 = createEMF(); > > + assertNotNull(emf1); > > + OpenJPAConfiguration conf1 = emf1.getConfiguration(); > > + > > + OpenJPAEntityManagerFactory emf2 = createEMF(); > > + assertNotNull(emf2); > > + OpenJPAConfiguration conf2 = emf2.getConfiguration(); > > + > > + assertFalse(emf1==emf2); > > + assertFalse(emf1.equals(emf2)); > > + assertFalse(conf1==conf2); > > + assertEquals(conf1, conf2); > > + assertEquals(conf1.hashCode(), conf2.hashCode()); > > + assertEquals(conf1.toProperties(false), > conf2.toProperties(false)); > > + } > > + > > + public void > testConfigurationIsReadOnlyAfterFirstConstruction() { > > + OpenJPAEntityManagerFactory emf = createEMF(); > > + assertNotNull(emf); > > + OpenJPAConfiguration conf = emf.getConfiguration(); > > + assertFalse(conf.isReadOnly()); > > + emf.createEntityManager(); > > + assertTrue(conf.isReadOnly()); > > + } > > + > > + public void testDynamicValuesCanNotBeChangedDirectly() { > > + OpenJPAEntityManagerFactory emf = createEMF(); > > + assertNotNull(emf); > > + emf.createEntityManager(); > > + OpenJPAConfiguration conf = emf.getConfiguration(); > > + > > + Value[] dynamicValues = conf.getDynamicValues(); > > + assertTrue(dynamicValues.length>0); > > + assertTrue(conf.isDynamic("LockTimeout")); > > + > > + int oldValue = conf.getLockTimeout(); > > + int newValue = oldValue + 10; > > + try { > > + conf.setLockTimeout(newValue); > > + fail("Expected exception to modify > configuration directly"); > > + } catch (Exception ex) { // good > > + assertEquals(oldValue, conf.getLockTimeout()); > > + } > > + } > > + > > + public void testDynamicValuesCanBeChanged() { > > + OpenJPAEntityManagerFactory emf = createEMF(); > > + assertNotNull(emf); > > + OpenJPAConfiguration conf = emf.getConfiguration(); > > + > > + Value[] dynamicValues = conf.getDynamicValues(); > > + assertTrue(dynamicValues.length>0); > > + assertTrue(conf.isDynamic("LockTimeout")); > > + > > + int oldValue = conf.getLockTimeout(); > > + int newValue = oldValue + 10; > > + > > + conf.modifyDynamic("LockTimeout", newValue); > > + assertEquals(newValue, conf.getLockTimeout()); > > + } > > + > > + public void testDynamicValuesAreCorrectlySet() { > > + OpenJPAEntityManagerFactory emf = createEMF(); > > + assertNotNull(emf); > > + OpenJPAConfiguration conf = emf.getConfiguration(); > > + > > + Value[] dynamicValues = conf.getDynamicValues(); > > + assertTrue(dynamicValues.length>0); > > + assertTrue(conf.isDynamic("LockTimeout")); > > + } > > + > > + public void testDynamicChangeDoesNotChangeHashCode() { > > + OpenJPAEntityManagerFactory emf = createEMF(); > > + assertNotNull(emf); > > + OpenJPAConfiguration conf1 = emf.getConfiguration(); > > + > > + int oldValue = conf1.getLockTimeout(); > > + int newValue = oldValue+10; > > + int oldHash = conf1.hashCode(); > > + conf1.modifyDynamic("LockTimeout", newValue); > > + int newHash = conf1.hashCode(); > > + > > + assertEquals(oldHash, newHash); > > + } > > +} > > > > > > > > > -- > Patrick Linskey > 202 669 5907 > > Notice: This email message, together with any attachments, may contain > information of BEA Systems, Inc., its subsidiaries and affiliated > entities, that may be confidential, proprietary, copyrighted and/or > legally privileged, and is intended solely for the use of the individual or > entity named in this message. If you are not the intended recipient, and have > received this message in error, please immediately return this by email and > then delete it. > -- Patrick Linskey 202 669 5907
