[Feature Request] Worker unable to start with a custom Data Converter

[sdellarosa]

What are you really trying to do?

I would like to use a custom data converter that converts all payloads into an encrypted payload.

Describe the bug

I have created my own implementation of DataConverterInterface, which I supply to both the client and the worker. This new converter uses the default DataConverter from the SDK to convert a value into a Payload, then converts this payload into an encrypted payload (see class below).

class MyDataConverter implements DataConverterInterface
{
    public function __construct()
    {
        $this->defaultDataConverter = DataConverter::createDefault();
        $this->encryptedPayloadConverter = new EncryptedPayloadConverter();
    }

    public function toPayload($value): Payload
    {
        $payload = $this->defaultDataConverter->toPayload($value);

        return $this->encryptedPayloadConverter->encrypt($payload);
    }

    public function fromPayload(Payload $payload, $type): mixed
    {
        $payload = $this->encryptedPayloadConverter->decrypt($payload);

        return $this->defaultDataConverter->fromPayload($payload, new Type(TYPE::TYPE_ANY));
    }
}

class EncryptedPayloadConverter
{
    public function encrypt(Payload $payload): Payload
    {
        $encrypted = new Payload();
        $encrypted->setMetadata([EncodingKeys::METADATA_ENCODING_KEY => 'binary/encrypted']);
        $encrypted->setData($this->encryptString($payload->serializeToString()));

        return $encrypted;
    }

    public function decrypt(Payload $payload): Payload
    {
        $decrypted = new Payload();
        $decrypted->mergeFromString($this->decryptString($payload->getData()));

        return $decrypted;
    }
}

When using the client to start a workflow, I can see that the workflow execution can be successfully started, with the input arguments encoded in binary/encrypted, as I intended. But, when I start RoadRunner for the worker (./rr serve), I get the error below and my worker is unable to start.

2023-01-09T09:38:40.002Z        INFO    temporal        connected to temporal server    {"address": "temporal:7233"}
2023-01-09T09:38:40.367Z        DEBUG   server          worker is allocated     {"pid": 23, "internal_event_name": "EventWorkerConstruct"}
2023-01-09T09:38:40.742Z        DEBUG   server          worker is allocated     {"pid": 27, "internal_event_name": "EventWorkerConstruct"}
2023-01-09T09:38:40.742Z        DEBUG   temporal        outgoing message        {"id": 0, "data": "", "context": ""}
2023-01-09T09:38:40.816Z        DEBUG   temporal        received message        {"command": null, "id": 0, "data": "\n\ufffd\u0007*\ufffd\u0007\n\ufffd\u0006\n\u001c\n\u0008encoding\u0012\u0010binary/encrypted\u0012\ufffd\u0006sIHh0C5aq1zf0KD7+z1OEv5kpwdKu2KPDsn5E8YRvxuo/uM0CsXqOFBnWwbgZc7GoioM0aLiqxBdBO+VdtGzeNziooMpIHkAUVqR4QgsB8Ge1IarnIJl7BfEF8Pet2Ov9EMrmdGEamznpDULv1UwYe/qFRhQhTgP9YwLr+SzH0KycxodQ2+/Q2BXCKnt/b7wmNyQUD1v8vdSlp7gE3EpQBY9J/BBhoAx/URGj8ruGibJ5NiFSsLS40njvvxwsw1kpgODqjh7KhgxhuyYgMm6rdNVcpauAmV3RXSSHUKkyRoelh1jQbkxeDPvnp+Om6DWBh0CQPREOH/UbG2GXtwxrfsYdrBz+kZsWSQ7MsJExe0BfyyuoJD1XIAU8tMEqjejRLfHNKohwv8rcQzPGT9B1PETWREOdbj87uFzRDjQ1cgQD9F1BThjgluj6eOtRwGlaSGykSw2KVlUOp4VubCJOvCFIhbI4zUfPLmKp4woQoDEsBNoqdZNn3sTnVwyODdcejBNXGzyKfQEVV2gwGh+yqIbH8uBn27RAfYR5n4SUDv+FsYR1za03N19G1Ph5J32MSLoHzDkpgHJRvb9LkQf4a9m2VnntmPd9qa1Etl5LvCZRCM/3zkKvsHgjtJgypjbQHyKOJjI6nBe3ah0skuJgTApig3dPESGAmPT0J4NsnU79E/XIfJHtX8za/NwIyfehzpWqPoMWkYKJiKNejQybwCt8WdBjd6tyDnRMSDiGy/kRyTNkMNMMTreQI/0BawGHlRGZl7QVpjmyG+fpCMx62hgQz63xv6jSRxPVAAuruVz8Hdh34r4AnOb6TMMaAwG83n5NP9NGxC8uHHVR6uI0+Rgkg=="}
2023-01-09T09:38:40.830Z        DEBUG   server          worker destroyed        {"pid": 23, "internal_event_name": "EventWorkerDestruct"}
2023-01-09T09:38:40.841Z        DEBUG   server          worker destroyed        {"pid": 27, "internal_event_name": "EventWorkerDestruct"}
handle_serve_command: Serve error:
        endure_start:
        endure_serve_internal: Function call error:
        endure_call_serve_fn: got initial serve error from the Vertex roadrunner_temporal.Plugin, stopping execution, error: temporal_plugin_serve:
        workflow_definition_init:
        workflow_fetch_wf_info: encoding binary/encrypted: payload encoding is not supported

