ByteArrayEntity: improve encapsulation

[marcus1]

I've run into a nasty bug in a custom variable type causing Foreign Key Constaint error in ACT_FK_VAR_BYTEARRAY.

The first part of solving the problem was to improve logging in DbSqlSession. This helped me diagnose what went wrong. These changes you can find in this pull request:

https://github.com/Activiti/Activiti/pull/66

Our custom variable type stores a list of JPA entities by storing the elements' ids in a the textValue field, and if it becomes too long, in a byte array field:

      String s = buffer.toString();
      if (s.length() > MAX_STR_LENGTH) {
         fields.setByteArrayValue(bytes);
         fields.setTextValue(null);
      }
      else {
         fields.setTextValue(s.length() == 0 ? null : s);
         fields.setByteArrayValue(null);
      }
‍‍‍‍‍‍‍‍‍‍‍

It turned out that the problem was that if setByteArrayValue is called multiple times, a new ByteArrayEntity is created each time.

Also, the foreign key byteArrayValueId in VariableInstanceEntity is updated, but this change is written to the db before the ByteArrayEntity is inserted, causing the Foreign Key Constaint violation.

So to avoid this problem we have to do:

      if (fields.getByteArrayValueId() != null) {
         fields.getByteArrayValue().setBytes(bytes);
      }
      else {
         fields.setByteArrayValue(bytes);
      }
‍‍‍‍‍‍‍‍

This is a problem of encapsulation. It is not obvious from the ValueFields interface that this is necessary, and thus quite error prone. Also it's inconsistent with how other entities work.

I'd like to change this and create a new pull request for it, but first wanted to check if it's okay with you.

While I'm at it, there are some changes to DbSqlSession I'd like to make:
- remove commented-out code
- move inner classes to bottom
- improve readability by extracting some common code

What do you think?

[rrludwig]

This issue causes some more trouble. Due to updates of these byte array ids, we run into race conditions. During an update of a value we find time slots where we get an id which is not valid anymore. These leads to NullPointerExceptions… 😞 But, we do not have a custom implementation, we just use the normal Activiti API and call getVariables() to HistoryService and RuntimeService quite frequently.

The issue is also responsible for the ticket I created: http://jira.codehaus.org/browse/ACT-1687

The handling of byte arrays and serializables need to be changed, in my opinion. The method setByteArrayValue mentioned above should be changed in the following way:

1) Check for existence of a former value. If there is one, perform an update in the database if the value to be set is not null.
2) If a former value exists, but the new value is null, perform a delete.
3) If no value exists and the new value is not null, perform an insert.
4) No former value and new value is null, do nothing.

Maybe, even a null value should be put into ACT_GE_BYTEARRAY table. So each serializable variable get a single, constant id.

[jbarrez]

@Marcus: I agree with what you are writing. If you create a pull request, we can continue the discussion there.

@rrludwig: thanks for reporting the issue. We will look into it.

But it sounds strange that this race condition occurs. Shouldn't transaction prevent this problem?

[marcus1]

Ok

[marcus1]

You can follow my progresss here if you want:

https://github.com/marcusk24/Activiti/commits/master

There are quite a number of changes, but I've tried to group the commits into understandable bite-size chunks.

It is already funtional. However, I still want to clean up the duplicate code in the classes that deal with ByteArrayEntities. I'll probably get to this tomorrow.

[marcus1]

Done: https://github.com/Activiti/Activiti/pull/83

[frederikherema1]

Thanks for your contribution, we're looking into it to get it reviewed.