Conductor: [grpc] Unable to pass integer value in workflow inputs

[sujaysudheenda]

Hi we are using the Netflix grpc workflow client (version:com.netflix.conductor:conductor-grpc-client:2.12.1) and we are unable to pass in a integer value in workflow input (or task inputs) during execution.

We are passing in dynamic workflows by using the startWorkflowRequest object.

We see the following error:
cannot map to Value type: 40

We suspect this is happening in here:
https://github.com/Netflix/conductor/blob/a7a6e32900b7c5cf78b861a144d3e663b9be11c3/grpc/src/main/java/com/netflix/conductor/grpc/ProtoMapper.java#L44

I understand that, these are generated files from protobuf (please correct me if am wrong).
Is it possible to update it to support integer values?

Happy to contribute as well, might need some assistance in updating this as I am new to this code base

Thanks in advance!

[sujaysudheenda]

@kishorebanala any suggestions/thoughts will be helpful 😃

[apanicker-nflx]

@sujaysudheenda Can you please provide a sample workflow input that you have tried and causes this issue?
@vmg @josedab Can you please help look into this?

[sujaysudheenda]

Sorry for missing the example in my original post. Here is a sample example that fails:

{
  "input": {
    "data": {
        "test": 1
    }
  }
}

If I specify test: 1.0 it works as it identifies it as Double

Also since we are using the GRPC client, we are setting the input on StartWorkflowRequest object

[vmg]

The interface we designed for GRPC is limited to "numeric" values (i.e. doubles) because of backwards compatibility with the old interface, IIRC. So all numbers need to be floating point. That's... not ideal. You could simply fix it by updating the toProto method, but I recall this was a breaking change so we did not do it.

[sujaysudheenda]

@vmg thank you for the response and suggestion.
Aren't these files auto-generated?
Also if I want to update this, should the integer also be converted to numberic?
I see that builder.setNumberValue takes in a Double. Is it possible for me to introduce a new method to the builder to save Int (Protobuf Int)?

Again I am pretty new to Protobuf and GRPC so my understand in the above question might be a bit off. Please assist

[vmg]

@sujaysudheenda: No, the file you've linked, ProtoMapper.java is not auto-generated. The rest of the protobuf code _is_, but not that implementation.

It is not possible to add a new method to the Value builder because Value is a standard ProtoBuf type: https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/Value.Builder -- these standard types do not support ProtoBuf INT fields because they need to be mapped 1:1 between ProtoBuf and JSON-ProtoBuf (the JSON specification does not have a concept of "integer", it only supports "numeric" values which are basically double).

Your best bet here to prevent breaking the ProtoBuf/GRPC spec is adding a type check to toProto that handles Java Integer and casts that to double, calling setNumberValue. You may lose precision here, but again, this is the only way that you'll be able to support the standard ProtoBuf Value.

Sorry this is inconvenient. I did not design this, Google did. :smile:

[sujaysudheenda]

😄 No worries! thanks for detailed explanation.

[sujaysudheenda]

@vmg I found another scenario where this is breaking for a valid use-case of having an integer.

I have a workflow that has HTTP task (workflow with only one task).

when I do workflowClient.getWorkflow(workflowInstanceId, true); using GRPC client.
it tries to parse the whole workflow object and during that, it fails to parse the Http status present in the task output after it has been executed.

Just posting it here for visibility, I totally understand this is not a bug in how you implemented this 😄

[sujaysudheenda]

Your best bet here to prevent breaking the ProtoBuf/GRPC spec is adding a type check to toProto that handles Java Integer and casts that to double, calling setNumberValue. You may lose precision here, but again, this is the only way that you'll be able to support the standard ProtoBuf Value.

@vmg do you think this will be a useful contribution to the open-source version of conductor? Just getting your thoughts before I open a PR as you are more familiar with this area