Property replacement

[alesj]

Just so that we don't totally hijack this thread:
- http://www.jboss.com/index.html?module=bb&op=viewtopic&t=99508

I'm working on the issue.
Just in the testing phase (having some problems with permission to set System property).

I added this tests:
- org.jboss.test.kernel.config.test.PropertyTestCase
- org.jboss.test.kernel.config.test.PropertyXMLTestCase

[alesj]

How do I get pass this:

access denied (java.util.PropertyPermission test.property.value write)
java.security.AccessControlException: access denied (java.util.PropertyPermission test.property.value write)
 at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
 at java.security.AccessController.checkPermission(AccessController.java:427)
 at java.lang.SecurityManager.checkPermission(SecurityManager.java:532)
 at java.lang.System.setProperty(System.java:699)
 at org.jboss.test.kernel.config.test.PropertyTestCase$1.run(PropertyTestCase.java:64)
 at java.security.AccessController.doPrivileged(Native Method)
 at org.jboss.test.kernel.config.test.PropertyTestCase.testPropertyWithPropertyValue(PropertyTestCase.java:60)
 at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
 at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
 at junit.extensions.TestSetup.run(TestSetup.java:23)

My code:

public void testPropertyWithPropertyValue() throws Throwable
 {
 // set property to be replaced
 final String CONST = "PropertyReplaceTestCase";

 AbstractTestDelegate delegate = getDelegate();
 delegate.enableSecurity = false;
 AccessController.doPrivileged(new PrivilegedAction<Object>()
 {
 public Object run()
 {
 System.setProperty("test.property.value", CONST);
 return null;
 }
 });

 // get property
 Object value = instantiateReplacePropertyValue();
 assertNotNull(value);
 assertEquals(String.class, value.getClass());
 assertEquals(CONST, value);
 }

[adrian.brock]

Hard to tell without seeing the whole code.

I'd guess you have AbstractKernelTest somewhere in your hierarchy?

Which installs a SecurityManager:

 public static AbstractTestDelegate getDelegate(Class clazz) throws Exception
 {
 AbstractTestDelegate delegate = new AbstractTestDelegate(clazz);
 delegate.enableSecurity = true;
 return delegate;
 }

The tests get only very basic privilieges, the idea being that we want to make
sure the MC works properly in a restricted environment.

[adrian.brock]

"alesj" wrote:

AbstractTestDelegate delegate = getDelegate();
delegate.enableSecurity = false;

This won't work.
The flag is only considered during test setup

You could perhaps add a couple of methods to AbstractTestCaseWithSetup
that let you temporarily suspend the security manager.

protected SecurityManager suspendSecurity()
{
 SecurityManager result = System.getSecurityManager();
 System.setSecurityManager(null);
 return result;
}

protected void resumeSecurity(SecurityManager securityManager)
{
 System.setSecurityManager(securityManager);
}

You could then do:

SecurityManager sm = suspendSecurity();
try
{
 // Do priviledged stuff
}
finally
{
 resumeSecurity(sm);
}

[adrian.brock]

Sorry that would of course need to be :-)

protected SecurityManager suspendSecurity()
{
 return AccessController.doPrivileged(new PrivilegedAction<SecurityManager>
 {
 SecurityManager result = System.getSecurityManager();
 System.setSecurityManager(null);
 return result;
 });
}

[alesj]

I added this SecurityManager suspend / resume and things get pass that write permission problem.

But now I see that the ${x} string gets actually picked up by simple StringEditor, and not by 'replacing' Editor.

I added this code check (in ValueConvertor):

if (clazz.isAssignableFrom(valueClass) && isPropertyReplaceValue(value) == false)
 return value;

So that we even get to PropertyEditors.

[alesj]

"alesj" wrote:

But now I see that the ${x} string gets actually picked up by simple StringEditor, and not by 'replacing' Editor.

Ok, this is expected behaviour, since our targetType is String.

Should I change the StringEditor to look for replacement?
Or should I wrap the string value in some ReplecamentString class and add ReplecamentStringEditor into commons editors?