If I don't supply my custom data converter, my worker successfully starts with the following logs.

2023-01-09T09:37:11.500Z        INFO    temporal        connected to temporal server    {"address": "temporal:7233"}
2023-01-09T09:37:12.244Z        DEBUG   server          worker is allocated     {"pid": 23, "internal_event_name": "EventWorkerConstruct"}
2023-01-09T09:37:12.723Z        DEBUG   server          worker is allocated     {"pid": 27, "internal_event_name": "EventWorkerConstruct"}
2023-01-09T09:37:12.723Z        DEBUG   temporal        outgoing message        {"id": 0, "data": "", "context": ""}
2023-01-09T09:37:12.873Z        DEBUG   temporal        received message        {"command": null, "id": 0, "data": "\n\ufffd\u0004*\ufffd\u0004\n\ufffd\u0004\n\u0016\n\u0008encoding\u0012\njson/plain\u0012\ufffd\u0004{\"TaskQueue\":\"default\",\"Options\":{\"MaxConcurrentActivityExecutionSize\":0,\"WorkerActivitiesPerSecond\":0.0,\"MaxConcurrentLocalActivityExecutionSize\":0,\"WorkerLocalActivitiesPerSecond\":0.0,\"TaskQueueActivitiesPerSecond\":0.0,\"MaxConcurrentActivityTaskPollers\":0,\"MaxConcurrentWorkflowTaskExecutionSize\":0,\"MaxConcurrentWorkflowTaskPollers\":0,\"StickyScheduleToStartTimeout\":null,\"WorkerStopTimeout\":null,\"EnableSessionWorker\":false,\"SessionResourceID\":null,\"MaxConcurrentSessionExecutionSize\":1000},\"Workflows\":[{\"Name\":\"HealthWorkflow\",\"Queries\":[],\"Signals\":[]}],\"Activities\":[]}"}
2023-01-09T09:37:12.873Z        DEBUG   temporal        worker info     {"taskqueue": "default", "optionsError": "json: unsupported type: func(error)"}
2023-01-09T09:37:12.873Z        DEBUG   temporal        workflow registered     {"taskqueue": "default", "workflow name": "HealthWorkflow"}
2023-01-09T09:37:12.873Z        DEBUG   temporal        workers initialized     {"num_workers": 1}
2023-01-09T09:37:12.942Z        INFO    temporal        Started Worker  {"Namespace": "default", "TaskQueue": "default", "WorkerID": "default:709d473a-d9e1-40c7-8ceb-371f2dd1c17c"}
2023-01-09T09:37:12.942Z        DEBUG   rpc             plugin was started      {"address": "tcp://127.0.0.1:6001", "list of the plugins with RPC methods:": ["informer", "resetter", "temporal"]}

To try to at least start the worker, I decided to see if skipping conversion on the first received message would help. But to skip the message, I cannot just simply skip converting all JSON payloads because workflow input and result payloads are in the JSON format, and these are the most important for me to encrypt. So instead, I added the following to my payload converter.

