[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry commented on WFCORE-1556:
------------------------------------------
I don't think the AD can ever be responsible for validating "required but not if alternatives are present". The AD is asked to validate (i.e. a call to validateAndSet) during execution of a particular step, but that step may be part of a composite, and it's only when the composite is complete that it's ok to validate the resulting model. For example the composite may have two write-attribute steps, one of which sets alternative a and the other undefines alternative b. The overall model state will never be valid after just one step.
There are other situations where things may be in flux, e.g. validating during parsing when some alternatives haven't been parsed yet.
In theory we could be lenient in some situations like parsing and write-attribute and stricter in others, like an add op, but that requires changes to AD that some OSHs may not use, it's complex and probably buggy, and it still doesn't allow for weird corner cases like a composite with a broken add step followed by a correcting write-attribute step.
It seems like we should be able to get rid of the need for setValidating(false) though and just detect the combination of required + alternatives and be lenient.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry commented on WFCORE-1556:
------------------------------------------
Another note to self: The "setValidateNull(false)" stuff is quite the black art. Here's one of the 3 uses of it in core and full, all of which are in the same LdapAuthenticationResourceDefinition class:
{code}
public static final SimpleAttributeDefinition USERNAME_FILTER = new SimpleAttributeDefinitionBuilder(ModelDescriptionConstants.USERNAME_ATTRIBUTE, ModelType.STRING, false)
.setXmlName("attribute")
.setAlternatives(ModelDescriptionConstants.ADVANCED_FILTER)
.setValidator(new StringLengthValidator(1, Integer.MAX_VALUE, true, true))
.setValidateNull(false)
.setAllowExpression(true)
.setFlags(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES).build();
{code}
The builder constructor passes "false" as the "allowNull" param, so this attribute is required. But it also has alternatives. This combination would normally fail to validate if the model has this attribute undefined but an alternative defined, even though such a combination is correct.
To get around this, the definition does two things:
1) It sets up a validator that has the "allowNull" field set to "true", meaning the validator will not reject undefined.
{code}
.setValidator(new StringLengthValidator(1, Integer.MAX_VALUE, true, true))
{code}
It does this even though the builder constructor says "allowNull=false".
2) It adds ".setValidateNull(false)". What this does is it turns off the logic added in AS7-3857 that resulted in the AD itself validating undefined values instead of relying on the provided validator to do this. Because this logic is turned off, the AD will not reject undefined, and instead will allow the validator to validate, and it is configured to accept undefined.
It works but is very kludgy. It's not surprising that out of hundreds of attributes that declare alternatives, a great many of which are probably required if no alternative is present, only 2 use this technique.
Also, it only kind of works, since all this does is cause the AD to accept undefined. It doesn't lead to proper validation of the overall model, i.e. a check that one of the alternatives is defined. To do that we had to add to the operation execution logic special validation at the end of Stage.MODEL execution.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry edited comment on WFCORE-1556 at 5/18/16 5:18 PM:
-------------------------------------------------------------------
I've done some analysis of cases where AbstractAttributeDefinitionBuilder.setAlternatives()/addAlternatives() are used in conjunction with "allowNull == false". These are cases where converting "allowNull" to "!required" and definining 'nillable' as "nillable = !required || alternatives != null" will produce a different result in the attribute descriptions from the current behavior of "nillable = allowNull".
I find:
TransactionSubsystemRootResourceDefinition PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
LdapAuthenticationResourceDefinition USERNAME_FILTER and ADVANCED_FILTER
org.jboss.as.server.mgmt.NativeManagementResourceDefinition SOCKET_BINDING
The last one appears to be bogus as the declared "alternative" is not a legal alternative in a server config.
The LdapAuthenticationResourceDefinition ones work because the relevant ADs also have "setValidateNull(false)" set, so the validator is not complaining about undefined values.
The TransactionSubsystemRootResourceDefinition ones work because the relevant OSHs use custom logic to avoid the normal validation checks.
The issue here a client using the 'nillable' value would now see 'true' instead of 'false' and may react incorrectly.
was (Author: brian.stansberry):
I've done some analysis of cases where AbstractAttributeDefinitionBuilder.setAlternatives()/addAlternatives() are used in conjunction with "allowNull == false". These are cases where converting "allowNull" to "!required" and definining 'nillable' as "nillable = !required || alternatives != null" will produce a different result in the attribute descriptions from the current behavior of "nillable = allowNull".
I find:
TransactionSubsystemRootResourceDefinition PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
LdapAuthenticationResourceDefinition USERNAME_FILTER and ADVANCED_FILTER
org.jboss.as.server.mgmt.NativeManagementResourceDefinition SOCKET_BINDING
The last one appears to be bogus as the declared "alternative" is not a legal alternative in a server config.
The LdapAuthenticationResourceDefinition ones work because the relevant ADs also have "setValidateNull(true)" set, so the validator is not complaining about undefined values.
The TransactionSubsystemRootResourceDefinition ones work because the relevant OSHs use custom logic to avoid the normal validation checks.
The issue here a client using the 'nillable' value would now see 'true' instead of 'false' and may react incorrectly.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry edited comment on WFCORE-1556 at 5/18/16 5:18 PM:
-------------------------------------------------------------------
Note to self: the "setValidateNull(false)" stuff came in as part of my work on AS7-3857 and reflects an earlier effort on how to deal with the fact that a ParameterValidator can't by itself handle the fact that an otherwise required attribute may be undefined due to the presence of an alternative.
was (Author: brian.stansberry):
Note to self: the "setValidateNull(true)" stuff came in as part of my work on AS7-3857 and reflects an earlier effort on how to deal with the fact that a ParameterValidator can't by itself handle the fact that an otherwise required attribute may be undefined due to the presence of an alternative.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1558) read-children-names on server-group deployments returns empty list, when it should just DTRT and return a list
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1558?page=com.atlassian.jira.plugi... ]
Brian Stansberry commented on WFCORE-1558:
------------------------------------------
[~harald.pehl] The issue is the read-children-names and read-children-resources handlers are not based on AbstractMultiTargetHandler. So xyz=* shouldn't work at all for them, but instead of fixing this by having them fail, we might as well make them based on AbstractMultiTargetHandler and give them the better behavior.
> read-children-names on server-group deployments returns empty list, when it should just DTRT and return a list
> --------------------------------------------------------------------------------------------------------------
>
> Key: WFCORE-1558
> URL: https://issues.jboss.org/browse/WFCORE-1558
> Project: WildFly Core
> Issue Type: Bug
> Reporter: Ken Wills
> Assignee: Ken Wills
>
> Using a wildcard :read-children-names on deployments returns an empty list, when it should probably just return an error, or the right answer ;)
> See OperationContextImpl.readResourceFromRoot for /server-group=*
> The OSHs are based on AbstractMultiTargetHandler.
> Output should be something like:
> {
> "outcome" => "success",
> "result" => [
> {
> "address" => [("server-group" => "main-server-group")],
> "outcome" => "success",
> "result" => ["simple-servlet.war"]
> }
> },
> {
> "address" => [("server-group" => "other-server-group")],
> "outcome" => "success",
> "result" =>
> "result" => ["simple-servlet.war"]
> }
> ]
> }
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry commented on WFCORE-1556:
------------------------------------------
Note to self: the "setValidateNull(true)" stuff came in as part of my work on AS7-3857 and reflects an earlier effort on how to deal with the fact that a ParameterValidator can't by itself handle the fact that an otherwise required attribute may be undefined due to the presence of an alternative.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry edited comment on WFCORE-1556 at 5/18/16 3:04 PM:
-------------------------------------------------------------------
I've done some analysis of cases where AbstractAttributeDefinitionBuilder.setAlternatives()/addAlternatives() are used in conjunction with "allowNull == false". These are cases where converting "allowNull" to "!required" and definining 'nillable' as "nillable = !required || alternatives != null" will produce a different result in the attribute descriptions from the current behavior of "nillable = allowNull".
I find:
TransactionSubsystemRootResourceDefinition PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
LdapAuthenticationResourceDefinition USERNAME_FILTER and ADVANCED_FILTER
org.jboss.as.server.mgmt.NativeManagementResourceDefinition SOCKET_BINDING
The last one appears to be bogus as the declared "alternative" is not a legal alternative in a server config.
The LdapAuthenticationResourceDefinition ones work because the relevant ADs also have "setValidateNull(true)" set, so the validator is not complaining about undefined values.
The TransactionSubsystemRootResourceDefinition ones work because the relevant OSHs use custom logic to avoid the normal validation checks.
The issue here a client using the 'nillable' value would now see 'true' instead of 'false' and may react incorrectly.
was (Author: brian.stansberry):
I've done some analysis of cases where AbstractAttributeDefinitionBuilder.setAlternatives()/addAlternatives() are used in conjunction with "allowNull == false". These are cases where converting "allowNull" to "!required" and definining 'nillable' as "nillable = !required || alternatives != null" will produce a different result in the attribute descriptions from the current behavior of "nillable = allowNull".
I find:
TransactionSubsystemRootResourceDefinition PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
LdapAuthenticationResourceDefinition USERNAME_FILTER and ADVANCED_FILTER
org.jboss.as.server.mgmt.NativeManagementResourceDefinition SOCKET_BINDING
The last one appears to be bogus as the declared "alternative" is not a legal alternative in a server config.
The LdapAuthenticationResourceDefinition ones work because the relevant ADs also have "setValidateNull(true)" set, so the validator is not complaining about undefined values.
The TransactionSubsystemRootResourceDefinition ones work because the relevant OSHs use custom logic to avoid the normal validation checks.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFCORE-1556) Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
by Brian Stansberry (JIRA)
[ https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi... ]
Brian Stansberry commented on WFCORE-1556:
------------------------------------------
I've done some analysis of cases where AbstractAttributeDefinitionBuilder.setAlternatives()/addAlternatives() are used in conjunction with "allowNull == false". These are cases where converting "allowNull" to "!required" and definining 'nillable' as "nillable = !required || alternatives != null" will produce a different result in the attribute descriptions from the current behavior of "nillable = allowNull".
I find:
TransactionSubsystemRootResourceDefinition PROCESS_ID_SOCKET_BINDING and PROCESS_ID_UUID
LdapAuthenticationResourceDefinition USERNAME_FILTER and ADVANCED_FILTER
org.jboss.as.server.mgmt.NativeManagementResourceDefinition SOCKET_BINDING
The last one appears to be bogus as the declared "alternative" is not a legal alternative in a server config.
The LdapAuthenticationResourceDefinition ones work because the relevant ADs also have "setValidateNull(true)" set, so the validator is not complaining about undefined values.
The TransactionSubsystemRootResourceDefinition ones work because the relevant OSHs use custom logic to avoid the normal validation checks.
> Poor handling of 'required', 'nillable' and 'alternatives' in AttributeDefinition
> ---------------------------------------------------------------------------------
>
> Key: WFCORE-1556
> URL: https://issues.jboss.org/browse/WFCORE-1556
> Project: WildFly Core
> Issue Type: Bug
> Components: Domain Management
> Reporter: Brian Stansberry
>
> The handling of the notions of 'require'd and 'nillable' don't comply with the specs in https://docs.jboss.org/author/display/WFLY10/Description+of+the+Managemen..., particularly the "Description of an Attribute" part, where the 'required' and 'alternatives' fields are well described, with their relationship spelled out, while 'nillable' doesn't appear at all. Then in "Description of an Operation Parameter or Return Value" nillable is specified, although I think those descriptions could be tightened up in general.
> The read-resource-description output for an attribute doesn't show "required" at all.
> After a fair bit of thinking, I've finally recalled why the two separate notions of "required" and "nillable" exist in the first place.
> The "required" and "alternatives" pieces go together. Is something required? And then if it is required, an alternative satisfies. So you can have two attributes/params, both required, but they are alternatives so one is defined and the other is not. And this is an ok thing.
> And then 'nillable' comes in to help clients understand in a simple way whether they need to account for the possibility an undefined value. Basically:
> nillable = !required || alternatives != null
> Right now, nillable is implemented as
> nillable = !required
> There are a number of problems I see with AttributeDefinition:
> 1) We don't output the 'required' metadata unless it's an operation param being described. This is contrary to spec. However we we shouldn't fix this unless we can have meaningful values for 'required', ones where the value can be true but the attribute/param can still have an undefined value, due to an alternative being present. If we can't fix that, there's no point outputting required as it's just redundant with what we output for 'nillable'.
> 2) We do output the 'nillable' metadata, even for attribute descriptions, where it isn't in the spec. But in this case I think we change the spec, as dropping nillable would be an incompatible change.
> 3) We don't properly validate the "required but has alternatives case." This can't be validated solely with ParameterValidator impls as those only see a single attribute value and don't have contextual information about other attributes/params. To get around this limitation, devs are saying that attributes with alternatives "allowNull" which is equivalent to saying they are not required. But they are required! So I think a fix for this will require AttributeDefinition itself to validate the overall resource model or operation object, and skip calling the ParameterValidator if the attribute/param is undefined and an alternative is defined.
> 4) AttributeDefinition and AbstractAttributeDefinitionBuilder unfortunately have a getter/setter/constructor param for property "allowNull" instead of a property named "required". This is unfortunate because "allowNull" intuitively maps to "nillable", but "required" is a much more useful thing to set. The value of "nillable" can be derived from a "required" setting and an "alternatives" setting, but the value of required cannot be so derived.
> I think a fix for this would probably start from 4), deprecating setAllowNull, adding get/setRequired and changing the meaning of the AD(Builder) constructor param to "required". The effect of this would be that all current code setting "allowNull" would now be setting a new "required" field. This should be a non-breaking change as in current code that's the effect anyway.
> Next would be to fix 3), by changing how AD validates.
> Next would be to change the metadata we output, such that "required" is always present and the "nillable" value is !required || alternatives != null. And change the spec accordingly.
> Last, in a separate task, would be to find attributes that were configuring "allowNull = true" solely to allow validation to pass when alternatives are present, and fix them to say "required=false".
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFLY-6626) Add log message indicating disabled <distributable/> flag from web-fragments
by Aaron Ogburn (JIRA)
Aaron Ogburn created WFLY-6626:
----------------------------------
Summary: Add log message indicating disabled <distributable/> flag from web-fragments
Key: WFLY-6626
URL: https://issues.jboss.org/browse/WFLY-6626
Project: WildFly
Issue Type: Enhancement
Components: Web (Undertow)
Affects Versions: 10.0.0.Final
Reporter: Aaron Ogburn
Assignee: Stuart Douglas
Attachments: undistributable.war
If a web-fragment doesn't have distributable set, then replication is not enabled. This happens silently so it may be confusing to some users that are expecting replication to be enabled when they have set <distributable/> in their WEB-INF/web.xml.
Other third party libraries they included may have web-fragments without distributable set so it'd be nice to log a notification when this happens so that users are easily informed why replication was not enabled as they may have expected.
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months
[JBoss JIRA] (WFLY-6626) Add log message indicating disabled <distributable/> flag from web-fragments
by Aaron Ogburn (JIRA)
[ https://issues.jboss.org/browse/WFLY-6626?page=com.atlassian.jira.plugin.... ]
Aaron Ogburn commented on WFLY-6626:
------------------------------------
Attached a simple app with an undistributable web-fragment.
> Add log message indicating disabled <distributable/> flag from web-fragments
> ----------------------------------------------------------------------------
>
> Key: WFLY-6626
> URL: https://issues.jboss.org/browse/WFLY-6626
> Project: WildFly
> Issue Type: Enhancement
> Components: Web (Undertow)
> Affects Versions: 10.0.0.Final
> Reporter: Aaron Ogburn
> Assignee: Stuart Douglas
> Attachments: undistributable.war
>
>
> If a web-fragment doesn't have distributable set, then replication is not enabled. This happens silently so it may be confusing to some users that are expecting replication to be enabled when they have set <distributable/> in their WEB-INF/web.xml.
> Other third party libraries they included may have web-fragments without distributable set so it'd be nice to log a notification when this happens so that users are easily informed why replication was not enabled as they may have expected.
--
This message was sent by Atlassian JIRA
(v6.4.11#64026)
9 years, 11 months