JBAS-6672, null values and defaults

[starksm64]

Essentially this JBAS-6672 issue is asking for null managed property values to be interpreted as resetting primitive values back to their default values. This does not seem to have an easy mapping as there is no practical way for the managed property layer to know what the initial value for the primitive field is. We would have to have a default value notion at the ManagedProperty level, and we would have to have a policy of using that value when a null was passed in for a primitive.

Is there a reason we have to set a primitive to null?

[ips]

On the the XML config file end, only a handful of the config elements are required. For example, if I have a ds.xml file containing only:

<connection-factories>
 <tx-connection-factory>
 <jndi-name>MyTx</jndi-name>
 <rar-name>jms-ra.rar</rar-name>
 <connection-definition>org.jboss.resource.adapter.jms.JmsConnectionFactory</connection-definition>
 </tx-connection-factory>
</connection-factories>

Somehow or another, the other properties will end up with default values on the underlying connection factory. This must be happening in one of two ways:

1) For any missing elements in the XML file, the deployer knows what defaults to use and passes those defaults to the underlying connection factory.
2) For any missing elements in the XML file, the deployer passes null to the underlying connection factory or does not call the corresponding setters at all, and the connection factory knows to use its own internal defaults.

Based on your comment that the profile service would have to store defaults for primitives, I'm guessing that 1) is what was done on the deployer end, because if 2) is what was done, the profile service could do the exact same thing and just rely on the underlying connection factory to provide defaults.

So assuming for now that 1) is what was done on the deployer end, I suggest the following:

1) Add some sort of defaultValue attribute on the ManagedProperty annotation, e.g.:

@ManagementProperty(name="idle-timeout-minutes", includeInTemplate=true, defaultValue="30")
public void setIdleTimeoutMinutes(int idleTimeout) {
this.idleTimeout = idleTimeout;
}

This attribute would be optional and would only be allowed for properties that map to SimpleValues or EnumValues. For primitive properties, when a defaultValue is specified, the Profile Service would set mandatory to false on that property, otherwise it would set mandatory to true. For non-primitive properties, unless the annotation explicitly contained mandatory=true, the properties would default to mandatory=false.

If the profile service can't provide something like the above, we'll have to make all of these primitive properties required in jboss-as-plugin's metadata (today they're optional), and when a user creates a new Connection Factory via JON or the Admin Console, they will have to specify values for 20 or so properties, rather than the 3 that really need to be required (jndi-name, rar-name, and connection-definition). Of course, RHQ provides a mechanism for specifying templates for new Resources, so we can include defaults for all 17 of these properties in our plugin descriptor. This basically places the responsibility of maintaining the defaults on RHQ, rather than the profile service.

[ccrouch]

Summarizing the discussion from todays console call:

1) When creating a new component, if a managed property is not specified or specified as null, then the profile service should make sure any default value for that property is applied during deployment, but that the default value is *not* persisted as part of the managed view of the component.

2) When viewing a resource created in step 1), any properties which were null originally, should be returned as null by the profile service, i.e. they should not be returned as either the default value for the property or the default value for underlying primitive type, e.g. 0 for int, false for boolean.

3) If the default value for a property changes, and that managed property has a null value (e.g. the user has never specified a value), then when the component gets deployed it should use the new default value.

Cheers

[emuckenhuber]

IMHO a null value should not be a valid way to define a default property?

I mean when you use a template, all the default values should be populated. And then it always
returns the actual value.
Although it would be nice to have a defaultValue on the ManagedProperty, as this could be
used to create the template and maybe later from the console directly.

Additionally we might want to have a way to ignore to update certain properties?
This could also be handled internally using Fields? which sets a modified flag once you do setValue();

[starksm64]

In our discussion of this issue on Wed, I thought that it was not the admin console that was specifying the null value. Rather it was somewhere between the ManagementView and the deployers that were coming up with the value. Mostly likely its coming from population ManagedProperties from uninitialized values in the attachment by the ICFs.

The main issue is we should not be taking an uninitialized value as a ManagedProperty value that was set via an admin edit.

[ccrouch]

Ok, just to make sure we're all clear on the use cases the console needs to support, I've described them below. The *how* part, is up for more discussion.

1) user doesn't want to specify a particular value for a configuration property, they are happy to take the server default value (which itself may change over time).
So if todays default is 20, we can't store 20 as the value of the property, since tomorrow the default may be 30, and the user has specified that they wish to always take the server default, regardless of what that is.

2) the user wants to specify a particular value for a configuration property. This is the typical scenario, e.g. a user enters in the jndiName for a datasource.