class EncryptedPayloadConverter
{
    public function encrypt(Payload $payload): Payload
    {
        $json = json_decode($payload->getData(), associative: true);

        if ($json && is_array($json) &&
            array_key_exists('TaskQueue', $json) &&
            array_key_exists('Options', $json) &&
            array_key_exists('Workflows', $json) &&
            array_key_exists('Activities', $json)
        ) {
            return $payload;
        }

        $encrypted = new Payload();
        $encrypted->setMetadata([EncodingKeys::METADATA_ENCODING_KEY => 'binary/encrypted']);
        $encrypted->setData($this->encryptString($payload->serializeToString()));

        return $encrypted;
    }
}

With this added, I can see that my worker can successfully start, and my workflow tasks are picked up and my workflow execution is completed, with the results payload encoded with binary/encrypted. Based on this, I suspect that the issue is that one of the underlying systems (perhaps RoadRunner, Goridge, etc) do not support the custom encoding.

While this works, this is kind of a hacky solution, and there's no way of knowing if there is any other internal messages that cannot be converted with a custom encoding which could cause the worker to error out and stop. Is there any other way to solve this problem?

Minimal Reproduction

Follow the steps above.

Environment/Versions

• OS and processor: macOS Ventura (Intel)
• Temporal Version: 1.18.4, and SDK version: 2.3.1
• Docker

Additional context

[roxblnfk]

Can you provide logs about a worker annihilation?

Please add that section in your rr.yaml

logs:
    mode: development
    level: debug
    file_logger_options:
        log_output: "log.log"

[cv65kr]

@sdellarosa You are going to introduce new data converter type binary/encrypted which is not supported by Temporal GO-SDK (Roadrunner use GO-SDK under the hood).

Take a look here: https://github.com/temporalio/sdk-go/blob/6580cbe0aa41a8b515791f95c2c15bb37db1dab1/converter/default_data_converter.go#L28 https://github.com/temporalio/sdk-go/blob/6580cbe0aa41a8b515791f95c2c15bb37db1dab1/converter/composite_data_converter.go#L125

You need to check if Temporal itself supports own data converter.

[sdellarosa]

@roxblnfk This is what I got in the logs:

