HistoricProcessInstanceQuery statefull invocations

[deg]

This remark is related to  http://forums.activiti.org/en/viewtopic.php?f=3&t=593

I was trying to reuse a created processInstanceQuery on activiti 5.16.3.  I assumed that finished and unfinished are each other opposites, just like deleted and notDeleted seem to be be each other opposites. So I expected that the code below

final HistoricProcessInstanceQuery processQuery = createQuery(procesDefinitionKey);
for (int i = 0; i < 3; i++) {
     System.out.println("Check[" + i + "] There are " + processQuery.finished().list().size() + " finished");
     System.out.println("Check[" + i + "] There are " + processQuery.unfinished().list().size() + " unfinished");
}
‍‍‍‍‍‍‍

would print something like this:

Check[0] There are 2 finished
Check[0] There are 0 unfinished
Check[1] There are 2 finished
Check[1] There are 0 unfinished
Check[2] There are 2 finished
Check[2] There are 0 unfinished
‍‍‍‍‍‍‍‍

instead it prints this:

Check[0] There are 2 finished
Check[0] There are 0 unfinished
Check[1] There are 0 finished
Check[1] There are 0 unfinished
Check[2] There are 0 finished
Check[2] There are 0 unfinished
‍‍‍‍‍‍‍‍

Looking into the source of HistoricProcessInstanceQueryImpl (Long live open source 🙂 ) i see that there are four booleans

protected boolean finished = false;
protected boolean unfinished = false;
protected boolean deleted = false;
protected boolean notDeleted = false;
‍‍‍‍‍‍

were i  would expect two. This allows <b>a query to be finished and unfinished, deleted and not deleted at the same time!</b> This results in strange and very unintuitive behavior.

[jbarrez]

Hmm you are correct that this is unexpected.

However, using two booleans here won't work: the query() without anything else, returns both finished and unfinished. So you've got 3 states, bot the boolean can only express two states. An option that then comes to mind is to use a Boolean (big B), so we can use null. But this will have the same problem as you post here: it would remember being set.

The real solution to this is probably to make the query objects immutable on each method call. But that's a huge rewrite that I can't do at the moment given other work … Unless you see another solution that would be quicker to implement?

[deg]

The problem now, as i see it, is that one concept is distributed over multiple datamembers. This allows for invalid states to be possible and  makes it more "difficult" to be consistent.

A query can be for a finished, unfinished or any process. In the same way it can be for for a deleted, not deleted or any process. The three "states" or represented by two doubles that are not kept in sync. When i invoke a deleted after an notDeleted the flags should be kept in sync. Which is now not the case.

I can also not "unselect" it. When i execute the code below (newly invented method added)

<code>
final HistoricProcessInstanceQuery processQuery = createQuery(procesDefinitionKey);
for (int i = 0; i < 3; i++) {
     System.out.println("Check[" + i + "] There are " + processQuery.finished().list().size() + " finished");
     System.out.println("Check[" + i + "] There are " + processQuery.unfinished().list().size() + " unfinished");
    System.out.println("Check[" + i + "] There are " + processQuery.resetFinished().list().size() + "  overall");
}</code>

I would expect something like this:
<code>
Check[0] There are 0 finished
Check[0] There are 3 unfinished
Check[0] There are 3 overall
Check[1] There are 1 finished
Check[1] There are 2 unfinished
Check[1] There are 3 overall
Check[2] There are 3 finished
Check[2] There are 0 unfinished
Check[2] There are 3 overall
</code>

To implement this i would probably just use an int. I don't like NULLs
<code>
protected int finished = 0;
protected int deleted = 0;
</code>

with the value 0 meaning not selected, 1 selected, -1 the oppossite of the flags meaning (unfinished, not deleted)
The setters on the QueryBuilder then just relate to the same field. With the addition of a new method that allows resetting the field to its default value.
But for each concept there is just one internal datamember. So no problems with inconsistent state. A query can not be for a deleted and not deleted process at the same time.

<code>
  public HistoricProcessInstanceQuery finished() {
    if (inOrStatement) {
      this.orQueryObject.finished = 1;
    } else {
      this.finished = 1;
    }
    return this;
  }
 
  public HistoricProcessInstanceQuery unfinished() {
    if (inOrStatement) {
      this.orQueryObject.unfinished = -1;
    } else {
      this.unfinished = -1;
    }
    return this;
  }

  public HistoricProcessInstanceQuery resetFinished() {
    if (inOrStatement) {
      this.orQueryObject.unfinished = 0;
    } else {
      this.unfinished = 0;
    }
    return this;
  }
 
  public HistoricProcessInstanceQuery deleted() {
    if (inOrStatement) {
      this.orQueryObject.deleted = 1;
    } else {
      this.deleted = 1;
    }
    return this;
  }
 
  public HistoricProcessInstanceQuery notDeleted() {
    if (inOrStatement) {
      this.orQueryObject.notDeleted = -1;
    } else {
      this.notDeleted = -1;
    }
    return this;
  }

public HistoricProcessInstanceQuery resetDeleted() {
    if (inOrStatement) {
      this.orQueryObject.notDeleted = 0;
    } else {
      this.notDeleted = 0;
    }
    return this;
  }
</code>


I would not make the queries immutable on each method call. Instead i would make immutable what's get passed to the command exeuctor. To me it is just the builder pattern. I build my query, i can constantly update or change my queryBuilder. When i run the query i would pass an immutable query object to the command executor. So the query builder can safely be used and modified again. The Query that was build is immutable.

So instead of this:
<code>
public U singleResult() {
    this.resultType = ResultType.SINGLE_RESULT;
    if (commandExecutor!=null) {
      return (U) commandExecutor.execute(this);
    }
    return executeSingleResult(Context.getCommandContext());
}
</code>

i would probably do something like this:

<code>
public U singleResult() {
    this.resultType = ResultType.SINGLE_RESULT;
    if (commandExecutor!=null) {
   final QueryCommand command = this.build()
          return (U) commandExecutor.execute(command);
    }
    return executeSingleResult(Context.getCommandContext());
}
</code>

Anyway, just my two cents.

regards,
DEG