[wildfly-dev] 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 <
brian.stansberry at redhat.com> 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
> wildfly-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/wildfly-dev




-- 
---
Harald Pehl
JBoss by Red Hat
http://hpehl.info
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/wildfly-dev/attachments/20160906/5a92afc9/attachment-0001.html