{"level":"INFO","ts":"2023-01-16T14:02:15.846Z","logger":"temporal    ","msg":"connected to temporal server","address":"temporal:7233"}
{"level":"INFO","ts":"2023-01-16T14:02:17.000Z","logger":"server      ","msg":"{\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"context\":{\"exception\":{\"class\":\"ErrorException\",\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"code\":0,\"file\":\"/var/www/vendor/symfony/error-handler/DebugClassLoader.php:331\"}},\"channel\":\"deprecation\",\"severity\":\"INFO\",\"timestamp\":\"2023-01-16T14:02:16.999+00:00\"}\n"}
{"level":"INFO","ts":"2023-01-16T14:02:17.001Z","logger":"server      ","msg":"{\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"context\":{\"exception\":{\"class\":\"ErrorException\",\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"code\":0,\"file\":\"/var/www/vendor/symfony/error-handler/DebugClassLoader.php:331\"}},\"channel\":\"deprecation\",\"severity\":\"INFO\",\"timestamp\":\"2023-01-16T14:02:17.001+00:00\"}\n"}
{"level":"INFO","ts":"2023-01-16T14:02:17.008Z","logger":"server      ","msg":"{\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"context\":{\"exception\":{\"class\":\"ErrorException\",\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"code\":0,\"file\":\"/var/www/vendor/symfony/error-handler/DebugClassLoader.php:331\"}},\"channel\":\"deprecation\",\"severity\":\"INFO\",\"timestamp\":\"2023-01-16T14:02:17.007+00:00\"}\n"}
{"level":"INFO","ts":"2023-01-16T14:02:17.041Z","logger":"server      ","msg":"{\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"context\":{\"exception\":{\"class\":\"ErrorException\",\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"code\":0,\"file\":\"/var/www/vendor/symfony/error-handler/DebugClassLoader.php:331\"}},\"channel\":\"deprecation\",\"severity\":\"INFO\",\"timestamp\":\"2023-01-16T14:02:17.041+00:00\"}\n"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.047Z","logger":"server      ","msg":"worker is allocated","pid":24,"internal_event_name":"EventWorkerConstruct"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.049Z","logger":"server      ","msg":"worker is allocated","pid":25,"internal_event_name":"EventWorkerConstruct"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.056Z","logger":"server      ","msg":"worker is allocated","pid":23,"internal_event_name":"EventWorkerConstruct"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.080Z","logger":"server      ","msg":"worker is allocated","pid":26,"internal_event_name":"EventWorkerConstruct"}
{"level":"INFO","ts":"2023-01-16T14:02:17.810Z","logger":"server      ","msg":"{\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"context\":{\"exception\":{\"class\":\"ErrorException\",\"message\":\"User Deprecated: The \\\"Spiral\\\\Attributes\\\\NamedArgumentConstructorAttribute\\\" interface extends \\\"Doctrine\\\\Common\\\\Annotations\\\\NamedArgumentConstructorAnnotation\\\" that is deprecated Implementing this interface is deprecated Use the Annotation @NamedArgumentConstructor instead.\",\"code\":0,\"file\":\"/var/www/vendor/symfony/error-handler/DebugClassLoader.php:331\"}},\"channel\":\"deprecation\",\"severity\":\"INFO\",\"timestamp\":\"2023-01-16T14:02:17.809+00:00\"}\n"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.836Z","logger":"server      ","msg":"worker is allocated","pid":67,"internal_event_name":"EventWorkerConstruct"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.837Z","logger":"temporal    ","msg":"outgoing message","id":0,"data":"","context":""}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.891Z","logger":"temporal    ","msg":"received message","command":null,"id":0,"data":"\n\ufffd\u0004*\ufffd\u0004\n\ufffd\u0004\n\u0016\n\u0008encoding\u0012\njson/plain\u0012\ufffd\u0004{\"TaskQueue\":\"default\",\"Options\":{\"MaxConcurrentActivityExecutionSize\":0,\"WorkerActivitiesPerSecond\":0.0,\"MaxConcurrentLocalActivityExecutionSize\":0,\"WorkerLocalActivitiesPerSecond\":0.0,\"TaskQueueActivitiesPerSecond\":0.0,\"MaxConcurrentActivityTaskPollers\":0,\"MaxConcurrentWorkflowTaskExecutionSize\":0,\"MaxConcurrentWorkflowTaskPollers\":0,\"StickyScheduleToStartTimeout\":null,\"WorkerStopTimeout\":null,\"EnableSessionWorker\":false,\"SessionResourceID\":null,\"MaxConcurrentSessionExecutionSize\":1000},\"Workflows\":[{\"Name\":\"HealthWorkflow\",\"Queries\":[],\"Signals\":[]}],\"Activities\":[]}"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.892Z","logger":"temporal    ","msg":"worker info","taskqueue":"default","optionsError":"json: unsupported type: func(error)"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.893Z","logger":"temporal    ","msg":"workflow registered","taskqueue":"default","workflow name":"HealthWorkflow"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.894Z","logger":"temporal    ","msg":"workers initialized","num_workers":1}
{"level":"INFO","ts":"2023-01-16T14:02:17.955Z","logger":"temporal    ","msg":"Started Worker","Namespace":"default","TaskQueue":"default","WorkerID":"default:0a537940-a86b-4ffd-8ad8-5dae21ea5a12"}
{"level":"DEBUG","ts":"2023-01-16T14:02:17.957Z","logger":"rpc         ","msg":"plugin was started","address":"tcp://127.0.0.1:6001","list of the plugins with RPC methods:":["informer","resetter","temporal"]}

@cv65kr Thanks, I was thinking it would indeed be related to the GO-SDK. Do you know if it's somehow possible to supply the same custom data converter there as well?

[cv65kr]

@sdellarosa there are examples in Go - https://github.com/temporalio/samples-go/tree/main/encryption https://github.com/temporalio/samples-go/tree/main/codec-server

I think it's not simple with roadrunner since you need create your own data converter and it needs to be compiled with Roadrunner. And ofc if you want to have a support of new converter in UI, you need to expose converter via server.

Velox could be answer here. Maybe @rustatian can jump here? Or maybe converter server is enough :)

[rustatian]

Hey @cv65kr @sdellarosa 👋🏻

RR doesn't support the binary/encrypted payloads atm. But, that's easy to support that case. I need some repo to reproduce this case... 😃 @sdellarosa @cv65kr Could you guys create a test-case repo?

[cv65kr]

@rustatian, binary/encrypted is not Temporal thing, is more custom thing and client implementation can be different. The problem I think is that you can have own codecs, not only this one case. I mean encryption/decryption is most popular thing and don't know if other case will be needed (but here you can have two cases, whole payload encryption and single field encryption).

