Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64787121
Aha so the goal is to make validation rules discoverable. I like that.
TOSCA/HOT/Heat have quite a rich notion of this - ranges, regexes etc.
Would it be better for the API to start down this route? eg. imagine a
.constraint(Constraints.NON_NULL)
Of course with it @Beta we can do this in the future instead. So that
shouldn't block this.
But I am concerned about the non-null check in the EntityFactory. I had
missed that. I don't like this for a few reasons:
- it limits our ability to set config post-deployment but pre-use
(originally that wasn't the intent of config but it is becoming more common
and I think that's good actually)
- it means non-null is enforced only on entities, not on policies or
anywhere else they are used (I'm thinking it would be nice to rename
ConfigKey and Bag to DataKey and Bag, to emphasise they could be used in
many places)
With that in mind would it make more sense to enforce constraints (ie the
non-null check, at this point) at the point of the get() call?
So if we wanted to validate a config at any point we just get() it. And if
we did want to enforce an "all config is set" in entity init, that could be
an easy one-line convenience that goes through the statics.
Actually thinking about this, where it could be really useful is if we had
a URL_EXISTS constraint - so many places we check those explicitly.
Best
Alex
On 26 Nov 2014 18:05, "Sam Corbett" <[email protected]> wrote:
> I'd argue it does simplify the code. It is not only preventing set(null),
> it also provides a standard way to catch missing configuration when an
> entity is created. Currently, to check a ConfigKey was set you have to
> write something like:
>
> @Override
> public void init() {
> checkNotNull(getConfig(MY_CONFIG), "Entity requires a value for
MY_CONFIG");
> }
>
> This splits the statement of intent that a value for MY_CONFIG must be
> set when the entity is created away from definition of the key. There is
no
> feasible way for us to generate documentation that indicates that
> MY_CONFIG must be set.
>
> With the change in this pull request we can replace the check in the init
> method by defining the config key as:
>
> ConfigKey<Object> MY_CONFIG = ConfigKeys.newConfigKey(Object.class)
> .nonNull()
> .build();
>
> Attempts to create entities missing mandatory configuration are
> automatically caught in InternalEntityFactory. We can extend the jsgui to
> indicate required configuration when you're adding an app or an entity
(and
> even prevent submissions that are missing such values) or are browsing the
> catalogue. We can also indicate required configuration in the catalogue
> documentation on the Brooklyn website.
>
> The change is backwards compatible - all existing ConfigKeys remain
> nullable. The nulling-a-parents-key case is valid. I envisage this being
> used selectively rather than left, right and centre.
>
> It should definitely be annotated @Beta.
>
> â
> Reply to this email directly or view it on GitHub
>
<https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64686894>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
