Fixing handling of 'required' attributes with 'alternatives'

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Fixing handling of 'required' attributes with 'alternatives'

Brian Stansberry
tl;dr;

It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.

Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.

The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.

Long version

Harald and Claudio you guys are the main audience here. :)

For even longer version see description and comments on [1].

In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:

1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
2) @Deprecate setAllowNull and point to setRequired
3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.

Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”  

This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':

/subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
/core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER

The latter two are not exposed in the console so the issue really is the two transaction attributes.

I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.

Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.

[1] https://issues.jboss.org/browse/WFCORE-1556
[2] https://issues.jboss.org/browse/WFLY-6608
[3] https://issues.jboss.org/browse/WFLY-6607

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Harald Pehl
Thanks Brian for bringing this up. Everything which makes the metadata more consistent helps management client such as HAL. And as more and more forms are based on MBUI (generated based on the r-r-d metadata) this is even more important.

Right now we already have some logic behind the 'nillable' and 'alternatives' attributes to decide whether an attribute is required or not:

boolean required = attributeDescription.hasDefined("nillable") && !attributeDescription.get("nillable").asBoolean();
if (attributeDescription.hasDefined("alternatives") && !attributeDescription.get("alternatives").asList().isEmpty()) {
    required = false;
}

In HAL we use attribute descriptions to add new resources and to modify existing resources. For the former we rely on the 'request-properties' node of the add operation description. The latter uses the 'attributes' node of the r-r-d op. If we want to make changes, it's important to be consistent and apply them to both nodes. Right now these two nodes already have slightly different attributes: The 'request-properties' already contain a 'required' attribute whereas the 'attributes' don't. 

The proposal makes sense to me and the impact on HAL should be minimal. Some questions I have:

1. Will the metadata contain both 'nillable' and 'required'? With 'required' being the leading attribute and 'nillable' being deprecated but still there for backwards compability?

2. Will your proposal also cover the 'request-properties' for the add operation?


On Tue, Sep 6, 2016 at 3:10 PM, Brian Stansberry <[hidden email]> wrote:
tl;dr;

It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.

Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.

The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.

Long version

Harald and Claudio you guys are the main audience here. :)

For even longer version see description and comments on [1].

In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:

1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
2) @Deprecate setAllowNull and point to setRequired
3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.

Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”

This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':

/subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
/core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER

The latter two are not exposed in the console so the issue really is the two transaction attributes.

I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.

Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.

[1] https://issues.jboss.org/browse/WFCORE-1556
[2] https://issues.jboss.org/browse/WFLY-6608
[3] https://issues.jboss.org/browse/WFLY-6607

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev



--
---
Harald Pehl
JBoss by Red Hat
http://hpehl.info

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Brian Stansberry

> On Sep 6, 2016, at 11:58 AM, Harald Pehl <[hidden email]> wrote:
>
> Thanks Brian for bringing this up. Everything which makes the metadata more consistent helps management client such as HAL. And as more and more forms are based on MBUI (generated based on the r-r-d metadata) this is even more important.
>
> Right now we already have some logic behind the 'nillable' and 'alternatives' attributes to decide whether an attribute is required or not:
>
> boolean required = attributeDescription.hasDefined("nillable") && !attributeDescription.get("nillable").asBoolean();
> if (attributeDescription.hasDefined("alternatives") && !attributeDescription.get("alternatives").asList().isEmpty()) {
>     required = false;
> }
>

Ah, good. So my proposed change to how nillable is calculated shouldn’t change the final result you get above for your required variable. :) So once I do what I propose on the server side you can adapt to it when convenient.

> In HAL we use attribute descriptions to add new resources and to modify existing resources. For the former we rely on the 'request-properties' node of the add operation description. The latter uses the 'attributes' node of the r-r-d op. If we want to make changes, it's important to be consistent and apply them to both nodes. Right now these two nodes already have slightly different attributes: The 'request-properties' already contain a 'required' attribute whereas the 'attributes' don't.

