AbstractCollectionMetaData fails with optional add() operati

[juha]

I'm mapping a bean with a java.util.List type property where the bean getter returns a read-only view of the list via Collections.unmodifiableList(original).

org.jboss.beans.metadata.plugins.AbstractCollectionMetaData#getValue fails here with a deployment exception since the method does not ensure the collection instance supports the optional Collection.add() operation (and doesn't fall back to default collection)

*** DEPLOYMENTS IN ERROR: Name -> Error
Manager -> java.lang.UnsupportedOperationException

It seems rather unintuitive that the MC requires the read-method to provide a mutable collection reference and proceeds to make changes to my collection via read-method access?

[alesj]

Can you add your XML example?

But I guess this is a cause of this feature:
- http://jira.jboss.com/jira/browse/JBMICROCONT-35

You can do a workaround with 'lazy' getter:

List getMyList()
{
 return mylist != null ? Collections.unmodifiableList(mylist) : null;
}

If that still works for you.

[juha]

<?xml version="1.0" encoding="UTF-8"?>

<deployment xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xsi:schemaLocation="urn:jboss:bean-deployer bean-deployer_2_0.xsd"
 xmlns="urn:jboss:bean-deployer:2.0">


 <bean name = "Manager" class = "PluginManager">
 <property name = "bootstrapPlugins">
 <list elementClass = "java.lang.String">
 <value>Test</value>
 </list>
 </property>
 </bean>

</deployment>

public class PluginManager
{

 private List bootstrapPlugins = new ArrayList(10);

 public void setBootstrapPlugins(List listOfPluginNames)
 {
 this.bootstrapPlugins = listOfPluginNames;
 }

 public List getBootstrapPlugins()
 {
 return Collections.unmodifiableList(bootstrapPlugins);
 }
}

[adrian.brock]

There's no way to check whether a collections is modifiable or otherwise.

But it should certainly be possible to turn this feature off,
with something like the requiresNew attribute on the collection below.

i.e. don't use the getter as the source of the collection to modify

<?xml version="1.0" encoding="UTF-8"?>

<deployment xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xsi:schemaLocation="urn:jboss:bean-deployer bean-deployer_2_0.xsd"
 xmlns="urn:jboss:bean-deployer:2.0">


 <bean name = "Manager" class = "PluginManager">
 <property name = "bootstrapPlugins">
 <list elementClass = "java.lang.String" requiresNew="true">
 <value>Test</value>
 </list>
 </property>
 </bean>

</deployment>

[alesj]

I added 'preinstantiate' attribute on the property element.
So you can turn preinstantiate lookup on/off for any property, not just collections.

Just writing the tests now.

[adrian.brock]

In fact, the solution should really be that the type specified on the collection
overrides this feature. But currently this is not the case.

i.e. the following should always create a new array list not use the getter.

<?xml version="1.0" encoding="UTF-8"?>

<deployment xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xsi:schemaLocation="urn:jboss:bean-deployer bean-deployer_2_0.xsd"
 xmlns="urn:jboss:bean-deployer:2.0">


 <bean name = "Manager" class = "PluginManager">
 <property name = "bootstrapPlugins">
 <list elementClass = "java.lang.String" class="java.util.ArrayList">
 <value>Test</value>
 </list>
 </property>
 </bean>

</deployment>

[alesj]

In fact, the solution should really be that the type specified on the collection
overrides this feature. But currently this is not the case.

i.e. the following should always create a new array list not use the getter.

<?xml version="1.0" encoding="UTF-8"?>

<deployment xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xsi:schemaLocation="urn:jboss:bean-deployer bean-deployer_2_0.xsd"
 xmlns="urn:jboss:bean-deployer:2.0">


 <bean name = "Manager" class = "PluginManager">
 <property name = "bootstrapPlugins">
 <list elementClass = "java.lang.String" class="java.util.ArrayList">
 <value>Test</value>
 </list>
 </property>
 </bean>

</deployment>

I'll turn the 'preinstantiate' to false by default.
So this will then work out-of-the-box.
And if you explicitly tell to use preinstantiated, then the class attribute will be ignored in the case getter returns non null instance.

[alesj]

"alesj" wrote:

I'll turn the 'preinstantiate' to false by default.
So this will then work out-of-the-box.
And if you explicitly tell to use preinstantiated, then the class attribute will be ignored in the case getter returns non null instance.

The only issue is that the behaviour will change.
Those who used preinstantiated instances before by default, should now explicitly mark them preinstantiate='true'.

I can issue a big fat log warning. :-)

[adrian.brock]

There's no need to start issuing warnings.
This stuff has never been released GA.

The correct order for this processing is
(and should always have been - if the user is explicit, do what they want):

1) Any class specified on the list element (like above) - new collection
2) Any class specified on the parent element (the typeInfo passed into getValue),
e.g.

 <property name="blah" class="java.util.ArrayList"><list/></property>

- new collection
3) The getter - use the returned collection (if any)
4) the default collection type - new collection

The only reason for the preinstantiated flag would be to turn off (3)
but you can always do this anyway by using (1) or (2).

[adrian.brock]

Actually (2) is probably not correct in the above, it should come between (3) and (4)

It doesn't actually exist for properties
(except as a hack/short hand for string value properties - not relevant here)
and actually defines which constructor/method to use for parameters
so it would always override the preinstantiated which isn't what is wanted
and sometimes it will just represent an interface anyway (because that is the parameter type).

So, the correct order should be:

1) AbstractTypeMetaData.getClassInfo(ClassInfo);
2) preinstantiated
3) TypeInfo in getValue() parameters
4) Default collection type

But can still stop the getter being used by adding a class= to the list element.

[alesj]

Yes, but this means people have to explicitly write class="java.util.ArrayList" if they don't want to use preinstantiated instance. All they really want is to write (list)...elements(/list).
I leave the preinstantiated attribute - false by default.
I think it is simple enough.

What we really wanted is to have Juha's case resolved. ;-)

[adrian.brock]

Yes, but the whole point of pre-instantiated is to reduce the amount users have
to write in xml - not least because it can also become inconsistent if they have
to do it two places.

Imagine if Juha's class was:

public class PluginManager
{
 private List bootstrapPlugins = new CopyOnWriteArrayList(10);

 public void setBootstrapPlugins(List listOfPluginNames)
 {
 this.bootstrapPlugins = listOfPluginNames;
 }

 public List getBootstrapPlugins()
 {
 return bootstrapPlugins;
 }
}

The user doesn't want to have to say "preinstantiated=true" or
class="java.util.concurrent.CopyOnWriteArrayList" to make this work
how they would expect!

It should just work out of the box.

It's only in classes like Juha's where you have to turn off preinstantiated
because the getter is returning an immutable.
Therefore you need to be explicit about what implementation you really want,
we can't work it out.

We should optimize for the common cases.

[juha]

There's no way to check whether a collections is modifiable or otherwise.

True. You could address the scenario by catching the UnsupportedOperationException on Collection.add() and falling back to a default mechanism.

It's not a complete solution but depending on how common you think the use case is, would address it without extra burden of configuration on user's part.

[adrian.brock]

There's no way to check whether a collections is modifiable or otherwise.

True. You could address the scenario by catching the UnsupportedOperationException on Collection.add() and falling back to a default mechanism.

It's not a complete solution but depending on how common you think the use case is, would address it without extra burden of configuration on user's part.

It doesn't quite work the way you describe.
The collection is first instantiated and then the values added.

We could hack it like you describe, but it wouldn't be very clean.
Maybe it is worth the ugliness for the ease of use? :-)

[juha]

I'm solidly in the camp of accepting ugliness in implementation for ease of use :-)