Little improvements on API (close #26)

[henri-tremblay]

No description provided.

[mathieucarbou]

@henri-tremblay : ok for me when build will pass.

Would it be possible also to add a fix so that we do not have a NPE in the case the object of the query is not in the tree ?

I..e:

Instead of doing:

    TreeNode treeNode = ContextManager.nodeFor(contextObject);
    if(treeNode == null) {
      return;
    }

    Set<TreeNode> result = queryBuilder()
        .descendants()
        .filter(context(attributes(Matchers.<Map<String, Object>>allOf(
            hasAttribute("type", descriptor.getType()),
            hasAttribute("name", descriptor.getObserverName()),
            hasTags(descriptor.getTags())))))
        .filter(context(identifier(subclassOf(OperationStatistic.class))))
        .build().execute(Collections.singleton(treeNode));

We should be able to just do something like:

 Set<TreeNode> result = queryBuilder()
        .descendants()
        .filter(context(attributes(Matchers.<Map<String, Object>>allOf(
            hasAttribute("type", descriptor.getType()),
            hasAttribute("name", descriptor.getObserverName()),
            hasTags(descriptor.getTags())))))
        .filter(context(identifier(subclassOf(OperationStatistic.class))))
        .build().execute(Collections.singleton(ContextManager.nodeFor(contextObject)));

or:

        .build().execute(contextObject);

And the query system would just return an empty set of nodes because the queried objects are not there.

(related to Terracotta-OSS/terracotta-platform#263)

[henri-tremblay]

I'm ok with adding the change. One question though. Can it hide bugs? By returning an empty list instead of crashing and showing us right away that something is wrong?

[mathieucarbou]

I don't know... I'm not sure: the query system is only used by you and me at the moment.

In some case, yes it would make sense that we have for example something like: ContextManager.getNodeFor(contextObject) throw NoSuchElementException. In the case where you would want to make sure that the context object is in the tree.

But in my case, where I just want to query the tree to find some stats, I do not care if the object is there or not, I care about if I have results or not. In my case, I will have some tests elsewhere to verify that some context objects are well put in the tree and that I can have some stat descriptors associated to them. So if I send the wrong context object, and it is not in the tree, this "bug" would be catched at another place (I won't get stats or descriptors).

It's like we would need something like these perhaps ?

• ContextManager.getNodeFor(contextObject) throw NoSuchElementException
• .build().execute(contextObject); // would do .build().execute(asList(ContextManager.nodeFor(contextObject)));
• .build().execute(asList(ContextManager.getNodeFor(contextObject)));
• .build().execute(asList(ContextManager.nodeFor(contextObject))); // would tolerate nulls to not "search" childs of null root nodes

[henri-tremblay]

I think I will leave the PR as is. I had a look and the problem is that the behaviour is currently not consistant. You get a NPE when querying on descendants or children. But if you don't it returns null.

So I prefer to do this change in another PR. You can file an issue. And then we will figure out a coherent behaviour.

[mathieucarbou]

ok!