Yes, this inconsistency is part of the overall task of WFCORE-1556. The actual metadata we generate doesn’t comply with the spec in [a] and [b] and then the spec itself is inconsistent between those two sections in how it deals with “required” vs “nillable”. And there’s no reason for that.

[a] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanAttribute
[b] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanOperationParameterorReturnValue

>
> The proposal makes sense to me and the impact on HAL should be minimal. Some questions I have:
>
> 1. Will the metadata contain both 'nillable' and 'required'? With 'required' being the leading attribute and 'nillable' being deprecated but still there for backwards compability?
>

It will contain both. I wouldn’t characterize either as leading or deprecated. Rather they have different functions:

required — indicates the attribute/parameter represents something that must be configured in some way. But configuring one of the ‘alternatives’ suffices.
nillable — indicates that attribute/parameter may have an undefined value for some reason

So nillable is useful to a client simply wanting to know whether it needs to deal with ‘undefined’. A more sophisticated client would use ‘required’ plus ‘alternatives’.

> 2. Will your proposal also cover the 'request-properties' for the add operation?
>

Yes, they will be made consistent, with all attribute and parameter descriptions exposing the same metadata fields with the same conceptual meaning.

In terms of the server side implementation, for almost all add operations, the same AttributeDefinition instance is used for generating both the attribute description and the add parameter description. And for all parameter descriptions we use the same AttributeDefinition classes that we use for resource attributes, so the behavior we put in the AD class will apply to both.

>
> On Tue, Sep 6, 2016 at 3:10 PM, Brian Stansberry <[hidden email]> wrote:
> tl;dr;
>
> It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.
>
> Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.
>
> The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.
>
> Long version
>
> Harald and Claudio you guys are the main audience here. :)
>
> For even longer version see description and comments on [1].
>
> In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:
>
> 1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
> 2) @Deprecate setAllowNull and point to setRequired
> 3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
> 4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.
>
> Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”
>
> This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':
>
> /subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
> /core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER
>
> The latter two are not exposed in the console so the issue really is the two transaction attributes.
>
> I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.
>
> Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.
>
> [1] https://issues.jboss.org/browse/WFCORE-1556
> [2] https://issues.jboss.org/browse/WFLY-6608
> [3] https://issues.jboss.org/browse/WFLY-6607
>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
>
>
> --
> ---
> Harald Pehl
> JBoss by Red Hat
> http://hpehl.info

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Brian Stansberry
PR for this: https://github.com/wildfly/wildfly-core/pull/1781

