Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constraints - serialization and {forbidden,required}{If,Unless} #999

Merged
merged 6 commits into from Sep 24, 2018

Conversation

Projects
None yet
4 participants
@ahgittin
Copy link
Contributor

commented Sep 21, 2018

No description provided.

@aledsage

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

retest this please

Build failure was environmental:

ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from git://github.com/apache/brooklyn-server.git
@tbouron
Copy link
Member

left a comment

I haven't done a throughout review but it looks good, it sure is a great addition!

But I'm wondering what happens if you set one of this new constraint directly in YAML blueprints? Would this be recognised @ahgittin ?

@@ -64,7 +66,7 @@ public boolean apply(@Nullable String resource, @Nullable BrooklynObject context

@Override
public String toString() {
return "ResourcePredicates.exists()";
return "ResourcePredicates.urlExists()";

This comment has been minimized.

Copy link
@tbouron

tbouron Sep 21, 2018

Member

Shouldn't this be simply urlExist()? Would be more consistent with the other predicate introduced by this PR

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

i don't follow -- why would urlExist be better?

@aledsage
Copy link
Contributor

left a comment

Looks really good - comments are pretty minor.

I've also played around with adding some more unit tests to get comfortable with what this does. I'll tidy those up and submit them in a PR once this is merged.

}
}
@SuppressWarnings({ "unchecked", "rawtypes" })
public List<Object> toExactJsonList(Predicate<?> constraint) {

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

Can we reduce the size of this seemingly-public api? And/or add @Beta to the class?

There are quite a few public things that are only called from this class.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

added @Beta. i can see all the public methods being useful.

PredicateSerializationRuleAdder.predicateListConstructor((o) -> ConfigConstraints.required()).
equivalentPredicates(Predicates.notNull(), StringPredicates.isNonBlank()).add(this);

PredicateSerializationRuleAdder.predicateListConstructor((o) -> Predicates.or((Iterable)o)).preferredName("any").equivalentNames("or").add(this);

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

I prefer us supporting just one name.

The advantage of one way is that our docs and examples are all consistent, it's simpler, and folk won't get distracted by the difference when troubleshooting (e.g. experience brooklyn devs still say "try doing X instead of Y" when both X and Y are supported, because that person is used to doing X so knows that way works but isn't as certain about Y).

The advantage of both ways is that folk can guess easier, and you can use the name you prefer for demos.

I therefore think one way is far better, on balance.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

if we just have one way then it has to be the toString() which is ugly to work with -- or we have a different parser for the toString(). given that we have Predicates.or and any, it would seem natural to have or as a similar word. the disadvantages of having multiple ways is much reduced when one way is explicitly preferred, esp if we are able to convert things to a canonical form which we do here. if you really don't like it i could drop any and all, just use or and and but don't think it's worth using the toString() or insisting on a separate parser to handle the conversion from the toString().

PredicateSerializationRuleAdder.stringConstructor(ConfigConstraints::requiredUnless).add(this);
}

public static ConstraintSerialization INSTANCE = new ConstraintSerialization();

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

Make it final, to avoid really weird behaviour things if someone accidentally changes it!


boolean parse() {
remaining = remaining.trim();
Matcher m = PATTERN_START_WITH_PREDICATE.matcher(remaining);

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

Observation: that looks fiddly. On balance, I think it's the right way to implement it. But back of my mind I don't want us to accidentally turn this into a full-blown DSL that's implemented with regex parsing.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

yeah it's not elegant but it gets the job done. more elegant alternative i think is a regex parser which supports balanced expressions but not worth bringing that in IMO.

@SuppressWarnings({ "unchecked", "rawtypes" })
private void init() {
PredicateSerializationRuleAdder.predicateListConstructor((o) -> ConfigConstraints.required()).
equivalentPredicates(Predicates.notNull(), StringPredicates.isNonBlank()).add(this);

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

These are equivalent in a UI where there is just a text box, but not equivalent in an input yaml file. I'm therefore very hesitant to turn StringPredicates.isNonBlank into the same meaning as Predicates.notNull.

By the way, I'm imagining we'll move to use this for parsing YAML blueprints (otherwise what is the point of making it bi-directional?). Is that your thinking as well?

If parsing strings from yaml blueprints, what else should we support? For example, should we allow StringPredicates.startsWith("a") and other such utilities? Very happy to keep it simpler for now, and not try to add support for things like that!

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

wondered about what else to add but didn't see a compelling reason for others

This comment has been minimized.

Copy link
@ahgittin

ahgittin Sep 22, 2018

Author Contributor

i wanted the default for required to be isNonBlank, but i think you're right notNull should be different.

}

public static Predicate<Object> forbiddenUnless(String otherKeyName) { return new ForbiddenUnlessPredicate(otherKeyName); }
public static class ForbiddenUnlessPredicate extends OtherKeyPredicate {

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

Make the classes protected at the very most. Otherwise someone will call the class' constructor directly, and we'll be stuck with it in our public API for ages to come.

}

private static abstract class OtherKeyPredicate implements BrooklynObjectPredicate<Object> {
private String otherKeyName;

This comment has been minimized.

Copy link
@aledsage

aledsage Sep 21, 2018

Contributor

Declare this final (pretty much wherever we can declare fields final, it's a good idea).

ahgittin added some commits Sep 22, 2018

forgot to use deserialization routines when parsing catalog input
also test and fix for some edge-cases in serializing

@ahgittin ahgittin referenced this pull request Sep 22, 2018

Merged

More constraints #72

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

all PR comments addressed. more exciting, it has been tested against apache/brooklyn-ui#72.

screen shot 2018-09-22 at 01 48 12

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

BTW you're both right, i forgot to apply the serialization to catalog input; i've fixed that also

@ahgittin ahgittin force-pushed the ahgittin:constraints branch from 27e95e0 to 3d214b8 Sep 22, 2018

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

retest this please

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

here's a blueprint to help test, showing all the supported constraints:

brooklyn.catalog:
  bundle: constraints-test1
  version: "0.0.1-SNAPSHOT"
  itemType: entity

  items:
    - id: constraint1
      name: "More constraints"
      iconUrl: https://upload.wikimedia.org/wikipedia/commons/c/cb/Foo_was_here.jpg
      tags:
      - ui-composer-hints:
            config-widgets:
              - key: sample-dropdown
                widget: suggestion-dropdown              
                suggestion-values:
                - value: hello
                - value: hi
                - value: bonjour
      item:
        type: org.apache.brooklyn.entity.stock.BasicEntity
        brooklyn.parameters:
         - name: sample-required
           constraints:
           - required
         - name: sample-dropdown
         - name: sample-regex
           constraints:
           - regex("h.*")
         - name: sample-regex2
           constraints:
           - regex: h.*
         - name: needs-another
           constraints:
           - forbiddenUnless: another
         - name: another
           constraints:
           - forbiddenIf: not-with-another
           - requiredIf: needs-yet-another
         - name: needs-yet-another         
         - name: not-if-needed-another
           constraints:
           - requiredUnless: needs-another
@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

build failure was due to being unable to checkout; have wiped workspace

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

retest this please

@aledsage

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Looks good - happy to merge, after I've build + run tests locally.

The jenkins PR build is still failing with the error below (I suspect it's a network connection problem while it's trying to download lots from github; I wonder if we can use --depth=1 for PR builds, or if that would mess up because it wants to check if the PR is mergeable etc):

Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress git://github.com/apache/brooklyn-server.git +refs/pull/*:refs/remotes/origin/pr/*" returned status code 128:

It would be good to also expand out the tests in ConfigParametersYamlTest, so that we regression-test it is always calling ConstraintSerialization (currently it just does required). I'll look at adding that, along with some more unit tests I was playing around with in ConstraintSerializationTest.

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

have added docs now too, so after merging this please look at:

@asfgit asfgit merged commit 3d214b8 into apache:master Sep 24, 2018

1 check passed

Jenkins: brooklyn-server-pull-request SUCCESS 5043 tests run, 0 skipped, 0 failed.
Details

asfgit pushed a commit that referenced this pull request Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.