3) user wants to explicitly specify that a value is empty for a configuration property, they don't want to user the server default. For example the server default for "new-connection-sql" on a Datsource could be ";". So if the user leaves that value unset thats what the Datasource will use, however if the user explicitly sets the value to null (or empty string) then they are saying they don't want any SQL run on new connections.

Thanks

[emuckenhuber]

Yeah the uses cases are fine. Although i still think doing managedProperty.setValue(null); should not replace it with a default value.

I guess a default value is also nice for the template generation.
Additionally we should leverage managedProperty.isModified() to skip not modified properties.
So that we dispatch and update only changed values.

Scott, we might want to remove the managedProperty.isRemoved(), managedProperty.setRemoved()?
i think i requested that once, but now thinking about that again it does not make much sense.

So we could provide setModified(boolean) - and setValue() could compare the original value with the new value and also update this flag.
And if needed a flag to indicate to reset the value to the default one. Although i'm not really sure if this is needed, as the template should have populated the default values anyway?

[starksm64]

"emuckenhuber" wrote:

Scott, we might want to remove the managedProperty.isRemoved(), managedProperty.setRemoved()?
i think i requested that once, but now thinking about that again it does not make much sense.

So we could provide setModified(boolean) - and setValue() could compare the original value with the new value and also update this flag.

Ok.

"emuckenhuber" wrote:

And if needed a flag to indicate to reset the value to the default one. Although i'm not really sure if this is needed, as the template should have populated the default values anyway?

Yes, I would say its the job of the template.

[ccrouch]

"emuckenhuber" wrote:
Although i still think doing managedProperty.setValue(null); should not replace it with a default value.

Ok, so what should we use?

In use case 1) we need to specify "I want to use the default" and in use case 3) we need to specify "I want to use null/nothing/blank".

So maybe for use case 1) we have
managedProperty.setDefaultValue()
and for 3) we have
managedProperty.setValue(null)

or alternatively (but this seems even more of a hack)
1) managedProperty.setValue(null);
3) managedProperty.setValue("");

Because the template will specify an actual value for the default, e.g. 20, its of no use to us when the user wants to persist the "default value" since the "default value" may change over time. This is exactly analogous to the case of defining the resources configuration via xml. If you had a template with every xml element specified in there and persisted that, it would be a different value than if you just included the xml elements you wanted to specify, letting the server set the other ones as it saw fit.

Cheers

[starksm64]

"emuckenhuber" wrote:
Although i still think doing managedProperty.setValue(null); should not replace it with a default value.

Ok, so what should we use?

I think we are going to have to use the ManagedProperty.setRemoved(true) to indicate that a property has no value to be applied to the underlying metadata. If we don't want to persist the default values, I don't see that a default value at the ManagedProperty level has much meaning. It would not be equivalent a missing element on an xml configuration. I suppose we could treat it like that to ensure the uninitialized values are part of the management interface, and the persistent view would have a 'managed-property default=true' setting. The main problem I see with this vs using ManagedProperty.setRemoved(true) is having to update the ManagedProperty settings to reflect the proper default values.

A ManagedProperty.setValue(null) should only be mapping to either an actual null or an uninitialized value for a primitive. I agree with Emanuel that we can't effectively duplex the meaning of a null to indicate a missing configuration value.

[ips]

In the RHQ config GUI, we don't make a distinction between null values and unspecified/removed values; I think we figured that 99% of the time calling setValue(null) on an underlying managed resource would be equivalent to not calling the setter at all. That said, I can understand you wanting to make the distinction just to support all possible use cases. In any case, the API you describe will work just fine for us. I have a few questions though:

1) Will:

managedComponent.getProperties().remove("foo")

be semantically equivalent to:

managedComponent.getProperty("foo").setRemoved(true)

? (I think it should be)

2) Will managementView.updateComponent() throw an Exception if for a property (managedProperty.isMandatory() && managedProperty.isRemoved()) is true? I think it should, because not specifying a value for a mandatory property should be considered a client error. The other option would be having setRemoved(true) throw an Exception if the prop is mandatory.

3) I think primitive SimpleValues should not be allowed to have a value of null. They are primitive after all. Either updateComponent() could throw an Excpetion, or, to nip it in the bud, the impl of setValue() in SimpleValueSupport could be changed to:

public void setValue(Serializable value)
 {
 if (value == null && this.metatype.isPrimitive())
 throw new IllegalArgumentException("Primitive SimpleValues cannot have a value of null.");
 this.value = value;
 }

[starksm64]

"ips" wrote:

1) Will:

managedComponent.getProperties().remove("foo")

be semantically equivalent to:

managedComponent.getProperty("foo").setRemoved(true)

? (I think it should be)

Yes it should.

"ips" wrote:

2) Will managementView.updateComponent() throw an Exception if for a property (managedProperty.isMandatory() && managedProperty.isRemoved()) is true? I think it should, because not specifying a value for a mandatory property should be considered a client error. The other option would be having setRemoved(true) throw an Exception if the prop is mandatory.

I would say calling setRemoved(true) on a manadatory property is where the exception should be thrown. Right now that is not the case so an update to jboss-managed would be needed. We can have a duplicate check in the server updateComponent code to add that behavior now, and ensure the server enforces the expected behavior.

"ips" wrote:

3) I think primitive SimpleValues should not be allowed to have a value of null. They are primitive after all. Either updateComponent() could throw an Excpetion, or, to nip it in the bud, the impl of setValue() in SimpleValueSupport could be changed to:

public void setValue(Serializable value)
 {
 if (value == null && this.metatype.isPrimitive())
 throw new IllegalArgumentException("Primitive SimpleValues cannot have a value of null.");
 this.value = value;
 }

The latest jboss-managed 2.1.0.CR6 simply maps MetaValues with null values onto the corresponding primitive unintialized value when the MetaValue is unwrapped, so essentially there is no way a primitive will have a null value. I view this as akin to auto promotion of nulls so that one does not have to track whether a property is a primitive.

[ips]

"ips" wrote:

1) Will:

managedComponent.getProperties().remove("foo")

be semantically equivalent to:

managedComponent.getProperty("foo").setRemoved(true)

? (I think it should be)


Yes it should.

Then it could be argued that the setRemoved/isRemoved API on ManagedProperty is not truly needed. The only advantages I see of including the API are that it's a bit easier to document Javadoc-wise, and it's easier to implement the isMandatory check I described in my previous post.

The latest jboss-managed 2.1.0.CR6 simply maps MetaValues with null values onto the corresponding primitive uninitialized value when the MetaValue is unwrapped, so essentially there is no way a primitive will have a null value. I view this as akin to auto promotion of nulls so that one does not have to track whether a property is a primitive.

I don't love this auto-promotion, because I think many users will still try to use simpleValue.setValue(null) to remove/unspecify that property. For non-primitive SimpleValues, this isn't a big deal, because 99% of the time it will have the same ultimate effect on the managed resource. But for primitive SimpleValues, it will end up setting the primitive to the Java init default, which will often not be the same as the underlying managed resource's default for the properties.

[ccrouch]

The latest jboss-managed 2.1.0.CR6 simply maps MetaValues with null values onto the corresponding primitive unintialized value when the MetaValue is unwrapped, so essentially there is no way a primitive will have a null value. I view this as akin to auto promotion of nulls so that one does not have to track whether a property is a primitive.

I think this is fine, as long as we can support the following scenarios:

a) ManagedProperty wraps an int and the user does not want to give it a value (i.e. he's happy with whatever value the server default is e.g. 0, 20, 100).
The user sets unset=true in the console and then the plugin calls property.setRemoved(true).

b) ManagedProperty wraps an int and the user specifies no value.
The user sets unset=false in the console, but leaves the value blank. The plugin will do property.setValue(null) and PS will initialize the underlying value to 0, which is the value the console will see next time it queries the PS.
I see this as just the fallout from using int's in a components' managed api, if the component developer wants to support letting the user specify null, then they should use an Integer.

Cheers

[starksm64]

I think this is fine, as long as we can support the following scenarios:

a) ManagedProperty wraps an int and the user does not want to give it a value (i.e. he's happy with whatever value the server default is e.g. 0, 20, 100).
The user sets unset=true in the console and then the plugin calls property.setRemoved(true).

b) ManagedProperty wraps an int and the user specifies no value.
The user sets unset=false in the console, but leaves the value blank. The plugin will do property.setValue(null) and PS will initialize the underlying value to 0, which is the value the console will see next time it queries the PS.
I see this as just the fallout from using int's in a components' managed api, if the component developer wants to support letting the user specify null, then they should use an Integer.

Agreed. So the only issue is whether we make the default value part of the management api on the ManagedProperty so that its both documented what the defaults are, and there is consistent behavior between a primitive with an initializer (int x = 1) and @ManagmentProperty(defaultValue="1"). I don't see I'll have that working by tomorrow, so we'll have to talk about if this is worthwhile pursuing for CR1 vs other issues tomorrow.