> On Sep 6, 2016, at 1:56 PM, Brian Stansberry <[hidden email]> wrote:
>
>>
>> On Sep 6, 2016, at 11:58 AM, Harald Pehl <[hidden email]> wrote:
>>
>> Thanks Brian for bringing this up. Everything which makes the metadata more consistent helps management client such as HAL. And as more and more forms are based on MBUI (generated based on the r-r-d metadata) this is even more important.
>>
>> Right now we already have some logic behind the 'nillable' and 'alternatives' attributes to decide whether an attribute is required or not:
>>
>> boolean required = attributeDescription.hasDefined("nillable") && !attributeDescription.get("nillable").asBoolean();
>> if (attributeDescription.hasDefined("alternatives") && !attributeDescription.get("alternatives").asList().isEmpty()) {
>>    required = false;
>> }
>>
>
> Ah, good. So my proposed change to how nillable is calculated shouldn’t change the final result you get above for your required variable. :) So once I do what I propose on the server side you can adapt to it when convenient.
>
>> In HAL we use attribute descriptions to add new resources and to modify existing resources. For the former we rely on the 'request-properties' node of the add operation description. The latter uses the 'attributes' node of the r-r-d op. If we want to make changes, it's important to be consistent and apply them to both nodes. Right now these two nodes already have slightly different attributes: The 'request-properties' already contain a 'required' attribute whereas the 'attributes' don't.
>
> Yes, this inconsistency is part of the overall task of WFCORE-1556. The actual metadata we generate doesn’t comply with the spec in [a] and [b] and then the spec itself is inconsistent between those two sections in how it deals with “required” vs “nillable”. And there’s no reason for that.
>
> [a] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanAttribute
> [b] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanOperationParameterorReturnValue
>
>>
>> The proposal makes sense to me and the impact on HAL should be minimal. Some questions I have:
>>
>> 1. Will the metadata contain both 'nillable' and 'required'? With 'required' being the leading attribute and 'nillable' being deprecated but still there for backwards compability?
>>
>
> It will contain both. I wouldn’t characterize either as leading or deprecated. Rather they have different functions:
>
> required — indicates the attribute/parameter represents something that must be configured in some way. But configuring one of the ‘alternatives’ suffices.
> nillable — indicates that attribute/parameter may have an undefined value for some reason
>
> So nillable is useful to a client simply wanting to know whether it needs to deal with ‘undefined’. A more sophisticated client would use ‘required’ plus ‘alternatives’.
>
>> 2. Will your proposal also cover the 'request-properties' for the add operation?
>>
>
> Yes, they will be made consistent, with all attribute and parameter descriptions exposing the same metadata fields with the same conceptual meaning.
>
> In terms of the server side implementation, for almost all add operations, the same AttributeDefinition instance is used for generating both the attribute description and the add parameter description. And for all parameter descriptions we use the same AttributeDefinition classes that we use for resource attributes, so the behavior we put in the AD class will apply to both.
>
>>
>> On Tue, Sep 6, 2016 at 3:10 PM, Brian Stansberry <[hidden email]> wrote:
>> tl;dr;
>>
>> It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.
>>
>> Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.
>>
>> The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.
>>
>> Long version
>>
>> Harald and Claudio you guys are the main audience here. :)
>>
>> For even longer version see description and comments on [1].
>>
>> In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:
>>
>> 1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
>> 2) @Deprecate setAllowNull and point to setRequired
>> 3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
>> 4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.
>>
>> Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”
>>
>> This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':
>>
>> /subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
>> /core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER
>>
>> The latter two are not exposed in the console so the issue really is the two transaction attributes.
>>
>> I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.
>>
>> Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.
>>
>> [1] https://issues.jboss.org/browse/WFCORE-1556
>> [2] https://issues.jboss.org/browse/WFLY-6608
>> [3] https://issues.jboss.org/browse/WFLY-6607
>>
>> --
>> Brian Stansberry
>> Manager, Senior Principal Software Engineer
>> JBoss by Red Hat
>>
>>
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>>
>>
>>
>> --
>> ---
>> Harald Pehl
>> JBoss by Red Hat
>> http://hpehl.info
>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Brian Stansberry
In reply to this post by Brian Stansberry

> On Sep 12, 2016, at 7:44 AM, Heiko W.Rupp <[hidden email]> wrote:
>
> On 6 Sep 2016, at 15:10, Brian Stansberry wrote:
>
>> It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal.
>
> Would this also include cases where e.g. a logger has an
> attribute 'mode' which could be one of file, console, rsyslog
> and then dependent attributes that are only usable with
> one of the modes. Something like filename for 'file',
> none for 'console' and 'remote-socket-binding' for 'rsyslog' ?
> Both 'filename' and 'remote-socket-binding' would be
> required only for their mode (and even forbidden otherwise).

No, that kind of thing is not covered by this work. We have ‘requires’ metadata for an attribute that names other attributes that must be set if the given attribute is set, but that’s just a relationship based on defined vs undefined, not on any particular defined value. There would need to be some sort of new metadata added to cover what you describe. Perhaps something like

“requires-if => [
    {
       “value” => true,
       “attributes” => [“another”, “athird”]
    },
    {
       “value” => false,
       “attributes” => [“afourth”, “afifth”]
    },
  ]

Note that for something like that we couldn’t support it for attributes that allow expressions, as the validation of the constraint would need to happen before we’re able to accurately resolve expressions. But for enum attributes like your example I don’t think that would be an issue. I don’t imagine using an expression to control an enum would be a typical thing.

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Brian Stansberry
In reply to this post by Brian Stansberry
Hi Harald,

This PR is merged so this will be there once this week’s core release is merged into full (probably by Monday).

