Hey criew,

thanks for bringing this up. We will have a look as soon as possible and keep you up to date with any insights in this ticket.

Best,
Tobias

Hey criew,

thanks again for providing this with all the details, that helped a lot in getting to the core of this faster!
I can reproduce this and I think it's valid to assume the transient flag is preserved correctly for TypedValueBuilders as well

I have a simple solution in mind that I will give a try within the next couple of days.
I will keep you posted with the results of that.

In case this needs more code than I currently assume, we will prioritize this properly for our roadmap.
It might be that we don't fix this fast then since using "create()" with the builder before setting the variable solves this quite conveniently and also is the documented way of using the object values.

We'll keep you in the loop.

Thanks again and best regards,
Tobias

Hi criew,

I have created a potential solution with the following pull request:
https://github.com/camunda/camunda-bpm-platform/pull/1761

However, in order to conveniently access the transient flag value of a builder, the interface would need to be extended.
While that is generally fine, this would trickle down into all implementing classes of this interface.
This includes other libraries like Camunda Spin for example.
Potentially, custom value types implemented by users based on this interface would also have to be updated.

While this is all doable in theory, our current assessment is that the benefits don't outweigh the costs here.
There is a quite convenient workaround in calling the #create method on the builder prior to setting it as a variable on an execution.
Plus, as I mentioned, this is also the documented and encouraged way of using the API.

Unless there are evident benefits to allow setting a builder instead of a typed value, we will not adjust the current behavior.

Best,
Tobias

Hi criew,

I am closing this ticket due to inactivity for now.
That said, feel free to comment at your leisure and we can reopen and revisit this if needed.

Cheers,
Tobias

Hi Tobias,

I did not have time to review it, yet. Of cource new interface methods was not intended. On the other hand, it is a clear bug - maybe it is a good option to throw an illegal state if someone passes a builder?

Hi criew,

I would object to the fact that this is a clear bug.
Passing on anything else than a typed value is simply handled as an unknown object value. We don't know anything about this value's transient state.
The API of the typed value builder does not contain any method to retrieve the transient state, so we cannot know.

However, I just had another idea on how to unintrusively do this as well without adjusting any API.
The PR is running our internal CI now to check if this works all fine.
I will let you know how this goes.

Best,
Tobias

Mmh, the "clear bug" was related to this code piece:

public static TypedValue untypedValue(Object value) {
  if (value instanceof TypedValue) {
    return untypedValue(value, ((TypedValue) value).isTransient());
  }
  else return untypedValue(value, false);
}

/**
 * Creates an untyped value, i.e. {@link TypedValue#getType()} returns <code>null</code>
 * for the returned instance.
 */
public static TypedValue untypedValue(Object value, boolean isTransient) {
  if(value == null) {
    return untypedNullValue(isTransient);
  } else if (value instanceof TypedValueBuilder<?>) {
    return ((TypedValueBuilder<?>) value).setTransient(isTransient).create();
  } else if (value instanceof TypedValue) {
    TypedValue transientValue = (TypedValue) value;
    if (value instanceof NullValueImpl) {
      transientValue = untypedNullValue(isTransient);
    } else if (value instanceof FileValue) {
      ((FileValueImpl) transientValue).setTransient(isTransient);
    } else if (value instanceof AbstractTypedValue<?>) {
      ((AbstractTypedValue<?>) transientValue).setTransient(isTransient);
    }
    return transientValue;
  }
  else {
    // unknown value
    return new UntypedValueImpl(value, isTransient);
  }
} 

1. untypedValue(Object) does not just wrap, but checks if the passed Argument is a TypedValue and if yet, preserves the transient state.
2. untypedValue(Object, boolean) also checks for TypeValue special cases and in addition also checks for TypedValueBuilders and overrides the transient flag before creation

For me, this is a hack.

I would just add an additional check for TypedValueBuilder in the first method (may not compile, just hacked in JIRA):

public static TypedValue untypedValue(Object value) {

    // additional check for typedValueBuiler
    if (value instanceof TypedValueBuilder<?>) {
       TypedValue created = ((TypedValueBuilder<?>) value).create();
       return untypedValue(created, created.isTransient());
    } else

    if (value instanceof TypedValue) {
        return untypedValue(value, ((TypedValue) value).isTransient());
    }
    else
        return untypedValue(value, false); 
}

Hi criew,

I agree that handling the TypedValueBuilder in the untypedValue(Object value, boolean isTransient) method but not in the untypedValue(Object value) method is inconsistent, yes
I applied a change very similar to yours in the pull request. I also added some tests covering builders as well now and our CI is also fine with it
We will run it through our internal code review and eventually merge it if there are no other concerns.

Best,
Tobias

Hi criew,

we merged the fix from https://github.com/camunda/camunda-bpm-platform/pull/1761.
Feel free to try this with the next alpha version of 7.17.

Thanks again for bringing this up and for the constant feedback and discussion

Best,
Tobias