https://docs.temporal.io/security#payload-codec

[wolfy-j]

@rustatian do we actually need to unpack all the payloads on RR end? Can't we just passthough payloads with undefined type to the server, I assume we only have to work with our internal commands.

[rustatian]

@rustatian, binary/encrypted is not Temporal thing, is more custom thing and client implementation can be different. The problem I think is that you can have own codecs, not only this one case. I mean encryption/decryption is most popular thing and don't know if other case will be needed (but here you can have two cases, whole payload encryption and single field encryption).

https://docs.temporal.io/security#payload-codec

Yeah, I know, thanks, I need to verify with the test-repo, why we can't just redirect that payload to the custom data-converter in the PHP. Why that case leads to error. I know the particular line which throws an error, but I need to double-check.

[rustatian]

@rustatian do we actually need to unpack all the payloads on RR end? Can't we just passthough payloads with undefined type to the server, I assume we only have to work with our internal commands.

That is because I need a test-repo to check that case. We do not touch the user's payload in any way (except, you know, marshal-unmarshal, encode-decode).

[roxblnfk]

In a month, we might implement interceptors. Using interceptors you could encrypt all payloads easily.

[cv65kr]

@rustatian in case when you still need test repo https://github.com/cv65kr/temporal-sdk-leaks/tree/data-converter

Error is caused because as a fallback RR using default converters from GO-SDK - https://github.com/temporalio/roadrunner-temporal/blob/master/data_converter/converter.go#L28

[cv65kr]

@roxblnfk cool, looking forward!

[rustatian]

@rustatian in case when you still need test repo https://github.com/cv65kr/temporal-sdk-leaks/tree/data-converter

Error is caused because as a fallback RR using default converters from GO-SDK - https://github.com/temporalio/roadrunner-temporal/blob/master/data_converter/converter.go#L28

Yeah, I know where it fails 😄 I just needed some test-case for that. Moreover, I don't think the interceptor is a good (and natural) place to decode the payload (since this is a data-convertor task).

[rustatian]

Hm, so, the issue is here:

// FromPayload converts single value from payload.
func (r *DataConverter) FromPayload(payload *commonpb.Payload, valuePtr any) error {
    return r.dc.FromPayload(payload, valuePtr) <-----
}

Since we're converting the payload like this err = c.dc.FromPayload(payloads[i], tmp). And later, in the CompositeDataConverter (temporal default), temporal tries to take a payload converter for the binary/encrypted, and, obviously, there are no such converters. And it fails with the error from the Bug.

Might be we need to implement the CompositeDataConverter (to support all std payloads), but with a slightly different behavior -> paththrough all payloads with an unknown encoder to the PHP.

[rustatian]

In only-Go, this is an expected error because, for sure, you should create a data converter for your own encoder... But here, since we don't touch the user payload, we may decode/encode well-known data types, but for the user-defined encoders, we should not return an error and pass this data to the PHP worker with the hope that PHP knows how to handle it.

[rustatian]

It looks like an easy task 😄 Since we don't actually need to decode the encrypted payload, we may easily detect the underlying data type with the reflection and use the standard decoder's approach for such payloads. Since, for example, binary/encrypted is binary first and then encrypted ⚡

[rustatian]

As is typical, upon delving into this task, it has become somewhat more complex 😄

When the user sends data with an unknown converter, such as binary/encrypted, it is not possible to decrypt it in order to allocate workers. Specifically, we have a payload and a valuePtr pointer, which points us to the actual type that should be filled with the data in the payload. This is a standard method for converting data from a payload to its true type, also known as unmarshalling.

In the event that the payload is encrypted or encoded with an unknown converter, it cannot be decoded to the "real" type. For example, we have a valuePtr pointing to a structure with an arbitrary number of fields. We have two scenarios with two different converters:

1. json/plain: all we need to do is unmarshal the "[]byte" into the schema (our structure). The process is successful and the structure (schema) is now filled with the data.
2. binary/encrypted (or any unknown converter): Without knowing how to unmarshal, for example, this: "ChYKCGVuY29kaW5nEgpqc29uL3BsYWluEtkEeyJUYXNrUXVldWUiOiJk" into our real type, it is not possible to fill our valuePtr with the data.