> On Sep 7, 2016, at 3:59 PM, Brian Stansberry <[hidden email]> wrote:
>
> PR for this: https://github.com/wildfly/wildfly-core/pull/1781
>
>> On Sep 6, 2016, at 1:56 PM, Brian Stansberry <[hidden email]> wrote:
>>
>>>
>>> On Sep 6, 2016, at 11:58 AM, Harald Pehl <[hidden email]> wrote:
>>>
>>> Thanks Brian for bringing this up. Everything which makes the metadata more consistent helps management client such as HAL. And as more and more forms are based on MBUI (generated based on the r-r-d metadata) this is even more important.
>>>
>>> Right now we already have some logic behind the 'nillable' and 'alternatives' attributes to decide whether an attribute is required or not:
>>>
>>> boolean required = attributeDescription.hasDefined("nillable") && !attributeDescription.get("nillable").asBoolean();
>>> if (attributeDescription.hasDefined("alternatives") && !attributeDescription.get("alternatives").asList().isEmpty()) {
>>>   required = false;
>>> }
>>>
>>
>> Ah, good. So my proposed change to how nillable is calculated shouldn’t change the final result you get above for your required variable. :) So once I do what I propose on the server side you can adapt to it when convenient.
>>
>>> In HAL we use attribute descriptions to add new resources and to modify existing resources. For the former we rely on the 'request-properties' node of the add operation description. The latter uses the 'attributes' node of the r-r-d op. If we want to make changes, it's important to be consistent and apply them to both nodes. Right now these two nodes already have slightly different attributes: The 'request-properties' already contain a 'required' attribute whereas the 'attributes' don't.
>>
>> Yes, this inconsistency is part of the overall task of WFCORE-1556. The actual metadata we generate doesn’t comply with the spec in [a] and [b] and then the spec itself is inconsistent between those two sections in how it deals with “required” vs “nillable”. And there’s no reason for that.
>>
>> [a] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanAttribute
>> [b] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanOperationParameterorReturnValue
>>
>>>
>>> The proposal makes sense to me and the impact on HAL should be minimal. Some questions I have:
>>>
>>> 1. Will the metadata contain both 'nillable' and 'required'? With 'required' being the leading attribute and 'nillable' being deprecated but still there for backwards compability?
>>>
>>
>> It will contain both. I wouldn’t characterize either as leading or deprecated. Rather they have different functions:
>>
>> required — indicates the attribute/parameter represents something that must be configured in some way. But configuring one of the ‘alternatives’ suffices.
>> nillable — indicates that attribute/parameter may have an undefined value for some reason
>>
>> So nillable is useful to a client simply wanting to know whether it needs to deal with ‘undefined’. A more sophisticated client would use ‘required’ plus ‘alternatives’.
>>
>>> 2. Will your proposal also cover the 'request-properties' for the add operation?
>>>
>>
>> Yes, they will be made consistent, with all attribute and parameter descriptions exposing the same metadata fields with the same conceptual meaning.
>>
>> In terms of the server side implementation, for almost all add operations, the same AttributeDefinition instance is used for generating both the attribute description and the add parameter description. And for all parameter descriptions we use the same AttributeDefinition classes that we use for resource attributes, so the behavior we put in the AD class will apply to both.
>>
>>>
>>> On Tue, Sep 6, 2016 at 3:10 PM, Brian Stansberry <[hidden email]> wrote:
>>> tl;dr;
>>>
>>> It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.
>>>
>>> Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.
>>>
>>> The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.
>>>
>>> Long version
>>>
>>> Harald and Claudio you guys are the main audience here. :)
>>>
>>> For even longer version see description and comments on [1].
>>>
>>> In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:
>>>
>>> 1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
>>> 2) @Deprecate setAllowNull and point to setRequired
>>> 3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
>>> 4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.
>>>
>>> Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”
>>>
>>> This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':
>>>
>>> /subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
>>> /core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER
>>>
>>> The latter two are not exposed in the console so the issue really is the two transaction attributes.
>>>
>>> I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.
>>>
>>> Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.
>>>
>>> [1] https://issues.jboss.org/browse/WFCORE-1556
>>> [2] https://issues.jboss.org/browse/WFLY-6608
>>> [3] https://issues.jboss.org/browse/WFLY-6607
>>>
>>> --
>>> Brian Stansberry
>>> Manager, Senior Principal Software Engineer
>>> JBoss by Red Hat
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> wildfly-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>>>
>>>
>>>
>>> --
>>> ---
>>> Harald Pehl
>>> JBoss by Red Hat
>>> http://hpehl.info
>>
>> --
>> Brian Stansberry
>> Manager, Senior Principal Software Engineer
>> JBoss by Red Hat
>>
>>
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fixing handling of 'required' attributes with 'alternatives'