[adrian.brock]

None of the property editors should be doing system property replacement.

The best way to fix this and maintain backwards compatibility is the following
diff in ValueConvertor.

[ejort@warjort plugins]$ svn diff ValueConvertor.java
Index: ValueConvertor.java
===================================================================
--- ValueConvertor.java (revision 59913)
+++ ValueConvertor.java (working copy)
@@ -31,6 +31,7 @@
 import org.jboss.reflect.plugins.introspection.ReflectionUtils;
 import org.jboss.reflect.spi.ProgressionConvertor;
 import org.jboss.reflect.spi.ProgressionConvertorFactory;
+import org.jboss.util.StringPropertyReplacer;
 import org.jboss.util.propertyeditor.PropertyEditors;

 /**
@@ -69,15 +70,37 @@
 * @return the value or null if there is no editor
 * @throws Throwable for any error
 */
- @SuppressWarnings("unchecked")
 public static Object convertValue(Class<? extends Object> clazz, Object value) throws Throwable
 {
+ return convertValue(clazz, value, false);
+ }
+
+ /**
+ * Convert a value
+ *
+ * @param clazz the class
+ * @param value the value
+ * @param replaceProperties whether to replace system properties
+ * @return the value or null if there is no editor
+ * @throws Throwable for any error
+ */
+ @SuppressWarnings("unchecked")
+ public static Object convertValue(Class<? extends Object> clazz, Object value, boolean replaceProperties) throws Throwable
+ {
 if (clazz == null)
 throw new IllegalArgumentException("Null class");
 if (value == null)
 return null;

 Class<? extends Object> valueClass = value.getClass();
+
+ // If we have a string replace any system properties when requested
+ if (replaceProperties && valueClass == String.class)
+ {
+ String string = (String) value;
+ value = StringPropertyReplacer.replaceProperties(string);
+ }
+
 if (clazz.isAssignableFrom(valueClass))
 return value;

Then add the following new method to TypeInfo
that invokes the alternative method in ValueConvertor

 /**
 * Convert a value
 *
 * @param value the original value
 * @param replaceProperties whether to replace properties
 * @return the converted value
 * @throws Throwable for any error
 */
 Object convertValue(Object value, boolean replaceProperties) throws Throwable;

The StringValueMetaData should be invoking this new method.

Finally, it would be good if StringValueMetaData also had an additional
parameter to turn off string property replace (like the JMX stuff does)

e.g. (but also for other places that take a "value")

<!-- This property is not replaced -->
<property name="blah" replace="false">${hello}</property>

[brian.stansberry]

Apologies in advance if this is a hijack; I'll open another thread if the work discussed here isn't the cause...

In AS trunk with the latest microcontainer jars, this no longer works in a -beans.xml:

<annotation>@org.jboss.aop.microcontainer.aspects.jmx.JMX(name="jboss:service=HASingletonDeploymentScanner,partitionName=${jboss.partition.name:DefaultPartition}", exposedInterface=org.jboss.ha.singleton.HASingletonDeploymentScannerMBean.class, registerDirectly=true)</annotation>

You get:

10:18:00,181 ERROR [AbstractKernelController] Error installing to Configured: name=HASingletonDeploymentScanner state=Instantiated
javax.management.MalformedObjectNameException: Invalid character ':' in value part of property
 at javax.management.ObjectName.construct(ObjectName.java:529)
 at javax.management.ObjectName.<init>(ObjectName.java:1304)
 at org.jboss.aop.microcontainer.aspects.jmx.JMXIntroduction.invoke(JMXIntroduction.java:71)
 at org.jboss.aop.advice.org.jboss.aop.microcontainer.aspects.jmx.JMXIntroduction_z_invoke_24464082.invoke(JMXIntroduction_z_invoke_24464082.java)
 at org.jboss.aop.joinpoint.MethodInvocation.invokeNext(MethodInvocation.java:101)
 at AOPContainerProxy$5.setKernelControllerContext(AOPContainerProxy$5.java)
 at org.jboss.kernel.plugins.dependency.KernelControllerContextAction.installAction(KernelControllerContextAction.java:208)
 at org.jboss.kernel.plugins.dependency.KernelControllerContextAction.install(KernelControllerContextAction.java:136)
...

[alesj]

<annotation>@org.jboss.aop.microcontainer.aspects.jmx.JMX(name="jboss:service=HASingletonDeploymentScanner,partitionName=${jboss.partition.name:DefaultPartition}", exposedInterface=org.jboss.ha.singleton.HASingletonDeploymentScannerMBean.class, registerDirectly=true)</annotation>

Uf, yes.
I removed the XB doing the replacement, but forgot to add this in the annotation value handling - since this string value is directly used, and not via StringValueMetaData.

[alesj]

How do we change this:

 <xsd:simpleType name="annotationType">
 <xsd:annotation>
 <xsd:documentation>
 <![CDATA[
 The annotation type represents a Java5 annotation on the particular join point.
 ]]>
 </xsd:documentation>
 </xsd:annotation>
 <xsd:restriction base="xsd:string">
 <xsd:whiteSpace value="collapse"/>
 </xsd:restriction>
 </xsd:simpleType>

so that I can add an this (replace="false"):

<annotation replace="false">@SomeAnnotation</annotation>

Is it ok that we have StringPropertyReplacer in AbstractAnnotationMetaData?

String annString = StringPropertyReplacer.replaceProperties(annotation);
 //FIXME [JBMICROCONT-99] [JBAOP-278] Use the loader for the bean?
 ann = (Annotation)AnnotationCreator.createAnnotation(annString, Thread.currentThread().getContextClassLoader());

[brian.stansberry]

The places where I was using that syntax, it wasn't critical. So for now I've removed it so the AS trunk builds will work correctly. I'll monitor JBMICRONTAINER-143 and once it's done I'll restore the property replacement syntax in the annotation values.

Thanks!

[starksm64]

"alesj" wrote:
How do we change this:

 <xsd:simpleType name="annotationType">
 <xsd:annotation>
 <xsd:documentation>
 <![CDATA[
 The annotation type represents a Java5 annotation on the particular join point.
 ]]>
 </xsd:documentation>
 </xsd:annotation>
 <xsd:restriction base="xsd:string">
 <xsd:whiteSpace value="collapse"/>
 </xsd:restriction>
 </xsd:simpleType>

To allow an attribute on the annotation element, you need to change it to a complexType/simpleContent:

 <xsd:complexType name="annotationType">
 <xsd:annotation>
 <xsd:documentation>
 <![CDATA[
 The annotation type represents a Java5 annotation on the particular join point.
 ]]>
 </xsd:documentation>
 </xsd:annotation>

 <xsd:simpleContent>
 <xsd:extension base="xsd:string">
 <xsd:attribute name="replace" type="xsd:boolean" default="false" />
 </xsd:extension>
 </xsd:simpleContent>
 </xsd:complexType>

"alesj" wrote:

Is it ok that we have StringPropertyReplacer in AbstractAnnotationMetaData?

String annString = StringPropertyReplacer.replaceProperties(annotation);
 //FIXME [JBMICROCONT-99] [JBAOP-278] Use the loader for the bean?
 ann = (Annotation)AnnotationCreator.createAnnotation(annString, Thread.currentThread().getContextClassLoader());

I guess, but I don't see this as a natural usage. A check of the replace attribute would be needed.

[alesj]

I guess, but I don't see this as a natural usage. A check of the replace attribute would be needed.

I agree.
How can we move this in the same way as we did it with StringValueMD?
But with this one is different - it's a plain String value with possible replacement.
Own TypeInfo --> StringInfo (similar to NumberInfo)?

[starksm64]

In terms of the current schema, its just a different type of element that would need to be treated separately as the AnnotationMetaData does not have a natural string representation outside of the annotation attribute values. We would have to make this a richer type of element with more of a javabean type of property/value structure, but the allowed values are more restricted.

There already is an AnnotationInfo. Doesn't the inherrited TypeInfo.convertValue(Object value, boolean replaceProperties) come into play?