[
https://issues.jboss.org/browse/WFCORE-1556?page=com.atlassian.jira.plugi...
]
Chao Wang commented on WFCORE-1556:
-----------------------------------
My apologies about the missing context. I was checking the related one WFLY-6607 and see
several failures after applying setRequired() on some attributes.
First, {{connectors}} needs to set required to true in broadcast-group definition. After
that, I have MessagingActiveMQSubsystem_1_X_TestCase failed due to missing {{connectors}}
of {{broadcast-group}} in test configuration file example
[
subsystem_1_0.xml|https://github.com/wildfly/wildfly/blob/master/messagin...].
I believe the XML file in test needs to add proper {{connectors}} value after it becomes
"required". In this case, no need for transformer, because the XML is incomplete
on {{broadcast-group}} attributes.
Secondly, {{jgroups-channel}} and {{socket-binding}} should be set to required true and
are alternatives for each other, this makes legacy messaging MigrateTestCase fails, In
[
subsystem_migration.xml|https://github.com/wildfly/wildfly/blob/master/le...].
Besides the missing {{connectors}} attribute in {{broadcast-group}}, the alternative
relations are also changed.
{{socket-binding}} previously has alternatives list "group-address",
"group-port", "local-bind-address", "local-bind-port",
"jgroups-stack", "jgroups-channel". First four are deprecated, but
they are still valid configuration as first example in [18.15.2.1. User Datagram Protocol
(UDP) Broadcast
Group|https://access.redhat.com/documentation/en-US/JBoss_Enterprise_Appl...]
As broadcast-groups with deprecated attributes in subsystem_migration.xml are valid, and
they don't match the current expected alternative behavior. So for this case, I was
wondering if there needs to be a transformer for them.
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
Assignee: Brian Stansberry
Fix For: 3.0.0.Alpha13
The handling of the notions of 'required' and 'nillable' don't comply
with the specs in
https://docs.jboss.org/author/display/WFLY/Description+of+the+Management+...,
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 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
(v7.2.3#72005)