Harald Pehl
Thanks for the info, Brian. I'll check the console with the new server-side impl (should affect the transaction attributes only, but will also do some more testing)

On Wed, Nov 23, 2016 at 12:50 AM, Brian Stansberry <[hidden email]> wrote:
Hi Harald,

This PR is merged so this will be there once this week’s core release is merged into full (probably by Monday).

> On Sep 7, 2016, at 3:59 PM, Brian Stansberry <[hidden email]> wrote:
>
> PR for this: https://github.com/wildfly/wildfly-core/pull/1781
>
>> On Sep 6, 2016, at 1:56 PM, Brian Stansberry <[hidden email]> wrote:
>>
>>>
>>> On Sep 6, 2016, at 11:58 AM, Harald Pehl <[hidden email]> wrote:
>>>
>>> Thanks Brian for bringing this up. Everything which makes the metadata more consistent helps management client such as HAL. And as more and more forms are based on MBUI (generated based on the r-r-d metadata) this is even more important.
>>>
>>> Right now we already have some logic behind the 'nillable' and 'alternatives' attributes to decide whether an attribute is required or not:
>>>
>>> boolean required = attributeDescription.hasDefined("nillable") && !attributeDescription.get("nillable").asBoolean();
>>> if (attributeDescription.hasDefined("alternatives") && !attributeDescription.get("alternatives").asList().isEmpty()) {
>>>   required = false;
>>> }
>>>
>>
>> Ah, good. So my proposed change to how nillable is calculated shouldn’t change the final result you get above for your required variable. :) So once I do what I propose on the server side you can adapt to it when convenient.
>>
>>> In HAL we use attribute descriptions to add new resources and to modify existing resources. For the former we rely on the 'request-properties' node of the add operation description. The latter uses the 'attributes' node of the r-r-d op. If we want to make changes, it's important to be consistent and apply them to both nodes. Right now these two nodes already have slightly different attributes: The 'request-properties' already contain a 'required' attribute whereas the 'attributes' don't.
>>
>> Yes, this inconsistency is part of the overall task of WFCORE-1556. The actual metadata we generate doesn’t comply with the spec in [a] and [b] and then the spec itself is inconsistent between those two sections in how it deals with “required” vs “nillable”. And there’s no reason for that.
>>
>> [a] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanAttribute
>> [b] https://docs.jboss.org/author/display/WFLY/Admin+Guide#AdminGuide-DescriptionofanOperationParameterorReturnValue
>>
>>>
>>> The proposal makes sense to me and the impact on HAL should be minimal. Some questions I have:
>>>
>>> 1. Will the metadata contain both 'nillable' and 'required'? With 'required' being the leading attribute and 'nillable' being deprecated but still there for backwards compability?
>>>
>>
>> It will contain both. I wouldn’t characterize either as leading or deprecated. Rather they have different functions:
>>
>> required — indicates the attribute/parameter represents something that must be configured in some way. But configuring one of the ‘alternatives’ suffices.
>> nillable — indicates that attribute/parameter may have an undefined value for some reason
>>
>> So nillable is useful to a client simply wanting to know whether it needs to deal with ‘undefined’. A more sophisticated client would use ‘required’ plus ‘alternatives’.
>>
>>> 2. Will your proposal also cover the 'request-properties' for the add operation?
>>>
>>
>> Yes, they will be made consistent, with all attribute and parameter descriptions exposing the same metadata fields with the same conceptual meaning.
>>
>> In terms of the server side implementation, for almost all add operations, the same AttributeDefinition instance is used for generating both the attribute description and the add parameter description. And for all parameter descriptions we use the same AttributeDefinition classes that we use for resource attributes, so the behavior we put in the AD class will apply to both.
>>
>>>
>>> On Tue, Sep 6, 2016 at 3:10 PM, Brian Stansberry <[hidden email]> wrote:
>>> tl;dr;
>>>
>>> It’s not an uncommon thing to have a management attribute A that is required; i.e. must be set, but also has an alternative attribute B, where setting B satisfies the requirement for A, making an undefined value for A legal. We’re not handling that correctly and I want to fix it.[1] But fixing it will require some coordination with the HAL console.
>>>
>>> Right now the way AttributeDefinition building works it’s not practical to declare that an attribute is required but only if no alternative is set. So instead attributes are declared as if they aren’t really required. This leads to less than helpful input validation, e.g. [2] and [3], since the attribute definition is imprecise.
>>>
>>> The change I’d like to make will alter the read-resource-description output for 4 attributes, changing the value of the ‘nillable’ description from ‘false’ to ‘true’ so I want to coordinate that with the console team.
>>>
>>> Long version
>>>
>>> Harald and Claudio you guys are the main audience here. :)
>>>
>>> For even longer version see description and comments on [1].
>>>
>>> In a nutshell, if devs set the ‘allowNull’ property on an AttributeDefinition to ‘true’, the r-r-d output for the attribute has “nillable” => true. But there is no way to say “allowNull but only if an alternative is set.” So people are setting “allowNull” to true even if the attribute should be set in the absence of alternatives. And HAL has no metadata available to it to tell users they *must* set one of the alternatives. So I want to:
>>>
>>> 1) Add a setRequired(boolean) method to the AD builders where the fact that it means “must be defined if no alternative is defined” is explicitly declared
>>> 2) @Deprecate setAllowNull and point to setRequired
>>> 3) Clarify the meaning of setAllowNull(true) as being the same as setRequired(false)
>>> 4) Change the builder ‘allowNull’ constructor param name to “optional” and document its meaning as “allowing undefined values even in the absence of defined alternatives”.  I could call the param ‘notRequired’ which is clearer in meaning but odd.
>>>
>>> Then I will add a new ‘required’ metadata field to the r-r-d output and change the impl of the existing ’nillable’ metadata to a logical “!required || (alternatives != null && alternatives.length > 0)”
>>>
>>> This will result in a change in the ‘nillable’ value for 4 attributes in the WildFly model from ‘false’ to ’true':
>>>
>>> /subsystem=transactions PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
>>> /core-service=management/security-realm=*/authenticaton=ldap USERNAME_FILTER and ADVANCED_FILTER
>>>
>>> The latter two are not exposed in the console so the issue really is the two transaction attributes.
>>>
>>> I haven’t checked other subsystems in things like Teiid or JDG, but if there are only 4 in all of WildFly I doubt there are many.
>>>
>>> Also, if people start using the new behavior to correct problems like [2] and [3] people may expect the console to understand and reflect the concept of “required but only if there is no alternative’.
>>>
>>> [1] https://issues.jboss.org/browse/WFCORE-1556
>>> [2] https://issues.jboss.org/browse/WFLY-6608
>>> [3] https://issues.jboss.org/browse/WFLY-6607
>>>
>>> --
>>> Brian Stansberry
>>> Manager, Senior Principal Software Engineer
>>> JBoss by Red Hat
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> wildfly-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>>>
>>>
>>>
>>> --
>>> ---
>>> Harald Pehl
>>> JBoss by Red Hat
>>> http://hpehl.info
>>
>> --
>> Brian Stansberry
>> Manager, Senior Principal Software Engineer
>> JBoss by Red Hat
>>
>>
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat






--
Harald Pehl
JBoss by Red Hat
http://hpehl.info

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Loading...