The good news is that this task can be solved by allowing users to implement RR's plugin-data-converters in Go, similar to middleware for HTTP.

[cv65kr]

@rustatian If I good understand correctly payload = data + metadata, maybe we could prepare fallback data converter in GO with just plain []byte conversion, so every time when type in not recognised - we just forward it (it forces PHP to returns always byte[] structure).

Logic responsible e.g. for encrypting should be done in PHP.

My point that logic is kinda duplicated, because in Go we have converters and in PHP we have converters.

[rustatian]

My point that logic is kinda duplicated, because in Go we have converters and in PHP we have converters.

Yeah, correct. I wouldn't say I like this solution, but this is the only one acceptable. We can't just forward the data, because we have to unmarshal it to the valuePtr (pointer to the real data). We can't transform the []byte into the structure if the data is encrypted.

[rustatian]

Ok 😄 We discussed with @wolfy-j and found a better solution.

The main problem is in the commands between the RR and PHP workers. In the case of the custom converter, we can't decode this info. We don't care about the user's data (in a good sense) because we just path through it to the Temporal (activity, workflow, etc).

The proposed solution: For internal communication, use the plain/json (or binary/proto) converter (one of the well-known). Having that, we will always be capable of decoding our internal messages w/o touching the user's data. And, we don't need to duplicate the data-converter in Go and PHP.

EDIT: The user's data will be encoded with the user's (in PHP) data converter.

[cv65kr]

Would be nice to see some example in samples-php 😄

[sdellarosa]

Hi all, thanks a lot for looking into this! 😄

Just to be clear, the final proposed solution, would be implemented in this SDK and not on RoadRunner, correct? And if so, when can I expect to see the changes shipped?

[rustatian]

Hey @sdellarosa 👋🏻

Just to be clear, the final proposed solution, would be implemented in this SDK and not on RoadRunner, correct?

Yeah, it seems that the SDK would be updated.

And if so, when can I expect to see the changes shipped?

Hard to say, personally, as I'm not a PHP dev, we have to wait for @roxblnfk or someone from the community 😢

[cv65kr]

@rustatian big disadvantage of this solution when you are using well-known converters you are loosing possibility e.g. to decrypt payload in Temporal UI.

[cv65kr]

@sdellarosa as a temporary solution you could something like that:

<?php

declare(strict_types=1);

namespace App;

use Temporal\DataConverter\DataConverterInterface;
use Temporal\DataConverter\DataConverter;
use Temporal\Api\Common\V1\Payload;
use Temporal\DataConverter\EncodingKeys;

class Converter implements DataConverterInterface
{
    private DataConverterInterface $dataConverter;

    public function __construct()
    {
        $this->dataConverter = DataConverter::createDefault();
    }

    public function toPayload($value): Payload
    {
        $payload = $this->dataConverter->toPayload($value);

        // It's example object which need to be encoded
        if ($value instanceof Example) {
            $encrypted = new Payload();
            $encrypted->setMetadata([
                EncodingKeys::METADATA_ENCODING_KEY => 'json/plain',
                'encrypted' => 'true'
            ]);
            $encrypted->setData(
                base64_encode($payload->getData())
            );

            return $encrypted;
        }

        return $payload;
    }

    public function fromPayload(Payload $payload, $type): mixed
    {
        if ($payload->getMetadata()->offsetGet('encrypted') === 'true') {
            $decrypted = new Payload();
            $decrypted->setMetadata([EncodingKeys::METADATA_ENCODING_KEY => 'json/plain']);
            $decrypted->setData(base64_decode($payload->getData()));
            return $this->dataConverter->fromPayload($decrypted, $type);

        }

        return $this->dataConverter->fromPayload($payload, $type);
    }
}

In this case concrete object Example is encrypted in both directions via base64.

Need to be aware about my previous comment.

[rustatian]

@rustatian big disadvantage of this solution when you are using well-known converters you are loosing possibility e.g. to decrypt payload in Temporal UI.

Possibly, but personally, I don't think that this is a disadvantage since data is encrypted (no one, even accidentally would see it). Unfortunately, we have to choose the best from the worst solutions, since we can't decode payload unknown to the RR converter 😢

[rustatian]

@sdellarosa Hey 👋🏻 Have you tried the prev answer? Did that help you?

[roxblnfk]

Hi. Feel free to reopen the issue if it is still actual.